Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#25 | 3224421-25.patch | 2.44 KB | daffie |
#25 | interdiff-3224421-23-25.txt | 461 bytes | daffie |
Comments
Comment #2
longwaveComment #3
alexpottI'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.
Comment #4
catchI 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.
Comment #5
longwaveThe only thing blocking us on Guzzle 7 right now is our dependency on
behat/mink-goutte-driver
and in turnfabpot/goutte
which only supports Guzzle 6, otherwise we require no code changes: #3104353: Upgrade to Guzzle 7Comment #6
catchNeeds a re-roll. I've bumped the Guzzle 7 issues.
Comment #7
andypostComment #8
xjmI 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.Comment #9
alexpottGuzzle 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.Comment #10
ankithashettyJust re-rolled the patch in #2, thanks!
Comment #12
mondrakeComment #13
andypostfix checksum
COMPOSER_ROOT_VERSION=9.3.x-dev composer update --lock
Comment #14
alexpottGiven #9 we're going to need to come up with another plan or extend this one using class aliases.
Comment #15
xjmPosted #3225966: Consider loosening our constraint to allow sites to install Guzzle 6 or 7, or otherwise handle PHP 8.1 deprecations for Guzzle 6.
Comment #16
alexpottIn #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.
Comment #17
daffie CreditAttribution: daffie commentedI think we should add a test with the second parameter being NULL in combination with PHP 8.1.
Comment #18
daffie CreditAttribution: daffie commentedI think we shall have to choose how we are going to fix this. To me the possible options are:
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.
Comment #19
daffie CreditAttribution: daffie commentedFixing this for D9.3 is critical.
Comment #20
alexpott@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.
Comment #21
daffie CreditAttribution: daffie commentedGiven 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.
Comment #22
catch#13 needs a re-roll.
Comment #23
ankithashettyJust rerolled patch in #13, thanks!
Comment #25
daffie CreditAttribution: daffie commentedRerolled.
Comment #27
catchThis is great. We can continue looking at Guzzle updates within 9.3.x in the follow-up.