Skip to content

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

Merged
merged 1 commit into from
Dec 27, 2019
Merged

Allow to register PSR17 and PSR18 classes as services #372

merged 1 commit into from
Dec 27, 2019

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Dec 26, 2019

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
Documentation
License MIT

We need a way to support PSR17 factories. Our CurlClient 2.0 does not use the deprecated php-http/message-factory.

Copy link
Collaborator

@dbu dbu left a 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"/>
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

@dbu dbu left a 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!

@dbu dbu merged commit 060b9b1 into master Dec 27, 2019
@dbu dbu deleted the psr17 branch December 27, 2019 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants