Consider loosening our constraint to allow sites to install Guzzle 6 or 7, or otherwise handle PHP 8.1 deprecations for Guzzle 6

If we going to do this to fix PHP 8.1 then we'd need to enable some way of updating to Guzzle 7 while testing on PHP 8.1. Also this will be the first time the tarball won't be compatible with all supported versions of PHP so we going to need to do something about that. Also for composer users core-recommend won't be fully compatible with PHP 8.1 either. Perhaps we should return to https://github.com/guzzle/guzzle/pull/2918 - maybe trying again with the point about if something is security supported then being fully compatible with all supported PHP versions. This might then mean that they make Guzzle 6 incompatible with PHP 8.1 which means we'll need to do something to make core-recommend installable on PHP 8.1.... I think this is all going to turn out to be very tricky. The lock file and core-recommended are going to get in the way.

OTOH, Guzzle 6 is not broken by PHP 8.1 - it emits lots of E_DEPRECATED so could consider making such deprecations skippable for the purposes of testing. And then we can proceed here with the plan in issue summary for people that want to use Guzzle 7. If this is what we do we'll have to hope Guzzle 6 is not marked as incompatible with PHP 8.1.

> The issue with that though is that when PHP 8.2 comes out in 2022, D9 will be actually broken on that PHP version

Why do you think this? As far as I can see, PHP will be following semver and the new deprecations introduced in PHP 8.1 will not become hard failures until PHP 9, which has no known release date yet. For reference please see the examples at https://wiki.php.net/rfc/internal_method_return_types which explicitly mention PHP 9.

I think we should consider doing this. Even though we've managed to make Drupal 9 not trigger any deprecations from Guzzle 6 or skip some - there are other deprecations that Guzzle 6 will trigger when it's used by contrib projects. For example:

Return type of GuzzleHttp\Command\Command::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

It's hard to skip these messages because they are not user deprecations but they are language level deprecations.

@alexpott, true, I'm hitting exactly that error in one of our projects at European Commission while trying to run on PHP 8.1 (and prepare for D10)

@alexpott, ref #4

If we going to do this to fix PHP 8.1 then we'd need to enable some way of updating to Guzzle 7 while testing on PHP 8.1.

Seems that this is not doable from Drupal core code but needs a pipeline change (?)

Also this will be the first time the tarball won't be compatible with all supported versions of PHP so we going to need to do something about that. Also for composer users core-recommend won't be fully compatible with PHP 8.1 either.

Not sure I get this.

I've temporary updated composer.lock just to see the tests on PHP 8.1

• Added CR https://www.drupal.org/node/3268032
• Updated IS
• Added release notes snippet

