Add plugin client builder #158

Taluu · 2019-01-06T21:41:30Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #96
Documentation	None, for now ?
License	MIT

What's in this PR?

This adds a plugin client builder, that allows to dynamically build a plugin client with plugins set on the go.

Why?

If you have some conditions on your plugin client (such as the auth can vary, some more headers can be dynamically added, ... like in the github's api from knp or my behat extension for http calls), it can be useful to not have a static plugin client with its plugin list, but to be able to add / remove them when needed.

As mentionned in the two repositories ([https://github.com/KnpLabs/php-github-api/blob/2.10.1/lib/Github/Client.php#L314-L331](Github's Client for the github php api of knp, or my behat extension), we both had to implement our own plugin builder, so this shows there's a "demand" for that.

We could have used the PluginClientFactory, but the problem with it is that when creating the client, we need to provide all the plugins on the go, which is not (IMO) practical.

Example Usage

use Http\Client\Common\PluginClientBuilder;

// $client is a php-http client or a psr-18 one
// $plugin is a php-http plugin, pick one (auth basic, auth oauth, ... anything goes)

$builder = new PluginClientBuilder();
$builder->addPlugin($plugin); // by default, priority is 0

// do stuff....

$pluginClient = $builder->createClient($client);
// plugin client is a PluginClient with the `$plugin` plugin

// ...

$pluginClient = $otherBuilder->createClient($client);
// plugin client is another PluginClient and not the same as the one built previously

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

To Do

Discuss the mutability ? decision : remove it.
How should we handle options in the builder ? In the create client ? In the constructor ? If in the create client, if changed should it retrigger the client creation ? decision : Add setter and remover for options, and inject that into the plugin client.
Add priorities in the plugins
Remove naming plugins
Tests ? As I didn't see many of those, and I wanted to throw a poc for a builder first

sagikazarmark

Thanks a lot for the PR. I had a few comments above.

src/PluginClientBuilder.php

Taluu · 2019-01-07T15:48:47Z

Updated with remarks from @sagikazarmark. I added the client in the constructor rather than the createClient to have a better "immutability" handling.

But with that, the options given in the createClient may pose problem if it changes... So we could either pass in into the constructor too, or find some sort of invalidating the built client.

Or we could add a setter for that one, as having options is, as its name implies it, optionnal.

I'm also not really convinced we should add a setter for the client though, I think the constructor should do the trick. Maybe adding a withClient($client) to get a new builder with the selected client ?

sagikazarmark · 2019-01-07T20:22:57Z

Just to be clear: I don't think in this case immutability is a good idea. It my not even be practical. For example: the builder could be called by various parts of a DIC. If it's immutable, essentially every call to it would be ineffective. So I think it's okay to have a mutable state in the builder.

Also, what's the point of caching the client? Again, the builder might be called by a DIC in a terminal stage of the dep resolution process and then never used again. And as I said, the natural thing to expect from a function called createClient, accepting a client instance is to return one with my client inside, not an earlier cached one. I think it would be more confusing than useful. Creating multiple clients with different base clients can be a perfectly valid use case. So my second comment means that I think caching the client is not necessary, may even be harmful in some cases.

gmponos · 2019-01-07T20:27:35Z

src/PluginClientBuilder.php

+     */
+    public function __construct($client)
+    {
+        if (!$client instanceof ClientInterface && !$client instanceof HttpClient && !$client instanceof HttpAsyncClient) {


If this targeting master (v2) is there a reason why to keep the check for both ClientInterface and HttpClient ?

If I understood @Nyholm, this is not specially for 2.0 (at least not blocking for it). I didn't check if there was a 1.x branch.

I'm not sure either if we should check for the ClientInterface though...

Master branch is v2

Yes there is a branch for v1...

Master branch which is v2 has as min requirement httplug: ^2.0 . The client on this version extends ClientInterface

I'm not sure either if we should check for the ClientInterface though...

Therefore my suggestion was to check only ClientInterface...

:)

Checking all three would make more sense for v1.

I'll retarget if there's hope for a 1.x release then. @Nyholm @sagikazarmark ? If not, I'll remove the HttpClient, as the async client is not in the psr-18.

why aren't you sure about that ?

ok sorry I haven't checked if plugin client accept an Async one.. it does..

for easier upgrade paths, version 2 accepts both PSR-18 clients and the old Httplug client interface. and the async client which only exists in Httplug.

So my suggestion is not accurate? What am I missing? check this

I removed the HttpClient, even though I'm also not sure due to @dbu's comment... I saw in the PR that introduced http-plug 2.0 (psr-18 compatible) that removing the compatibility for 1.0 was on purpose because it involved some magic.

So I should either target 1.x and thus support also the HttpClient, or leave it as is and remove it in here. But, AFAIC, new features should only target 2.0 (master), so....

sagikazarmark · 2019-01-08T06:31:29Z

@Taluu I'm not against supporting this in 1.x as well per se, but 2.x is the new target of every feature now.

dbu

cool, thanks for the pull request!

i agree with mark that the builder should be mutable.

the plugin order is important. right now, we have addPlugin that appends at the end. what if we had prependPlugin and appendPlugin instead, to also add to the front?

we could do insertAfter and insertBefore additionally, but then the things using the builder would need to know about each other. or we could use a priorities - but that would collide with the name concept.
do we really need a remove method and implicit overwrite by setting the same name again? without remove, there would be no need for the name.

hm, another thing with overwriting: addPlugin adds at the end, except when you overwrite an existing name. if the name is already in the list, the overwritten plugin goes there.

src/PluginClientBuilder.php

Taluu · 2019-01-08T08:59:18Z

I'll remove the immutability and the cache then. But for the plugin removal, it can be useful when we want to e.g remove a previous auth mechanism and replace it with another one. Unless the fact that a plugin in the bottom will somehow overwrite the previous one, I didn't check that case.

Adding appendPlugin and prependPlugin can be useful, I agree, but for insertAfter and insertBefore, I have some doubts, because as you said we would need to somehow know the plugins within the builder...

I'd rather add priorities actually. I'll try to work on that.

sagikazarmark · 2019-01-08T10:00:27Z

Generally a builder serves one purpose. If you want for example create a base builder and use it later as the base for several other builders, you could add a cloneBuilder or new method that returns a new builder which can be modified without modifying a parent.

Priorities sound like and interesting feature. It can especially be useful when the building does not happen at one place (eg. the aformentioned DIC example). IIRC symfony event dispatcher has some sort of priority support.

Someone working on the HTTPlugBundle could jump in here as I imagine the bundle an similar solutions are first class citizens of this builder.

Taluu · 2019-01-08T10:38:16Z

PR updated. I removed the immutability, and added priorities for plugins instead of names. I had to remove the possibility to remove a plugin, as we now have no means to remove a specific plugin (as there's no name given).

I chose to keep the fluent interface in the builder, as this is one of the few cases where it is legit. I'll update the first message with the examples.

dbu

great! i think with priorities its easier than with defined order and prepend/append insertBefore/insertAfter 👍

and i agree on the fluent interface for a builder.

i have some suggestions for documentation and commented on the options.

if the others agree with this design, we should have some tests to make sure priorities work as expected. and an entry in the documentation.

src/PluginClientBuilder.php

joelwurtz

Love it!

It just miss a test (like testing priorities)

GrahamCampbell · 2019-11-17T15:20:39Z

What's the status of this PR?

Taluu · 2019-11-17T15:27:56Z

IIRC, adding tests. I totally forgot about that one. 😂

Taluu · 2019-11-23T12:00:28Z

Test added on the plugins priority check and the fact that the options should be passed onto the plugin client properly

johnss · 2019-12-12T17:55:09Z

Builder in KnpLabs Github API allow remove plugin https://github.com/KnpLabs/php-github-api/blob/master/lib/Github/HttpClient/Builder.php
is there any strong reason to exclude remove plugin capability?

dbu

looks good to me. i think we can merge as is.

for the question of removing plugins: in general i am wary of such functionality, though the builder seems like the one place where it might be ok.

please help to understand the use cases: when would you need this? and how would you go about removing them? pass in the instance that should be removed? or remove by class with instanceof checks?

Taluu · 2019-12-13T14:54:28Z

I'm also not in favor of "removing" stuff in a first pass. We could still consider it afterwards though.

dbu · 2019-12-16T10:01:17Z

thanks for the contribution!

Taluu force-pushed the builder branch 4 times, most recently from 3edf370 to 3358fa6 Compare January 6, 2019 21:47

sagikazarmark requested changes Jan 7, 2019

View reviewed changes

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

Taluu force-pushed the builder branch from 3358fa6 to aad1c89 Compare January 7, 2019 15:45

gmponos reviewed Jan 7, 2019

View reviewed changes

dbu reviewed Jan 8, 2019

View reviewed changes

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

Taluu force-pushed the builder branch from aad1c89 to be88c95 Compare January 8, 2019 10:36

Taluu force-pushed the builder branch from be88c95 to 0a563ca Compare January 8, 2019 11:00

dbu approved these changes Jan 8, 2019

View reviewed changes

src/PluginClientBuilder.php Outdated Show resolved Hide resolved

src/PluginClientBuilder.php Show resolved Hide resolved

Taluu force-pushed the builder branch 3 times, most recently from f9771be to 021b245 Compare January 10, 2019 13:43

Taluu commented Jan 10, 2019

View reviewed changes

src/PluginClientBuilder.php Show resolved Hide resolved

dbu mentioned this pull request Jan 20, 2019

How to disable a plugin on a specific client in a specific environment ? php-http/HttplugBundle#306

Closed

joelwurtz requested changes Jan 21, 2019

View reviewed changes

dbu mentioned this pull request Nov 18, 2019

Add a new plugin to the PluginClient #96

Closed

Add plugin client builder

1915f71

Taluu force-pushed the builder branch from 021b245 to 22f0e91 Compare November 23, 2019 11:52

Taluu added 2 commits November 23, 2019 12:57

Add a test on the plugin client builder

0c0a37d

Add test on passing options from the client builder to the client

cbb22ef

Taluu force-pushed the builder branch from 22f0e91 to cbb22ef Compare November 23, 2019 11:59

Taluu requested review from sagikazarmark and joelwurtz November 23, 2019 12:00

dbu approved these changes Dec 13, 2019

View reviewed changes

dbu merged commit 0e6a80e into php-http:master Dec 16, 2019

Taluu deleted the builder branch February 4, 2020 09:52

Taluu mentioned this pull request Jan 27, 2021

Use PHP-HTTP's PluginClientBuilder Taluu/Behapi#69

Merged

Add plugin client builder #158

Add plugin client builder #158

Uh oh!

Conversation

Taluu commented Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

Example Usage

Checklist

To Do

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Taluu commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagikazarmark commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmponos Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jan 8, 2019

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Taluu commented Jan 8, 2019

Uh oh!

sagikazarmark commented Jan 8, 2019

Uh oh!

Taluu commented Jan 8, 2019

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelwurtz left a comment

Choose a reason for hiding this comment

Uh oh!

GrahamCampbell commented Nov 17, 2019

Uh oh!

Taluu commented Nov 17, 2019

Uh oh!

Taluu commented Nov 23, 2019

Uh oh!

johnss commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu left a comment

Taluu commented Jan 6, 2019 •

edited

Loading

Taluu commented Jan 7, 2019 •

edited

Loading

Taluu Jan 7, 2019 •

edited

Loading

gmponos Jan 8, 2019 •

edited

Loading

johnss commented Dec 12, 2019 •

edited

Loading