Skip to content

Support discovering PSR-17 factories from guzzlehttp/psr7 package #167

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
May 18, 2020
Merged

Support discovering PSR-17 factories from guzzlehttp/psr7 package #167

merged 1 commit into from
May 18, 2020

Conversation

ste93cry
Copy link
Contributor

@ste93cry ste93cry commented Mar 6, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? it depends on the outcome of the discussion
Deprecations? no
Related tickets fixes #164
License MIT

What's in this PR?

In the next version of guzzlehttp/psr7 there will be support for PSR-17 (actually the feature has not been released yet), so I simply added support for discovering the factories.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

To Do

For the moment I added the new factories in the middle of the list instead of at the end. What this means is that suddenly the priorities of discovery will change and the guzzle/psr7 package will be preferred over the php-http/guzzle6-adapter, which may be considered a BC. Just let me know if I have to move the classes to the end of the list instead

Copy link
Contributor

@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.

thanks a lot for your explanations in #164
i agree we should add detection for the factory here. as for the ordering, i am not fully sure. while there should be no effect from getting a different factory class, people might accidentally are typehinting an implementation instead of the interface. while breaking that would not be ideal, its also so wrong that i would consider it a bug of the user... discovery does not promise to return a specific implementation (and if you know what you want, just implement it yourself).

on the other hand, if a user plans to use the guzzle factory, why would they install the adapter? though they might not be aware that guzzle psr7 provides factories now, and still install the adapter, or don't remove it when updating.

@Nyholm do you have a preference for the ordering or ok if we merge as is, hiding the php-http adapter if a new guzzle psr7 version is installed?

@ste93cry
Copy link
Contributor Author

ste93cry commented May 8, 2020

@Nyholm @dbu is there anything left to do or can this be merged?

@dbu dbu merged commit 7e30d5c into php-http:master May 18, 2020
@ste93cry ste93cry deleted the feature/add-support-for-guzzle-psr17-discovery branch May 18, 2020 07:43
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.

"No PSR-17 request factory found" incorrectly suggests guzzlehttp/psr7
2 participants