discovery
discovery copied to clipboard
php-http
Suggestion: require `psr/http-message-implementation` and `psr/http-factory-implementation`
This package operates (at runtime) on the assumption that following packages are available:
psr/http-message-implementationpsr/http-factory-implementation
IMO a good idea to include these in "require".
Yes, that means that downstream implementations need to add a "provide" section on their side, but effectively, that removes one possibly very annoying runtime error that will only be observed very late on.
Ref: https://github.com/sensiolabs/SensioFrameworkExtraBundle/issues/620#issuecomment-805140215
do we hard fail when one of these are not around? or do we only not find anything if they are missing?
i notice that we don't even require psr/http-client-implementation. @Nyholm wdyt about the topic? having the discovery without any candidates makes no sense. did we not have them in order to support the puli scenario? or are there other reasons to not have them?
or should we only fix the code to not assume that these implementations always exist, and not find with an explanation if we don't find any?
If we require those implementations. We would never throw an exception saying "hey, nothing is installed". The user would get an exception when running composer update. For composer 1 (at least), that error message was very cryptic.
We much rather throw our exception and show how to fix the problem. We can now let the consumer of php-http/discovery decide if their users should get an exception in runtime or at composer update.
If runtime failure is avoidable, then why not avoid it?
On Fri, Mar 26, 2021, 08:30 Tobias Nyholm @.***> wrote:
If we require those implementations. We would never throw an exception saying "hey, nothing is installed". The user would get an exception when running composer update. For composer 1 (at least), that error message was very cryptic.
We much rather throw our exception and show how to fix the problem. We can now let the consumer of php-http/discovery decide if their users should get an exception in runtime or at composer update.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/php-http/discovery/issues/197#issuecomment-808003272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEAIGKL5LZQHXEFYCK3TFQZYTANCNFSM4ZVYH75A .
We are not sure if php-http/discovery is used to only find a PSR-7 implementation. If so, we wrongly force the user to also install a PSR-18 implementation. Same thing the other way around (though less likely).
Im not saying that you are wrong. Im saying that it is not the responsibility of this package.
Eh, this package is really only here to fetch psr-7 and psr-18 impl: it is the only thing it's for.
A psr-18 impl will come with a psr-7 impl.
Overall, deferring to the runtime seems more like asking for trouble, while I don't see any actual damage done by adding a requirement, just better DX.
On Fri, Mar 26, 2021, 08:34 Tobias Nyholm @.***> wrote:
We are not sure if php-http/discovery is used to only find a PSR-7 implementation. If so, we wrongly force the user to also install a PSR-18 implementation. Same thing the other way around (though less likely).
Im not saying that you are wrong. Im saying that it is not the responsibility of this package.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/php-http/discovery/issues/197#issuecomment-808005714, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEDIKQDIS2KN7PBL7V3TFQ2JZANCNFSM4ZVYH75A .
Would this work with a PSR-7 and PSR-18 implementation provided by the application itself? (ie. provides section added to the root package)
Probably not: the discovery needs to still be registered with this package, after all.
On Fri, Mar 26, 2021, 11:12 Márk Sági-Kazár @.***> wrote:
Would this work with a PSR-7 and PSR-18 implementation provided by the application itself? (ie. provides section added to the root package)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/php-http/discovery/issues/197#issuecomment-808093728, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEHKP3OBFM2SBN6MSHLTFRMXFANCNFSM4ZVYH75A .
But we also support discovering httplug clients for the async support. I dont think it is a good idea to also require php-http/client-implementation. For the same reasons I was trying to make in https://github.com/php-http/discovery/issues/197#issuecomment-808005714
Probably not: the discovery needs to still be registered with this package, after all.
The discover can be registered by the application itself, I was curious whether the application (root package) can provide a virtual package and whether the discover package can accept it or the providing package also has to be a dependency of the root package.
I don't see why it wouldn't work, but that's a use case I've met before, so it should be taken into account when deciding this.
I see the pro/contra arguments, I agree it would be useful to catch this at "build" time, but it has al sorts of problems (pointed out by Tobias). Maybe we could create a meta package that requires this one and the virtual packages. We could use Tobias' famous subtree split service to automate releases.
I was curious whether the application (root package) can provide a virtual package and whether the discover package can accept it or the providing package also has to be a dependency of the root package.
Yes, you can declare that your package "provides" the meta/virtual package.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
On Fri, Mar 26, 2021 at 11:49 AM Márk Sági-Kazár @.***> wrote:
Probably not: the discovery needs to still be registered with this package, after all.
The discover can be registered by the application itself, I was curious whether the application (root package) can provide a virtual package and whether the discover package can accept it or the providing package also has to be a dependency of the root package.
I don't see why it wouldn't work, but that's a use case I've met before, so it should be taken into account when deciding this.
I see the pro/contra arguments, I agree it would be useful to catch this at "build" time, but it has al sorts of problems (pointed out by Tobias). Maybe we could create a meta package that requires this one and the virtual packages. We could use Tobias' famous subtree split service to automate releases.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/php-http/discovery/issues/197#issuecomment-808115784, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVECAGUMBLSD7OUELUBLTFRRFLANCNFSM4ZVYH75A .