Plugins #16
Plugins #16
Conversation
| $def->setFactory([new Reference($arguments['factory']), 'createClient']) | ||
| ->addArgument($arguments['config']); | ||
|
|
||
| if (empty($arguments['plugins'])) { |
There was a problem hiding this comment.
Do we want this? An alternative could be to send everything to PluginClientFactory so every client is an instance of PluginClient.
There was a problem hiding this comment.
i would keep it. there is no way to add plugins later at runtime, so it would only be relevant for people that typehint the concrete class instead of the interface, which would be so wrong that we should not care about helping them hide the problem :-)
|
👍 thanks tobias! |
|
Thank you for reviewing this so quickly. I've updated according to your comments. |
Nyholm
force-pushed
the
plugins
branch
2 times, most recently
from
dfbc6f2
to
d02d918
Compare
| <services> | ||
| <service id="httplug.plugin.content_length" class="Http\Client\Plugin\ContentLengthPlugin" public="false" /> | ||
| <service id="httplug.plugin.decoder" class="Http\Client\Plugin\DecoderPlugin" public="false"> | ||
| <argument /> |
There was a problem hiding this comment.
there is an undefined argument here
There was a problem hiding this comment.
Isn't that okey since the decoder has a default value there?
There was a problem hiding this comment.
There was a problem hiding this comment.
We do not have tests that cover this. You are correct that the bundle lacks tests.
|
i agree solving plugin configuration in a separate PR. but right now there are two plugin configs with empty tags, i think that will cause problems, no? if we can omit the params we should, otherwise we should remove those plugins for now. |
|
We can omit those. Ignore my comment in: #16 (comment) |
|
alright, i think this is ready to merge. @Nyholm can you please squash the commits? feel free to merge, things look good for me. |
|
Done. I've squashed my commints |
|
great, thanks a lot! |
|
Thank you for merging. |
Added plugins with the configuration feature discussed in #15.