Add Plugin Client #14
Conversation
| @@ -13,7 +13,8 @@ | |||
| "require": { | |||
| "php": ">=5.4", | |||
| "php-http/httplug": "^1.0", | |||
| "php-http/message-factory": "^1.0" | |||
| "php-http/message-factory": "^1.0", | |||
| "symfony/options-resolver": "^2.6|^3.0" | |||
There was a problem hiding this comment.
This is one of my concerns. Adding this dependency here. Maybe we should rethink option handling in the plugin client?
There was a problem hiding this comment.
the big benefit of options-resolver is that we are much more flexible to add things without a BC break. adding new constructor parameters can easily lead to BC break, and for these options things, its tricky to decide on the order of them - with separate parameters, the order can not be changed to make logical sense. the array is so much easier.
hm. on the other hand we have only one option for now. maybe we could instead do that manually instead of using OptionsResolver - we can add a requirement for the resolver when we add more options. for just one, its indeed overkill if its not a free include. wdyt?
- remove the resolver dependency
- keep $options an array
- resolve manually for now
- add a comment next to the options that if we add more options, we should introduce the OptionsResolver
There was a problem hiding this comment.
This is exactly what I had in my mind.
|
FYI @joelwurtz |
| * | ||
| * @author Joel Wurtz <joel.wurtz@gmail.com> | ||
| * | ||
| * TODO: make class final once removed from plugins. |
There was a problem hiding this comment.
"make class final in version 2.0, once plugins does not extend it anymore" ?
|
Sorry for the delay, under high load actually :) In fact in my mind i would also move all plugins implementations since the dependency is very strong, it does not make sense IMO to have them alone in there repository, this would add a dependency on php-http/message however, |
|
The reason I didn't move all plugins here is that it would add too much dependency and code here. I think having plugins separated is not bad. That's our core collection of plugins. You might want to use the plugin client, even without our plugins. |
|
Although I see the point in lowering the number of repositories, I feel that plugins don't really fit here. @php-http/owners any other input regarding this? It's a 1-1 ATM. |
Yes there is this use case but it's only for the 1% IMO and we should not bother about that, so we should either move all code from plugins or nothing, but not something between the two. |
|
Well, I could even live with plugins that does not have any external dependencies. Adding any external dependencies here would make the package a monolith. |
|
Hm, wouldn't we add a dependency on message with #13? Can we make it an optional dependency? |
|
i think having the clients depend on message seems really not that wrong. i would prefer to keep everything plugin in php-http/plugin, but if we want to move it anyways we can just as well move the plugins that have no extra dependencies here, and move each plugin with special dependencies to its own repository with properly declared dependencies. |
Yeah, I could live with that. WDYT @joelwurtz? IMO we had a discussion about doing this (extracting plugins with external dependencies). One possible thing we could do to avoid pluginception is that having message related, message dependent plugins in a message-plugins package. Log and cache could go into their own. |
Agreed! |
|
message related, message dependent plugins
in a message-plugins package. Log and cache could go into their own.
i don't like that. rather have one repo per plugin, otherwise it gets
really hard for people to figure out which plugins exist.
but maybe the concrete example is moot if we need message anyways in
client-common and can just put those plugins in here.
|
Indeed. The router client is going to need it anyway. |
In flysystem you get local and FTP adapters out of the box (no dependencies) and it suggests (in composer.json) all of the available adapters |
|
Well, I think @dbu meant that we can do that, but plugins should either be completely separated or be together. However, it seems that message is going to be a dependency of this package. So plugins without dependencies and plugins depending on message could come here as well. Pros:
Cons:
|
|
exactly, things that add no additional dependencies in this repo, everything else in a clearly named repo. afaik flysystem does the same, grouping separate plugins together makes it really confusing, its an implementors logic rather than consumer logic, and we do these things for the consumers. |
|
@dbu How can we proceed in the HttplugBundle if get this sorted out? Can we just drop the plugins repo dependency and use the new packages? Isn't it a BC break? Is there such thing regarding dependencies at all? |
|
hm. it certainly is not a patch version change to drop a dependency. but for a minor version i would be ok - it seems a bit of a grey area. but after a minor version upgrade, you should check your stuff and notice you miss the plugin. we can ease things by mentioning which plugins now need to be explicitly required if used in the changelog file. |
|
Well if we have the php-http/message dependency we can move all plugins since other dependencies are only in suggest on the php-http/plugins package, no ? If we look at the suggestion also (about log and cache for exemple) this is a pretty common dependency to have (at least for log ATM, and cache should follow fast looking at how it is adopted), so the suggestion will be available only for a few people. The only plugin we can certainly move out is the Stopwatch one (and make it a dependency on the bundle or directly add him as very few people use stopwatch withtout the symfony framework) |
|
I would say drop suggestions and move them to separate packages. |
|
IMO we should be more focus on the most common developer experience rather than being perfect about package separation, and even if its better to have this separated in "architecture design", i think most people wants to have the LoggerPlugin out of the box rather to have to require another package. |
|
i think having plugins with external dependencies in their own package is simpler to understand. its easier to say: "if you want logging: require php-http/plugin-logger" than to say: "if you want logging, there is a plugin already but you need to require psr/log". |
|
In that case, I would also go with #18 immediately. |
|
well, for the things we add here, yes!
the clients that already exist here can't be made final of course.
|
Yep, I will add todos for them. |
|
Okay, what's left is to decide whether we want cache and log plugins in this repo or not. Pros:
Cons:
I still think that we should avoid optional dependencies at all cost, because it adds a considerable maintenance overhead (which version compatible with which optional dependency). Also, I don't think that requiring to install a log-plugin package is any worse than requiring to install a log implementation. On the other hand having everything at one place makes it easier for users to use the package (although we can document the fact of the installation). If we can't make an agreement on this topic, I propose we have a vote about it (owners with "real" vote, community votes as feedback) |
|
Also let's set a deadline: this week. |
|
I’m in favour of having separate cache and log plugin packages. For developer friendliness, we could apply Symfony’s trick, where we add one |
|
Sorry for not replying before.
Am I missing something? About cache and log plugin:I think (based on intuition only) that plugins that require no dependencies but Symfony OptionResolver and PSR7 should be in client-common. Other plugins should live in their own repository. This will address the issue php-http/plugins#61 About the namespaces:The current PRs suggest that we have plugins in |
I think we can use |
|
i am +1 on having the plugins with extra dependencies in separate repositories so we can properly specify the requirements rather than have "optional dependencies". i am -1 on the |
|
Re: Namespaces: why not keep the naming as we have it in this repo and make them Http\Client\Common\Plugin? otherwise we have two different autoload roots in one package, which would be quite confusing. if we really want to avoid Common, we better move everything else to a new folder Common and change the PSR-4 root namespace accordingly. but i would prefer to put the Common in the namespace |
Exactly my concern regarding the optional dependencies as well. I don't have any arguments against using Common. |
sagikazarmark
force-pushed
the
plugin_client
branch
3 times, most recently
from
ca9ca1c
to
4fe307b
Compare
|
@dbu what would happen with logging in HttplugBundle if we extract the logger plugin to a separate package? Right now logging is enabled by default, which lets us choosing two paths:
Planning to sort this out in the next few days and I am looking at other scenarios to finally decide if it could cause any problems to extract cache, log and stopwatch to separate packages. |
|
in the cmf, we handled this sort of "optional" dependency by saying logging defaults to |
|
Documentation added, squashed, remaining plugins separated from the main repo, ready for a final review. About single repo: for v2 version of the plugins repo we could have it as a meta package requiring all major plugins. However, we have to be careful with versioning. |
| @@ -6,6 +6,7 @@ | |||
|
|
|||
| - Add a flexible http client providing both contract, and only emulating what's necessary | |||
| - HTTP Client Router: route requests to underlying clients | |||
| - Plugin client and implementations moved here from `php-http/plugins` | |||
There was a problem hiding this comment.
what implementations? (i assume you mean plugin implementations, but that is not clear)
should we say "Plugin client and simple plugins moved here from..."?
There was a problem hiding this comment.
Good catch. We refer to these as core plugins in other places.
There was a problem hiding this comment.
|
are the separate repos fully ready? then i would say go ahead and merge! thanks for the effort! |
sagikazarmark
force-pushed
the
plugin_client
branch
from
4fe307b
to
9c1c93b
Compare
|
From my POV they are ready. Update composer dev alias, changelog and tag necessary. |
Update todo Remove options resolver dependency Revert: Remove options resolver dependency Add AddHostPlugin Add Authentication plugins Add content length plugin Add final warning to plugins Add cookie plugin Add decoder plugin Add error plugin Add header plugins Add history plugin Fix namespace import order Add redirect plugin Add retry plugin Make plugin classes final, related #18 Fix throw keyword Manually apply php-http/plugins#68 Manually apply and close php-http/plugins#65
What's in this PR?
Moves Plugin client and plugins from the plugins repo
Checklist