Problem/Motivation

Guzzle 6 passes NULL as the $numeric_prefix parameter to http_build_query(). PHP 8.1 has tightened up parameter types and only allows strings here.

The Guzzle team will not fix this as Guzzle 6 is considered end of life, and the issue does not exist in Guzzle 7: https://github.com/guzzle/guzzle/pull/2918

Steps to reproduce

Proposed resolution

Guzzle does not use use function or prefix any function calls with \ so we can inject a shim into the GuzzleHttp namespace that intercepts the call and passes on the correct argument.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
2.44 KB
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

I'm undecided if we should push back a bit on https://github.com/guzzle/guzzle/pull/2918 and point out that is something is security supported then it needs to work on all supported PHP versions. They could reasonably say that triggering a deprecation doesn't mean anything is broken.

That said, this is an elegant fix that buys us sometime to potentially sort out Guzzle 7 compatibility in Drupal 9 without the pressure of the PHP 8.1 release date.

I've added this shim to #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) and it fixes the PHP 8.1 deprecation notices due to Guzzle 6.

catch’s picture

I think I would probably go for:

1. Commit this anyway

2. Work on Guzzle 7 compatibility - we need it for 10.x anyway.

3. If we hit problems with Guzzle 7 (apart from our own min/max testing issues which aren't their fault), then go back to ask for a commt.

longwave’s picture

The only thing blocking us on Guzzle 7 right now is our dependency on behat/mink-goutte-driver and in turn fabpot/goutte which only supports Guzzle 6, otherwise we require no code changes: #3104353: Upgrade to Guzzle 7

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll. I've bumped the Guzzle 7 issues.

andypost’s picture

Issue tags: +Needs reroll
xjm’s picture

I think we're leaning toward doing #3187315: Remove mink-goutte-driver as a core dependency in 9.3.x given the low risk of disruption. However, I think we should also still do this issue, because there are some more disruptive changes between Guzzle 6 and 7. We've already addressed the ones that affected core, but there are a number of things that could affect other projects as well.

We can also open an issue to consider making our constraint be:
"guzzlehttp/guzzle": "^6.5.2 || ^7.3.0",
...but keep core-recommended pinned at the lower version and use the min/max retesting workaround to ensure we stay forward-compatible with Guzzle 7.

alexpott’s picture

Guzzle 6 compatibility with PHP 8.1 has gotten a bit more complex with the latest round of deprecations in PHP. Classes which extend \Countable and \IteratorAggregate so \GuzzleHttp\Cookie\CookieJar::count(), \GuzzleHttp\Cookie\CookieJar::getIterator() and \GuzzleHttp\Handler\MockHandler::count() all need the annotation #[\ReturnTypeWillChange] added to avoid PHP 8.1 emitting the deprecation. And I wouldn't be surprised if there were more deprecations by the end of the beta cycle. PHP 8.1 seems to be in the process of tightening quite a few things up.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.44 KB
932 bytes

Just re-rolled the patch in #2, thanks!

Status: Needs review » Needs work

The last submitted patch, 10: 3224421-10.patch, failed testing. View results

mondrake’s picture

Issue tags: +PHP 8.1
andypost’s picture

Status: Needs work » Needs review
FileSize
460 bytes
2.44 KB

fix checksum COMPOSER_ROOT_VERSION=9.3.x-dev composer update --lock

alexpott’s picture

Status: Needs review » Needs work

Given #9 we're going to need to come up with another plan or extend this one using class aliases.

alexpott’s picture

Status: Needs work » Needs review

In #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) we've managed to work around the deprecations triggered by Guzzle. I think potentially we should continue on here with the shim and ignore the deprecations and be able to do #3225966: Consider loosening our constraint to allow sites to install Guzzle 6 or 7, or otherwise handle PHP 8.1 deprecations for Guzzle 6 at more leisurely pace.

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I think we should add a test with the second parameter being NULL in combination with PHP 8.1.

daffie’s picture

I think we shall have to choose how we are going to fix this. To me the possible options are:

  1. Keep Guzzle version 6 and do not support PHP 8.1 on D9.3. Support for PHP 8.1 shall have to wait for D10. To me, this is not a very acceptable option.
  2. Upgrade to Guzzle version 7 and have PHP 8.1 support on D9.3. D9.3 sites that depend on Guzzle 6 will break. There will not be many sites out there, but supporting PHP 8.1 on D9.3 is just far more important. We can allow sites to downgrade to Guzzle version 6, only then for those sites no more PHP 8.1 support.
  3. Maintain 2 different D9.3 branches, one for Guzzle 6 and one for Guzzle 7. Will be more work to maintain, more work (and cost) for the testbot. It is doable, only not very attractive. Also a lot of communication to the Drupal community about the 2 branches and how they are different.
  4. Pay the maintainers of Guzzle to support PHP 8.1 on Guzzle version 6. Lets ask them if that is interesting to them and if so, how much will it cost. Use the Drupal community to ask which sites are on D9 and will break when we upgrade to Guzzle version 7. Ask those site owners for the money to pay the Guzzle maintainers. Tell those site owners that the alternative is option #2. Also is it an option that the Drupal Association put in some or all of the money.

For me is option #4 to most attractive, because D9.3 can stay on Guzzle version 6 and have support for PHP 8.1. When this is not possible then for me it would be option #2.

daffie’s picture

Priority: Normal » Critical

Fixing this for D9.3 is critical.

alexpott’s picture

Status: Needs work » Needs review

@daffie we can use Guzzle 6 on PHP 8.1 if we do the patch here and skip some deprecations - that's working just fine on #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves). Re #17 - I don't think we need an explicit test here. Once we have testing on PHP 8.1 we get test coverage - we can't have that yet because the test suite does not pass because of issues like this.

I think there is a lot of value changing the constraints to allow Guzzle 7 in Drupal 9 but then we're back to the old min/max testing issue that we've still not solved and I think we can use the approach in this issue and deprecation skipping to get us PHP 8.1 compatibility without having to solve that.

daffie’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Given the explanation of @alexpott in comment #20, the patch looks good to me.
The shim is added, when the parameter $numeric_prefix is null the value is changed to an empty array.
The docblock is copied from the PHP manual.
For me it is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#13 needs a re-roll.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.44 KB
1.23 KB

Just rerolled patch in #13, thanks!

Status: Needs review » Needs work

The last submitted patch, 23: 3224421-23.patch, failed testing. View results

daffie’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
461 bytes
2.44 KB

Rerolled.

  • catch committed 3c4ef6b on 9.3.x
    Issue #3224421 by ankithashetty, daffie, andypost, longwave, alexpott,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

This is great. We can continue looking at Guzzle updates within 9.3.x in the follow-up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.