-
Notifications
You must be signed in to change notification settings - Fork 53
add plugin client factory #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Common; | ||
|
||
use Http\Client\HttpClient; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class PluginClientFactorySpec extends ObjectBehavior | ||
{ | ||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Common\PluginClientFactory'); | ||
} | ||
|
||
function it_returns_a_plugin_client(HttpClient $httpClient) | ||
{ | ||
$client = $this->createClient($httpClient); | ||
|
||
$client->shouldHaveType('Http\Client\Common\PluginClient'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
<?php | ||
|
||
namespace Http\Client\Common; | ||
|
||
use Http\Client\HttpAsyncClient; | ||
use Http\Client\HttpClient; | ||
|
||
/** | ||
* @author Fabien Bourigault <bourigaultfabien@gmail.com> | ||
*/ | ||
final class PluginClientFactory | ||
{ | ||
/** | ||
* @var callable | ||
*/ | ||
private static $factory; | ||
|
||
/** | ||
* Set the factory to use. | ||
* The callable to provide must have the same arguments and return type as PluginClientFactory::createClient. | ||
* This is used by the HTTPlugBundle to provide a better Symfony integration. | ||
* | ||
* @internal | ||
* | ||
* @param callable $factory | ||
*/ | ||
public static function setFactory(callable $factory) | ||
{ | ||
static::$factory = $factory; | ||
} | ||
|
||
/** | ||
* @param HttpClient|HttpAsyncClient $client | ||
* @param Plugin[] $plugins | ||
* @param array $options { | ||
* | ||
* @var string $client_name to give client a name which may be used when displaying client information like in | ||
* the HTTPlugBundle profiler. | ||
* } | ||
* | ||
* @see PluginClient constructor for PluginClient specific $options. | ||
* | ||
* @return PluginClient | ||
*/ | ||
public function createClient($client, array $plugins = [], array $options = []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this method dynamic but the factory callable static? this means i must instantiate the factory, but i can only set one single callable. why not make the callable an optional constructor argument? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The setFactory is static as we need to change the implementation at runtime when bundle profiling is enabled and, as we aim to do 0config integration, we can't have something non-static here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, i see. maybe a bit of comment in the code would help so we remember in the future ;-) |
||
{ | ||
if (static::$factory) { | ||
$factory = static::$factory; | ||
|
||
return $factory($client, $plugins, $options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deos simply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only in PHP 7+ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. weird. can you add a comment that this is to work with php 5 and can be removed in 7? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wrong. It doesn't work in any PHP version: "Function name must be a string". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alright. then maybe a comment that using it directly does not work at least up to php 7.1? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't work too in 7.2.0alpha2. IMHO it doesn't worth to write a comment about something not supported by the language itself. |
||
} | ||
|
||
return new PluginClient($client, $plugins, $options); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the workaround for not having an interface? If so, having an interface for this is still less evil. This solution provides zero type safety (even if the support PHP versions do not support return types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the stuff is marked internal, we choosed to avoid over engineering and to not use an interface.