We should decide what to do with the tarball & core-recommend (see #4)

More IS updates.

Tagging with PHP 8.1

Fixing title

@xjm but this MR allows tests with both Guzzle 6 and 7. Keeping the constraint to only allow Guzzle 6 effectively blocks many sites to run on PHP 8.1 and, as an effect, they cannot prepare for Drupal 10

Should we mark this as Critical?

It tests Guzzle 7 on PHP 8.1, but it does not test Guzzle 7 on other PHP versions. So we could easily regress Guzzle 7 support on PHP 8.0 or whatever as it stands. Or we could further regress Guzzle 6 support on PHP 8.1. We need min/max testing.

So this is true but it seems most likely that Guzzle 6 or 7 regressions on the respective PHP versions will be due to changes in either PHP or Guzzle, which for Guzzle 6 won't be fixed, and for Guzzle 7 it's probably better than it not being possible to use it with 9.4 at all. Without min/max testing, it seems the least worst option (vs. forcing everyone onto guzzle 7 or keeping everyone stuck on guzzle 6).

Core recommended I can't think of anything that doesn't require min/max testing but it would be worth an explicit follow-up.

> Everything above says it is only deprecation notices for theoretical contrib

Yes, but it's non-suppressed PHP deprecation notices, which by default fail tests and are AFAIK also logged?

I'm not sure I have an opinion on this issue, it's similar to discussions we had around allowing newer twig/symfony major versions. I see the problems with both options.

As for core-recommends, I'm not sure how much work it would be to have a drupal/core-recommends-81 package, I'm assuming not trivial? That said, using of core-recommends is optional (we don't use it atm), so *if* we'd *not* use a newer guzzle for testing then it would kind of make sense to say that drupal-composer is locked to the older version, but you can use drupal/core or use a version alias? Of course the first would result in those that already use drupal/core to switch.

An option could be to add guzzle 6 explicitly to https://github.com/drupal/recommended-project/blob/9.4.x/composer.json (would only help new projects) and a change record/release note that tells existing sites to add that requirement to their projects if they experience problems with guzzle 7?

Just this issue I was looking for. Thanks for working on this.

Ran into a contrib use case trying to use upgrade_status in my local dev environment and wanted to document. Since we emit errors in dev environments, this deprecation is breaking json responses when trying to run a scan.

ResponseText: Deprecated: Return type of GuzzleHttp\Cookie\CookieJar::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /app/vendor/guzzlehttp/guzzle/src/Cookie/CookieJar.php on line 220
Deprecated: Return type of GuzzleHttp\Cookie\CookieJar::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /app/vendor/guzzlehttp/guzzle/src/Cookie/CookieJar.php on line 225
{"status":true,"percentage":"100","message":"Completed 1 of 1.","label":"Analysis complete for pathauto."}

This may not be a critical use case but it does make creating a bullet proof environment for testing upgrades a bit harder since PHP 8.1 isn't fully supported here.

I posted a possible work around for upgrade_status but I think its still worth considering loosening. Core isn't the only thing using guzzle, I also have internal libraries using it, marketing libraries that use it, etc. It would be much easier to be testing all of these things at once on Guzzle 7 and PHP 8.1 in 9.4 before upgrading.

We have added 4 new environments to drupalci that run a `composer update --with-all-dependencies` after it installs instead of using the lock file. We can adjust the command if necessary, but thats what we've got right now.

The names in the environments list are using mysql 5.7, php 7.3/7.4/8.0 and 8.1 and have the term "updated deps".

I think these are probably failing because it upgraded phpunit along with everything else.

This error probably gives us the proper clue:

12:24:53 Drupal requires Prophecy PhpUnit when using PHPUnit 9 or greater. Please use 'composer require --dev phpspec/prophecy-phpunit:^2' to ensure that it is present.

Im not sure what I should be upgrading, or what it is we want to ensure works, exactly.. is it *just* guzzle? if so I can reduce the breadth of the update.

I think we should only exclude PHPUnit from the composer update, because that's handled separately by drupal-phpunit-upgrade anyway. So I think that would mean something like:
composer update --with-all-dependencies --with phpunit/phpunit:^8? However, that ^8 would need to be different for Drupal 10 than for Drupal 9; is there a way to do it without having to hard-code that (e.g., read it from the root composer.json)?

--with phpunit/phpunit:8^ doesnt seem to work, but I think theres a better option. Going to try --no-dev which will not upgrade our dev dependencies.

Turns out that --no-dev really just means "Upgrade the dev dependencies, but dont install them" so that didnt work either.

ButI figured out some nice hackery sprinkles to put on top of the hacky ice cream cone to make this work.
composer update --with-all-dependencies --with phpunit/phpunit:$(composer show -i |grep phpunit\/phpunit |awk '{print $2}')

This will upgrade everything, except keep phpunit/phpunit in whatever version it was in before. As a bonus it doesnt actually change anything regarding phpunit, so the upgradephpunit script that runs on php8.1 still works.

Theres two jobs running now, and tests are happening.

Nice. Maybe still needs a CR to let projects know that they can update Guzzle?

Sorry, there’s already a CR

Heck yeah. I was so confused as to what was going on but glad its sorted out XD.

+1 to RTBC, looks right and sounds like we've got a way to test it now.

@xjm. as projects are on 9.3 and will update soon on 9.4 wouldn't be OK to merge this in 9.4.x, or even 9.3.x? The reason is that sites should make the conversion to PHP 8.1 and shouldn't wait for 9.5.0 for this

Committed/pushed to 9.5.x and 9.4.x, thanks!

@claudiu.cristea the intention with this one has always been to get it into 9.4.x (or 9.3.x if it had been ready by 9.3 beta). It's late enough in the 9.3 cycle that I reckon anyone upgrading to 8.1 and blocked on this would just start testing with 9.4 at this point in a dev branch or wait a couple more weeks.

Yay

Oh, actually it’s 9.4

@catch, doesn't seem to be committed in 9.4.x https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/composer.json

Thanks, pushed failed. Did it again.

@xjm asked me to create the 2 follow ups

#3281666: Tarball sites:will use Guzzle 6 which has deprecation warnings in PHP 8.1

#3281667: Come up with a way to allow core-recommended (and tarballs?) to install Guzzle 7 for PHP 8.1 compatibility and Laminas-feed 2.19 for PHP 8.2 compatibility

Right now they are pretty much just place holders. I will read this issue and add any of the idea you folks have come up with. Or feel free to update the issues yourselves. thanks

This already has a release note snippet, but was not yet tagged for the 9.4 release notes, doing so now!

@xjm, Thanks, good to have that recorded.

keeping core-recommended pinned to a specific version leaves us all in the possition we are in today. CVE-2022-31043 is a new guzzle vulnerability that requires updating to 6.5.7, but core-recommended is pinned to 6.5.6:

"name": "drupal/core-recommended",
"require": {
...
"guzzlehttp/guzzle": "6.5.6",
...
}

So now I cannot simply fix the vulnerability with "composer update", I have to wait for core to publish a release. This is crazy. It should be changed to:

"name": "drupal/core-recommended",
"require": {
...
"guzzlehttp/guzzle": "^6.5.6",
...
}

EDIT: https://www.drupal.org/project/drupal/issues/3285157

@xeM8VfDh I appreciate your energy, but:

1. core-recommended literally is pinning. That's what it's for. You don't have to use it.
2. This issue fixed the very thing you are reporting. Did you even look?

I am on the latest 9.3.15 and composer update does not bump my guzzle due to what I mentioned. Is this fix not deployed, or am I missing something? I am using Pantheon, so I didn't explicitly include core-recommended... maybe this is a Pantheon issue?

Even if I am missing something, core-recommended needs to be updated to fix the CVE. Additionally, I don't see why on earth we would pin to a specific version number as opposed to a minor release ('6.5.6' instead of, say, '^6.5'). That seems do go againt versioning best practices, no?

@xjm, I am using both drupal/core and drupal/core-recommended, is that wrong?

#3198340: Strict constraints in drupal/core-recommended make it harder for Composer-managed sites to apply their own security updates when a core update is not available is probably the better issue for the discussion on pinning of core-recommended.

maybe this is the issue:

"name": "pantheon-upstreams/upstream-configuration",
"version": "dev-master",
"dist": {
    "type": "path",
    "url": "upstream-configuration",
    "reference": "07d637fbfe37092f9e4b8bd965b546d2af61db58"
},
"require": {
    ...
    "drupal/core-composer-scaffold": "^9",
    "drupal/core-recommended": "^8.8 || ^9",
    ...
},

EDIT: but then again, I think the solution isn't dropping core-recommended, but rather pinning to a specific minor release in order to get security updates. Maybe even pin to a major release. Whatever the case, core-recommended needs to be updated to pin 6.5.7 at the very least, but should probably pin ^6.5 or ^6

thanks @cmlara for pointing that issue out, I've commented there.

@xeM8VfDh This will be in 9.4.0 which is out soon. We close issues here when the commits occur not when the code releases.

Maybe am missing something. I see two commits here, neither of which address drupal/core-recommended, which is currently pinned to an explicit version of guzzle thats associated with a CVE vulnerability. Am I missing a commit somewhere?

#70, #75

The commits touch the composer.json in drupal/core. My issue is related to drupal/core-recommended. That's a separate repo and composer.json, no?

I see you recommended getting off of core-recommended, but I can't because Pantheon controlls that. What I am saying, is that core-recommended is exposed to a vulnerability. Regardless of whether you think I should get off it or not, doesn't it need to be fixed for those that are using it?

What I am saying, is that core-recommended is exposed to a vulnerability. Regardless of whether you think I should get off it or not, doesn't it need to be fixed for those that are using it?

Yes. Regrettably, we can't build releases instantly.

Lol, I am not trying to be an ass here, I am just genuinely confused. I realize we don't have instant builds. That's fine. I am happy waiting until 9.4.0 but what I am trying to convey to you is my confusion as to whether the fix here actually fixes core-recommended. As far as I can tell, the commits only touch drupal/core. Isn't drupal/core-recommended a separate repo entirely? If so, I fail to see how the commits here fix the vulnerable pinned guzzle version in drupal/core-recommended.

Am I wrong? I'm happy to be wrong, all I am concerned about is that some upcoming release bumps the pinned guzzle version in drupal/core-recommended.

You are not wrong. This issue does nothing for core-recommended. If you want minor- and patch-release control over dependencies, do #70. If your hosting provider doesn't allow that you will have to make your own decisions.

#3198340: Strict constraints in drupal/core-recommended make it harder for Composer-managed sites to apply their own security updates when a core update is not available is the issue moving forward.

I keep saying that. But according to @xeM8VfDh Pantheon doesn’t allow that. It would be nice to see a doc citing that although I don’t think that affects this issue much for the time being.

Regarding Pantheon and drupal/core-recommended, the current default upstream is not prescriptive about whether you use drupal/core or drupal/core-recommended, although the later is the default. Once you create your site, you are free to switch to drupal/core if you wish.

The Pantheon code shown in #73 is from an older version of the upstream. At one time, the require for drupal/core-recommended was in a place that individual sites could not modify. Sites using the older upstream may convert to the current upstream using the deprecated upstream migration guide.

Thanks for the updated information @greg.1.anderson, I appreciate you pointing me in this direction

For those unwilling to leave the warm embrace of drupal/core-recommended but who would still like to use PHP 8, I've had success by using cweagans/composer-patches to patch Guzzle itself. I added the following values to my composer.json and committed the attached patch to my project:

{
  // snip.
  "extra": {
    // snip.
    "patches": {
      "guzzlehttp/guzzle": {
        "Suppress deprecation warnings raised because of Guzzle 6.5 on PHP 8. Remove for Guzzle 7. See https://www.drupal.org/project/drupal/issues/3225966.": "path/to/file/in/my/repo.patch"
      }
    }
  }
}

The above patch in #86 works for me using `drupal/core-recommended` version 9.5.11 on PHP 8.1.11, thanks.

The interesting part: when we remove the package "drupal/core-recommended", the site is running fine with out any issue with Guzzle 7.
As, the package drupal/core-composer-scaffold and drupal/core-project-message is having a dependency of drupal/core.

anyone think, there might be an issue when we remove the package "drupal/core-recommended" completely ?