Skip to content

Add Guzzle7 Adapter Client Discovery #189

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 4 commits into from
Sep 22, 2020
Merged

Add Guzzle7 Adapter Client Discovery #189

merged 4 commits into from
Sep 22, 2020

Conversation

barryvdh
Copy link
Contributor

@barryvdh barryvdh commented Sep 22, 2020

Related:

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes php-http/guzzle6-adapter#72
Documentation Adds support for https://github.com/php-http/guzzle7-adapter
License MIT

What's in this PR?

Adds support for https://github.com/php-http/guzzle7-adapter

Why?

To provide the HttpClient for Guzzle7

Checklist

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

@dbu
Copy link
Contributor

dbu commented Sep 22, 2020

thanks!

can you please also add an integration test for the adapter so we can ensure it keeps working, very similar to

- install_test will-find "Http\Discovery\HttpClientDiscovery::find();" "php-http/guzzle6-adapter:^2.0.1"

@dbu
Copy link
Contributor

dbu commented Sep 22, 2020

oh, and no need to worry about the phpspec 8 failure in the test suite - that is unrelated to what you are doing.

@barryvdh
Copy link
Contributor Author

Seems to be passing :) (Used 0.1 though, because there is no 1.x yet)

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.

looks good, thanks!

@Nyholm fine like this or do you notice any issues?

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.

Yes, it looks good.

Thank you

@dbu dbu merged commit 3d7f833 into php-http:master Sep 22, 2020
@dbu
Copy link
Contributor

dbu commented Sep 22, 2020

@GrahamCampbell
Copy link
Contributor

I would actually be 👎 against the creation of that Guzzle 7 adapter. People should be using the PSR interfaces these days, which Guzzle 7 already provides. No need to create a Guzzle 8 adapter, certainly, when Guzzle 8 is released.

@sagikazarmark
Copy link
Member

@GrahamCampbell The Guzzle 7 adapter implements the async version of HTTPlug, so for those in need of HTTPlug async, this is necessary.

For those who already use PSR-18 only, Guzzle 7 will be discovered directly.

@dbu
Copy link
Contributor

dbu commented Sep 23, 2020

another valid use case is using guzzle 7 with older libraries that asks for httplug instead of psr-18.

but i agree that new projects that do synchronous requests should use psr-18 and not httplug these days.

@GrahamCampbell
Copy link
Contributor

another valid use case is using guzzle 7 with older libraries that asks for httplug instead of psr-18.

I was rather hoping by not providing compat for that we'd encourage them to upgrade. Many maintainers aren't even aware that they could/should do that.

@dbu
Copy link
Contributor

dbu commented Sep 23, 2020

until there is a psr for async requests (which can't happen before there is a psr for promises), httplug can't be faded out anyways, so i think the cost of not forcing people to adapt is not that high. compared to the cost of people being confused why they can't find a http client when they have guzzle and the adapter installed. if they ask for a psr-18 adapter, they will get guzzle directly and not our adapter, so that is fine.

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.

5 participants