Skip to content
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

Add getter/setter to the plugin client #43

Closed
wants to merge 5 commits into from

Conversation

Baachi
Copy link

@Baachi Baachi commented Jan 13, 2016

Hey guys,

currently there is no way to add dynamicly plugins to client.
I need this for the integration in the php-gitlab-api package, because
i want to attach the AuthenticationPlugin and the ErrorPlugin at runtime.

If there is a better way, let me think!

@@ -69,4 +69,19 @@ function it_throws_loop_exception(HttpClient $client, RequestInterface $request)

$this->shouldThrow('Http\Client\Plugin\Exception\LoopException')->duringSendRequest($request);
}

function it_adds_a_plugin(Plugin $plugin)
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if it is the right way, i never worked with phpspec.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine.

*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin)
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming to "appendPlugin"? Because you are appending items to the end of a queue. It would be more clear that the order of the plugins is important.

Copy link
Author

Choose a reason for hiding this comment

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

I come from the symfony world and they use always add or addXXX but appendPlugin sounds reasonable.
@sagikazarmark What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really think it matters, both sounds OK. The reason why I usually prefer add is that it fits into the three character long actions: add, get, set, has

Copy link
Member

Choose a reason for hiding this comment

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

Sure, use addPlugin. But make sure to write a comment that the plugin added is appended to the end of the plugin queue

Copy link
Member

Choose a reason for hiding this comment

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

👍 Agree, this behavior is crucial to be mentioned as the order of plugins matter. Probably this is a reason against allowing to add plugins runtime: it can cause confusions.

@sagikazarmark
Copy link
Member

Well, I remember something about building the plugin chain. I think it makes it possible to add plugins runtime. Nowever this needs @joelwurtz 's confirmation.

Personally I think it is better to remove authentication and other stuff like that from the API.
The fact that your API client uses HTTP endpoints doesn't really matter from your application point of view.

Currently I am working on an API client which has three kinds of backends: REST, Cache (caching heavy requests) and a mock backend using fixtures.

That said, this is something that depends on the personal taste. With HttplugBundle + your custom api bundle you can hide this detail from your API client: you configure the API client and the HTTP Client and just inject it to the API Client. No matter if it is a PluginClient or just a guzzle adapter.

Another scenario: you use some kind of API proxy in front of the real API. Same endpoints, but no authentication data required as the proxy handles that. You could use the same API client, but you can't because of the burnt-in API credentials.

@Baachi
Copy link
Author

Baachi commented Jan 13, 2016

@sagikazarmark Yeah i already thought about to remove this "dynamic" from it. But currently anyone can use this library which effort, just $client = new Gitlab\Client('https://git.example.org');;.
If i remove it he have to do it:

$http = new PluginClient(HttpClientDiscovery::find());
$http->addPlugin(new Gitlab\ErrorPlugin()); // This needs the library
$http->addPlugin(new AuthenticationPlugin(new GitLabAuthentication(Client::AUTH_OAUTH_TOKEN, 's3cr3t', 'john'))); // GitLab has it own authentication algorithm

$client = new Gitlab\Client('https://git.example.org', $http);

Or i'm still wrong?

$this->options = $this->configure($options);
}

