Skip to content

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

Merged
merged 1 commit into from
Jul 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Added

- `PluginClientFactory` to create `PluginClient` instances.

### Deprecated

- The `debug_plugins` option for `PluginClient` is deprecated and will be removed in 2.0. Use the decorator design pattern instead like in [ProfilePlugin](https://github.com/php-http/HttplugBundle/blob/de33f9c14252f22093a5ec7d84f17535ab31a384/Collector/ProfilePlugin.php).
Expand Down
21 changes: 21 additions & 0 deletions spec/PluginClientFactorySpec.php
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');
}
}
55 changes: 55 additions & 0 deletions src/PluginClientFactory.php
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;
Copy link
Member

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).

Copy link
Contributor Author

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.


/**
* 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 = [])
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deos simply return static::$factory(...) not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only in PHP 7+

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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".
See https://3v4l.org/WA5ek.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
}