-
Notifications
You must be signed in to change notification settings - Fork 50
Allow to register PSR17 and PSR18 classes as services #372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am a bit worried of regressions for users that don't have a psr17 implementation installed. did i mis-think this or could this explode for some people?
<argument type="service" id="httplug.message_factory"/> | ||
<argument type="service" id="httplug.stream_factory"/> | ||
<argument type="service" id="httplug.psr17_response_factory"/> | ||
<argument type="service" id="httplug.psr17_stream_factory"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this lead to regressions for users that have no psr17 implementation available?
the factories throw exception when no implementation is present. should we have a compiler pass that checks if the factories find anything and removes the services if they don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is very true. I have to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested in Symfony 5.
It is not true, we are only using the factory if we are actually trying to use the service. So this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but are we only trying to use the service when a psr-17 implementation is available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ie, the curl client is using psr17. It is currently broken.
So if you use the httplug.factory.curl, then we will use these psr17 factory services.
There is nothing that is using these services out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, got it now. thanks!
We need a way to support PSR17 factories. Our CurlClient 2.0 does not use the deprecated php-http/message-factory.