Skip to content
This repository has been archived by the owner on Jan 6, 2024. It is now read-only.

Another case for allowing PHP 8 in composer.json #83

Closed
jorissteyn opened this issue Jan 31, 2021 · 4 comments
Closed

Another case for allowing PHP 8 in composer.json #83

jorissteyn opened this issue Jan 31, 2021 · 4 comments

Comments

@jorissteyn
Copy link

Considering the previous six conversations about this topic have been closed as won't fix, I was reluctant to bring this up again. But not all facts have been brought up in the previous discussions and I believe explicitly disallowing PHP 8 is not the right decision. Please let me explain.

It's true the version constraints on Guzzle 5 and 6 were wrong, they unintentionally allow PHP 8. Guzzle 5 does not work on PHP 8 and the version constraints have been updated [1] accordingly. Guzzle 6 is in security-only mode so did not receive the PHP 8 fixes Guzzle 7 had and PHP 8 is not officially supported. But accidentally, it does work. Looking at the changes done to Guzzle 7 to support PHP 8 [2], they only touch annotations and tests.

While nobody will guarantee every part of the library works on PHP 8, in practice Guzzle 6 runs fine on PHP 8, including the async functions used by this adapter. Also, Drupal 9.1 officially supports PHP 8 and they do that using Guzzle 6.5.5.

So, yes it's good advice to tell people to upgrade to Guzzle 7. A warning about using Guzzle 6 on PHP 8 might be appropriate too. But I don't see why people depending on Guzzle 6 should be forced to fork this repository if they have a depedency on this adapter.

If there actually is an incompatibility I'd appreciate it if someone could point out what part is incompatible.

[1] guzzle/guzzle#2834
[2] https://github.com/guzzle/guzzle/pull/2715/files

Related issues: #76 #77 #78 #80 #81 #82

@dbu
Copy link
Contributor

dbu commented Feb 1, 2021

i understood that guzzle 6 has failures on php 8. maybe it depends on which guzzle 6 minor version, in addition?

given how simple the adapter is, what is the problem with using guzzle7-adapter?

@jorissteyn
Copy link
Author

That's preferrable of course, but if for some reason you must use Guzzle 6 (in my case, Drupal doesn't yet support Guzzle 7) then you must use the Guzzle 6 adapter.

@dbu
Copy link
Contributor

dbu commented Feb 1, 2021

@sagikazarmark do you have inputs on the topic of guzzle 6 on php 8? my understanding was that there are issues - but if there are none, @jorissteyn arguments convinced me that we should allow installation of this adapter on php 8

@dbu
Copy link
Contributor

dbu commented Mar 2, 2021

merged PHP 8 support in #86, will tag a release shortly.

@dbu dbu closed this as completed Mar 2, 2021
@dbu dbu mentioned this issue Mar 2, 2021
2 tasks
@dbu dbu mentioned this issue Mar 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants