prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language
https://prismic.io
Other
109 stars 83 forks source link

Support GuzzleHTTP 7.x #184

Closed pmzandbergen closed 1 year ago

pmzandbergen commented 2 years ago

https://github.com/guzzle/guzzle/blob/7.0.0/UPGRADING.md#60-to-70

GuzzleHTTP 7.x seems to work flawlessly 👍 .

Please add a tag after checking / merging.

sanderjongsma commented 2 years ago

Prismic please merge this.

c0nst4ntin commented 2 years ago

@gsteel When I tested this locally, it all continued to work. After I ran a composer update the tests failed.

There appear to be several calls to outdated classes and functions, unlike @pmzandbergen suggested (see: https://github.com/guzzle/guzzle/blob/7.0.0/UPGRADING.md#60-to-70)

It would solve Issue #170 which many people have requested.

Whilst PR #171 seems to have done a more thorough job at fixing this, it also locked PHP 7.4 and updated PHP Unit which caused some conflicts.

How do you think we should proceed with this matter? I could try and extend this PR to the extent where it combines changes from #184 and #171 so we could merge this.

Would this be backwards compatible, though? Wold merging this PR without composer update solve the incompatibility changes whilst being backwards compatible?

gsteel commented 2 years ago

How do you think we should proceed with this matter?

I would personally do nothing until a patch is merged with decent CI. Ideally, you should be running ci with locked, latest and lowest deps on the minimum supported version of PHP and lowest and latest on all the others.

I'd encourage you to check out https://github.com/laminas/laminas-ci-matrix-action on GHA to get all that going.

I had a skim of #171 but like me, the author has gone with his own fork so you'd have to ask if he has the will to rebase and refine, or use his work as the basis for a new patch. (I'd get PHPUnit upgraded first, and separately)

By the looks of your own research, blindly merging this patch as an untested version bump is not likely to go well for consumers of this lib.

Would this be backwards compatible, though?

Currently, this lib supports ^6. If, during testing, you cannot support both 6 and 7, then you can drop 6 in a minor, introducing support for 7 at the same time as long as you don't change the public api of the lib, which is perfectly possible. I don't tend to use Guzzle much, but there's no reason why you can't just drop 6 in the next minor as a matter of course. Does Guzzle 6 even support PHP 8.0?

pTinosq commented 1 year ago

Sorry for bringing this up on a weekend.

Any chance we can get this merged soon? When working with Laravel, looks like you need GHTTP ^7.5

Here is the full output when running composer require prismic/php-sdk

C:\[removed]>composer require prismic/php-sdk
Using version ^5.2 for prismic/php-sdk
./composer.json has been updated
Running composer update prismic/php-sdk
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires prismic/php-sdk ^5.2 -> satisfiable by prismic/php-sdk[5.2.0, 5.2.1].
    - prismic/php-sdk[5.2.0, ..., 5.2.1] require guzzlehttp/guzzle ^6.3 -> found guzzlehttp/guzzle[6.3.0, ..., 6.5.x-dev] but the package is fixed to 7.5.0 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.
You can also try re-running composer require with an explicit version constraint, e.g. "composer require prismic/php-sdk:*" to figure out if any version is installable, or "composer require prismic/php-sdk:^2.1" if you know which you need.
c0nst4ntin commented 1 year ago

Sorry for bringing this up on a weekend.

Any chance we can get this merged soon? When working with Laravel, looks like you need GHTTP ^7.5

Here is the full output when running composer require prismic/php-sdk

C:\[removed]>composer require prismic/php-sdk
Using version ^5.2 for prismic/php-sdk
./composer.json has been updated
Running composer update prismic/php-sdk
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires prismic/php-sdk ^5.2 -> satisfiable by prismic/php-sdk[5.2.0, 5.2.1].
    - prismic/php-sdk[5.2.0, ..., 5.2.1] require guzzlehttp/guzzle ^6.3 -> found guzzlehttp/guzzle[6.3.0, ..., 6.5.x-dev] but the package is fixed to 7.5.0 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.
You can also try re-running composer require with an explicit version constraint, e.g. "composer require prismic/php-sdk:*" to figure out if any version is installable, or "composer require prismic/php-sdk:^2.1" if you know which you need.

I have a patch working locally, which should be backwards compatible. I will create the PR tomorrow and try to get it merged ASAP.

I hope this is fine for you. I'll make sure to tag you in the release

pTinosq commented 1 year ago

Amazing stuff. Thank you very much.

c0nst4ntin commented 1 year ago

This got solved by #199 Please upgrade to Version 5.3.0 to make use of this!

If you have any troubles, please leave a comment or open a new issue!