/**
* Adds a plugin to the end of the queue.
Copy link
Author

Choose a reason for hiding this comment

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

@Baachi Baachi force-pushed the feature/getter-pluginclient branch from 0e4e92d to a70d550 Compare January 13, 2016 15:28
@dbu
Copy link
Contributor

dbu commented Jan 13, 2016

i think we originally designed the plugin client with the idea that you will have a way to know which plugins you need at the time you instantiate the client. and that you use additional configurations if you have different use cases for clients. mutability can lead to all kind of confusion.

if we start allowing to modify the plugins, we would also need a way to check whether a plugin is already configured, or you might add ErrorPlugin twice. @joelwurtz and i discussed that idea at symfony con if i remember correctly, and we discussed it in the group and came to the conclusion that its not a good idea.

if we would not change this, there are two ways you could handle this:

  1. document how to set up the client
  2. wrap the client passed by the library user in your own PluginClient. however, i am not sure if restarting the request in a nested PluginClient would lead to auth still being added.

maybe a compromise of both? if you get passed a PluginClient, assume that the user read the doc and did add the necessary plugins, otherwise create a PluginClient around the passed client?

*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin)
Copy link
Contributor

Choose a reason for hiding this comment

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

if we decide to want this functionality, i would call this appendPlugin, i find that more precise than add. we might also want prependPlugin at some point...

Copy link
Member

Choose a reason for hiding this comment

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

With prepend added it would indeed make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I would be ok with addPlugin in normal.

However, like described in our docs, order of plugins matters, so big 👍 for the appendPlugin / preprendPlugin method name.

*
* @param Plugin[] $plugins
*/
public function setPlugins(array $plugins = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be appendPlugins, setPlugins makes me expect that this would replace the existing plugins (which would be a bad idea)

@sagikazarmark
Copy link
Member

If i remove it he have to do it

Not necessarily. You can always create a factory method with a default client. I remember we also planned something like a PluginClientFactory.

You can also accept a null http client and create one inside the constructor. But that would mean you have to rely on the discovery inside your API client, which is not necessarily bad, but comes at a price.

@Baachi
Copy link
Author

Baachi commented Jan 13, 2016

@dbu I know that modifiy the state of the object can lead to error but why not simply check inside the addPlugin method if the given plugin is already in the queue, if yes throw an exception.

document how to set up the client

This was my second intention (see my comment above, but i don't know if its best practice, because it would make the usage of this library a little bit harder.

wrap the client passed by the library user in your own PluginClient. however, i am not sure if restarting the request in a nested PluginClient would lead to auth still being added.

This is what i try to avoid, because this will definitly lead to errors. I'm not a big fan of create or wrap objects inside a constructor.

@Baachi
Copy link
Author

Baachi commented Jan 13, 2016

@sagikazarmark This indeed a good idea, a factory which create a standard client which have already the needed plugins configured. Maybe this is the way to go.

@dbu
Copy link
Contributor

dbu commented Jan 13, 2016

there are some valid use cases for adding multiple instances of the same plugin class. and mutability can be a more subtle problem. for example, if that same plugin client is also used for other requests, your users credentials might suddenly get pushed to other end points.

i think i would recommend the way mark proposes. then the user can either use your factory or do their own stuff:

$gitlab = new Gitlab\Client('https://git.example.org', Gitlab\HttpClientFactory::create('john', 's3cr3t'));

// factory is something like
public static function create($user, $pass, Client $client = null) 
{
if (!$client) {
    $client = HttpClientDiscovery::find();
}
return new PluginClient($client, [
    new Gitlab\ErrorPlugin(), // This needs the library
    new AuthenticationPlugin(
         // GitLab has it own authentication algorithm
        new GitLabAuthentication(Client::AUTH_OAUTH_TOKEN, 's3cr3t', 'john')),
]);
}

or they do their own client and its their job to instantiate the plugins you need.

@joelwurtz
Copy link
Member

I'm not a fan of this overall, currently plugins are injected by constructor and are somehow immutable.

Having a add / append / preprend method will not assure someone that the plugin stack is consistent as how it was originally defined.

For your use case it's the same thing than having different configuration for a HttpClient, you should have multiple plugin stacks :

$basePluginClient = new PluginClient($httpClient, [$plugin1, $plugin2]);

// Decorate (preprend plugin)
$authenticatedPlugin = new AuthenticationPlugin();
$extendedPluginClient = new PluginClient($basePluginClient, [$authenticatedPlugin]);

// New plugin stack (if you need to append)
$extendedPluginClient = new PluginClient($httpClient, [$plugin1, $plugin2, $authenticatedPlugin]);

However i find the current way of creating plugin client and stack not so user friendly, and want to add in a near futur a factory system which would allow to do the following :

$pluginClientFactory = new PluginClientFactory();
$pluginClient = $pluginClientFactory->create($httpClient, [
    'authentication' => [$myAuthenicationMethod],
    'cookie',
    'error',
    'log' => [$logger],
    ...
]);

So when you are in this use case (you need different plugin stack) the factory will be the recommended way to handle that.

@dbu
Copy link
Contributor

dbu commented Jan 14, 2016

@Baachi do you think the inputs from joel and me could solve what you want to do well enough? i now remember the main reason we did not want mutable plugin lists inside the PluginClient is that the order of plugins highly matters, at least for some of the plugins. we said the responsability should be in the code before the PluginClient is instantiated.

@Baachi
Copy link
Author

Baachi commented Jan 14, 2016

For sure, you give me a lot of idea's how to solve this problem, thanks a lot. I will close this PR, because why introduce unneeded complexity.

Guys you do a great job with this project 👍

@dbu
Copy link
Contributor

dbu commented Jan 14, 2016

great that the project is useful to you! don't hesitate to ask for further help if you need, in issues or the php-http slack channel.

note to self: we should put some of what was discussed here into the documentation, created an issue to remember: php-http/documentation#75

@sagikazarmark
Copy link
Member

@Baachi thanks for the kind words. Glad this is useful for you as well.

The plugin factory is also a good idea.

@dbu
Copy link
Contributor

dbu commented Jan 14, 2016 via email

@sagikazarmark
Copy link
Member

I meant the generic one that Joel mentioned.

@dbu
Copy link
Contributor

dbu commented Jan 14, 2016

i am not sure a generic one makes sense. if its

class PluginFactory
{
    static function create($client, $plugins) {
         return new PluginClient($client, $plugins);
    }
}

that is too trivial to make sense. if it optionally uses the client factory to create the client itself, its still not much use. the only hard thing is collecting all plugins an application needs to be active, and that is not something the factory can help with.

@joelwurtz
Copy link
Member

To develop, my view is that each plugin has it's coresponding factory like a AuthenticationPluginFactory each of this define a name and a way to be instanciate

class AuthenticationPluginFactory implements PluginFactory
{
   public function getName()
   {
        return 'authentication';
   }

   public function create($options = [])
   {
        // Checks options with symfony options resolver ?

        // Create plugin
        return new AuthenticationPlugin($options['authentication']);
   }
}

Then the PluginClientFactory can register PluginFactory (and we can register all ours plugins by default, but also provide the register so people can add their own implementation).

Also @dbu i don't want to make the creation static so we can use this factory like a service.

@dbu
Copy link
Contributor

dbu commented Jan 14, 2016

i am not sure we need a factory for instantiating plugins. that feels like too much. also, AuthenticationPlugin::__construct() should validate the options, not a factory.

what could make sense is a PluginClientBuilder that allows to add plugins and can then give a PluginClient. like the symfony ContainerBuilder that is used to define what is needed to create the Container object.

in the context of the HttplugBundle, we currently are building a way to explicitly list plugins needed on services. other bundles could only add plugins using a compiler pass. a PluginClientBuilder could make this more flexible. the builder could use priorities for the plugins being added to handle timing issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants