Skip to content

Support Guzzle 7+ #175

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 10 commits into from
Jul 2, 2020
Merged

Support Guzzle 7+ #175

merged 10 commits into from
Jul 2, 2020

Conversation

GrahamCampbell
Copy link
Contributor

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

What's in this PR?

Adds support for Guzzle 7 and higher.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

  • If the PR is not complete but you want to discuss the approach, list what remains to be done here

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 28, 2020

PHPUnit tests seem to be failing. I suspect this to be an issue with the tests, rather than the code!

@Nyholm
Copy link
Member

Nyholm commented Jun 28, 2020

Why not improve the condition to make sure the correct version of Guzzle is used?

acrobat added a commit to KnpLabs/php-github-api that referenced this pull request Jun 28, 2020
…inimum version to ^2.0 (GrahamCampbell)

This PR was merged into the 2.15.x-dev branch.

Discussion
----------

`php-http/client-implementation` is deprecated and Guzzle 7 doesn't implement it, so this will be a blocker for anyone wanting to upgrade from Guzzle 6 to 7 and use this package at the same time.

---

Related: GitLabPHP/Client#511, BitbucketPHP/Client#39, php-http/discovery#175.

Commits
-------

ec31be1 Switch to PSR18 client impl
@GrahamCampbell
Copy link
Contributor Author

Ready for re-review now. I've also expanded the "install" tests, so we have some proof that we can detect Guzzle 6 and 7 as PSR-18 clients, and that this won't break in the future.

@GrahamCampbell GrahamCampbell requested a review from dbu July 1, 2020 23:05
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, yes this is much clearer imho!

and good that you added the tests so we can see whether it actually works 👍

@dbu dbu merged commit fef8475 into php-http:master Jul 2, 2020
// Guzzle 6 does not implement the PSR-18 client interface, but Guzzle 7 does.
foreach (self::$classes[Psr18Client::class] ?? [] as $c) {
if (GuzzleHttp::class === $c['class']) {
if (defined('GuzzleHttp\ClientInterface::MAJOR_VERSION')) {
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we need this.

This logic should be on the condition

@Nyholm
Copy link
Member

Nyholm commented Jul 2, 2020

Fixed in #177

@GrahamCampbell GrahamCampbell deleted the patch-1 branch July 2, 2020 09:31
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.

Guzzle 7 Not Found
4 participants