Skip to content

Add support for nyholm/psr7 #104

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
Feb 6, 2018

Conversation

pyrech
Copy link
Contributor

@pyrech pyrech commented Jan 26, 2018

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

Add support for nyholm/psr7's factories.

@pyrech
Copy link
Contributor Author

pyrech commented Feb 5, 2018

Is this PR wanted? I'm using my fork for now but it would be awesome if this PR could be merged and deployed soon! 😉
Thanks!

@sagikazarmark sagikazarmark requested a review from Nyholm February 5, 2018 22:17
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is great. Thank you

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.

i have a tiny worry here: should the new factories not be added at the bottom of the list? being first, if guzzle and nyholm are installed, the autodiscovery might switch what is found. if people really restricted themselves to PSR7, that will not matter, but maybe they relied somehow on guzzle being detected... is there a specific reason to put it on top? otherwise i'd prefer it at the bottom of the list.

@pyrech
Copy link
Contributor Author

pyrech commented Feb 6, 2018

It was mentionned in the related issue. Nyholm is the lightest and the more complete PSR implementation. IMO it has its place as the recommended implementation to use by default

@Nyholm
Copy link
Member

Nyholm commented Feb 6, 2018

Changing the order is fine according to our bc promise: http://php-http.readthedocs.io/en/latest/httplug/backwards-compatibility.html

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.

agreed!

thanks for the pull request @pyrech - and you are just too modest, @Nyholm, for not having added your own implementation :-D

@dbu dbu merged commit 4d9b3a6 into php-http:master Feb 6, 2018
@dbu
Copy link
Contributor

dbu commented Feb 6, 2018

@pyrech do you try this out in your project quickly, just so we are 100% sure its all fine? then @Nyholm or me can tag a new minor version.

@pyrech
Copy link
Contributor Author

pyrech commented Feb 6, 2018

I'm already using my fork on production but will switch back to this package asap and ping here when done 😉

@pyrech pyrech deleted the feature/support-nyholm branch February 6, 2018 10:49
@pyrech
Copy link
Contributor Author

pyrech commented Feb 6, 2018

Just tested it and it works 👍

@dbu
Copy link
Contributor

dbu commented Feb 6, 2018

thanks. i tagged 1.4.0 with this.

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.

4 participants