Add a new plugin to the PluginClient

[Taluu]

Q	A
Bug?	no
New Feature?	yes
Version	1.7.0

Actual Behavior

Once your PluginClient has been initialized, you can't add new plugins to it

Expected Behavior

Being able to add some new plugins on the fly. I'm currently registering it in a Behat container, so I registered a few plugins on the construct https://github.com/Taluu/Behapi/blob/master/src/Container.php#L85.

But I would like to be able to give the possibility for the users to add some new plugins (such as the auth plugin or a random listener) so that they can do stuff on their own.

Possible Solutions

Add a addPlugin or something to the PluginClient

[Taluu]

I could try to use something like #40 though in order to go around this "problem" (Because I don't really want to rewrite the plugin client just to add a addPlugin method)

[joelwurtz]

This is somehow intended, because adding a plugin can change the behavior of your http client (like the error plugin or even the decode one). So it has a high chance of side effect and WTF errors in many situation.

So yes please use the plugin client builder in order to achieve that.

[joelwurtz]

In fact, we may accept having an addPlugin method if it's immutable (return a new PluginClient instance)

[Taluu]

The problem being that the builder was not merged, the factory was merged instead. And in the factory, you have to specify all the plugins in the factory method.

So it's not possible ATM unless you make your own builder, which I intend to do (Even though I think I shouldn't have to do it, and it should be handled by this repo)

[Taluu]

Having it immutable is a no go too, because I'm injecting this client through a DI, and each services depending on that client may have to add some plugins. So I need to modify the instance, and not have another one.

[joelwurtz]

In fact having both a builder and a factory can make sense as they have different approach, and i totally understand your use case which and it might be useful for other libraries, so you can propose a builder here.

[Taluu]

After another glance at the pluginclient, I don't think having a addPlugin modifying the same object can have side-effects, as in the constructor, the plugin list is left as is. It's only in the sendRequest (and its async counterpart) that the chain is being built, and at this point it is safe to assume that it won't move.

Maybe adding a sort of "freeze flag" on the first sendRequest (or the async one) would do the trick ?

ofc, the builder is the best solution IMO.

[dbu]

we do have the plugin builder, so can we close this issue or is there something left open?

[Taluu]

AFAIC, there is no such builder ? As said earlier, the factory isn't what I was looking for (but the closed builder was, but was not merged...,). I built my own builder, but I'm still convinced it should be done in this repository....

Unless I missed something ?

[dbu]

i was looking at Taluu/Behapi#44 but did not realize that its in a different repository 🤦‍♂️

do you want to do a pull request to add such a builder, then?

[Nyholm]

What is the scenario for adding a new plugin?

You said you use the plugin client as a service in you DIC. Services should be immutable so I dont understand why you ever would like to modify the behavior of a HTTP client in runtime

[Taluu]

The service I'm injecting is the builder actually. I need to do this in my Behat extension so that I can add an auth plugin to the http client. The thing is, there are as many auths (and plugins thus...), and covering them all through configs of sorts would be IMO unefficient.

Thus why I made the plugin client builder. You can see a use-case here : https://gist.github.com/Taluu/ea4ea97391b5dd7fa4d762d536b83e52. I am using another context for the auth part, and I need to inject an auth plugin in this particular context.

[Nyholm]

Okey. I understand. Thank you.

I do something similar here: https://github.com/KnpLabs/php-github-api/blob/2.10.1/lib/Github/Client.php#L314-L331

It seams like both you and I have created solution that work with the current implementation. (Where we are both immutable and final)

What code change do you suggest? And why would that be an improvement?