Upgrade to Guzzle 7

What would the next steps be for Guzzle 7 compatibility? Are there other deprendcies we should consider updating? If do they should make have their own issues.

Thanks for working on this issue. Still no new beta/RC/release from Guzzle. (The patch also no longer applies.) Therefore, I think postponed is the most accurate status. We were hoping we'd get Guzzle 7 before our RC, but looks like that's not happening.

So, I'm moving to 9.1.x. We can't/shouldn't actually bump the Guzzle requirement from 6 to 7 in a minor, but we can test it once stable and ensure we're forward-compatible for it for D10.

Guzzle 7.0.0 has been released, so we should create a new patch and test against the stable release.

+++ b/composer.json
@@ -12,7 +12,8 @@
+        "guzzlehttp/guzzle": "7.0.1 as 6.5.5"

+++ b/composer.lock
@@ -7177,7 +7240,14 @@
+    "aliases": [
+        {
+            "alias": "6.5.5",
+            "alias_normalized": "6.5.5.0",
+            "version": "7.0.1.0",
+            "package": "guzzlehttp/guzzle"
+        }
+    ],

Uh. Wut? Why the aliasing?

Cleaning up leftover beta targets from 9.0.x. We still need this testing issue for D10.

According to @Gábor Hojtsy we are deprecating the Goutte driver in #3176655: Deprecate GoutteDriver use in core and use Behat\Mink\Driver\BrowserKitDriver directly instead. That removes the blocker.

Steps to create this patch:

1. diff --git a/composer.json b/composer.json
   index 12508828a1..d984b596ce 100644
   --- a/composer.json
   +++ b/composer.json
   @@ -17,7 +17,6 @@
        "require-dev": {
            "behat/mink": "^1.8",
            "behat/mink-browserkit-driver": "^1.3",
   -        "behat/mink-goutte-driver": "^1.2",
            "behat/mink-selenium2-driver": "^1.4",
            "composer/composer": "^2.0.2",
            "drupal/coder": "^8.3.7",
   diff --git a/core/composer.json b/core/composer.json
   index 46542ac447..7cbc411da2 100644
   --- a/core/composer.json
   +++ b/core/composer.json
   @@ -34,7 +34,7 @@
            "twig/twig": "^2.12.0",
            "doctrine/reflection": "^1.1",
            "doctrine/annotations": "^1.4",
   -        "guzzlehttp/guzzle": "^6.5.2",
   +        "guzzlehttp/guzzle": "^7.2.0",
            "symfony-cmf/routing": "^2.1",
            "laminas/laminas-feed": "^2.12",

2. rm -rf vendor
3. composer install
4. composer remove behat/mink-goutte-driver
5. COMPOSER_ROOT_VERSION=9.2.x-dev composer update drupal/core guzzlehttp/guzzle

Aight, looks like we've some compatibility issues to resolve:

1. Media.Drupal\Tests\media\Functional\ProviderRepositoryTest
   ✗	
   Drupal\Tests\media\Functional\ProviderRepositoryTest
   fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-731.xml:
   PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.
   
   Testing Drupal\Tests\media\Functional\ProviderRepositoryTest
   ..F.                                                                4 / 4 (100%)
   
   Time: 53.25 seconds, Memory: 6.00 MB
   
   There was 1 failure:
   
   1) Drupal\Tests\media\Functional\ProviderRepositoryTest::testNonExistingProviderDatabase with data set #0 ('http://oembed1.com/providers.json', 'Could not retrieve the oEmbed...s.json')
   Failed asserting that exception of type "GuzzleHttp\Exception\ConnectException" matches expected exception "Drupal\media\OEmbed\ProviderException". Message was: "cURL error 6: Could not resolve host: oembed1.com (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://oembed1.com/providers.json" at
   /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:210

2. Test.Drupal\Tests\Core\Test\BrowserTestBaseTest
   ✗	
   Drupal\Tests\Core\Test\BrowserTestBaseTest
   fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-621.xml:
   PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.
   
   Testing Drupal\Tests\Core\Test\BrowserTestBaseTest
   .W..                                                                4 / 4 (100%)
   
   Time: 288 ms, Memory: 6.00 MB
   
   There was 1 warning:
   
   1) Drupal\Tests\Core\Test\BrowserTestBaseTest::testGetHttpClientGoutte
   Class "Goutte\Client" does not exist.

3. Also aggregator issue; although we might deprecate the module in core, the contrib version will still need to resolve:
   

   Aggregator.Drupal\Tests\aggregator\Functional\FeedParserTest
   ✗	
   Drupal\Tests\aggregator\Functional\FeedParserTest
   exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-19.xml:
   PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.
   
   Testing Drupal\Tests\aggregator\Functional\FeedParserTest
   ....E                                                               5 / 5 (100%)
   
   Time: 1.05 minutes, Memory: 6.00 MB
   
   There was 1 error:
   
   1) Drupal\Tests\aggregator\Functional\FeedParserTest::testInvalidFeed
   Behat\Mink\Exception\ExpectationException: The string "The feed from aSJTH0oJ seems to be broken because of error" was not found anywhere in the HTML response of the current page.
   

4. Plus a test that just needs an update for the change:

   Drupal\Tests\ComposerIntegrationTest
   fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-111.xml:
   PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.
   
   Testing Drupal\Tests\ComposerIntegrationTest
   .............S.............................................FF     61 / 61 (100%)
   
   Time: 822 ms, Memory: 6.00 MB
   
   There were 2 failures:
   
   1) Drupal\Tests\ComposerIntegrationTest::testVendorCleanup with data set #0 ('Drupal\Core\Composer\Composer', 'packageToCleanup')
   Failed asserting that an array contains 'behat/mink-goutte-driver'.

I think this will resolve #33.4; points 1-3 still need fixing.

Ah yep, first bit of #35 makes sense as simply part of the dep removal. Attached should address that as well, leaving #33.1 and .3.

Oh, we need to remove the whole test, not just the one method:

	Browsertestbase.Drupal\FunctionalTests\GoutteDriverTest
✗	
Drupal\FunctionalTests\GoutteDriverTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-82.xml:
PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\FunctionalTests\GoutteDriverTest
E                                                                   1 / 1 (100%)

Time: 4.99 seconds, Memory: 4.00 MB

There was 1 error:

1) Drupal\FunctionalTests\GoutteDriverTest::testGoTo
Error: Class 'Behat\Mink\Driver\GoutteDriver' not found

/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:320
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:221
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:370
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

ERRORS!
Tests: 1, Assertions: 1, Errors: 1.

Moved the Goutte removal into its own issue and fixing it up there: #3187315: Remove mink-goutte-driver as a core dependency

OK, let's see if I can make an MR on top of the other issue work nicely...

OK, here we go. This is branched from #3187315: Remove mink-goutte-driver as a core dependency and can be rebased against that issue's branch until it lands, whereupon we rebase against HEAD and git magically resolves everything. 🤞

To keep this up to date:

1. Ensure #3187315: Remove mink-goutte-driver as a core dependency is up to date with a rebase against 9.2.x and passing tests.
2. git revert d9f6ffe (or whatever the latest commit is to update the constraint).
3. git remote add drupal-3104353 git@git.drupal.org:issue/drupal-3104353.git
4. git fetch drupal-3104353
5. git checkout -b '3187315-goutte' --track drupal-3187315/'3187315-goutte'
6. git checkout 3104353-guzzle
7. git rebase 3187315-goutte (and resolve any conflicts).
8. rm -rf vendor
9. composer install
10. COMPOSER_ROOT_VERSION=9.2.x-dev composer update drupal/core guzzlehttp/guzzle
11. git commit -am "Update lockfile and metapackages."
12. git push --set-upstream drupal-3104353 HEAD

Note that the commit I just pushed might get the Media test passing, but there are other places in HEAD that could be affected and just might be lacking test coverage:

[ayrton:drupal | Wed 14:46:56] $ grep -rl "use GuzzleHttp\\\Exception\\\RequestException" * | grep -v "vendor"
core/tests/Drupal/Tests/DrupalTestBrowser.php
core/includes/install.core.inc
core/modules/locale/locale.batch.inc
core/modules/update/src/UpdateFetcher.php
core/modules/aggregator/src/Form/OpmlFeedAdd.php
core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php
core/modules/system/system.module
core/modules/media/src/OEmbed/ProviderRepository.php
core/modules/media/src/OEmbed/ResourceFetcher.php
core/modules/media/src/OEmbed/UrlResolver.php
core/modules/media/src/Plugin/media/Source/OEmbed.php

So that's green, but it's possible the following other places also might need to be changed and just lack test coverage, since they also catch the same exception:

1. core/tests/Drupal/Tests/DrupalTestBrowser.php
2. install_retrieve_file() and install_check_localization_server()
3. locale_translation_http_check()
4. core/modules/update/src/UpdateFetcher.php
5. core/modules/aggregator/src/Form/OpmlFeedAdd.php
6. system_retrieve_file()
7. core/modules/media/src/OEmbed/ResourceFetcher.php
8. core/modules/media/src/OEmbed/UrlResolver.php
9. core/modules/media/src/Plugin/media/Source/OEmbed.php

Both Guzzle\Exception\RequestException and Guzzle\Exception\ConnectException exist in both Guzzle 6 and 7. However, in Guzzle 6, ConnectException extends RequestException, whereas in Guzzle 7 they are siblings (both extending TransferException directly) that implement different interfaces. This means that it's also probably safe to backport the simple fixes I made above to D9; it'd just be slightly redundant.

So whether we catch both or not might differ on a case-by-case basis. My guess is that in most cases we'll want to catch both because usually all we care about is that the request didn't work and we want to log, handle, or re-throw that. In other cases where something depends on the response, ConnectException might not be appropriate. At least one of the cases above (forget which) checked for a null response within the catch, so it's possible that spot should actually be refactored a little.

Edit: We could also potentially simplify it even more and just catch TransferException (the parent) in most cases.

Going to make that change.

I have not changed DrupalTestBrowser to use TransferException, because it re-throws the exception if the response is NULL, and that's always the case for ConnectException. I also added an additional catch block to locale_translation_http_check() since the logic of the existing catch relies on methods that ConnectException no longer has.

@longwave Yep, my next step if that was green was to make a separate issue to exactly that. And green it is, so doing that now. :)

Posted #3188920: Make Guzzle exception handling forward-compatible with Guzzle 7 which is green and NR!

Ugh, please ignore the two branches I pushed by accident there.

Retitling and postponing this now that we have an actual path forward for D10.

Rebased onto latest 9.2.x and #3187315: Remove mink-goutte-driver as a core dependency.

Updating outdated IS.

Closed the old MR and rebuilt the new one atop the new branch of #3187315: Remove mink-goutte-driver as a core dependency which was itself rebuilt atop 9.3.x.

@longwave Yes, as per my comment on #3224421-8: [PHP 8.1] Add a shim to Guzzle 6 for PHP 8.1 compatibility we can change our constraint to "guzzlehttp/guzzle": "^6.5.2 || ^7.3.0", so that sites can use it, but keep core-recommended pinned to Guzzle 6.

@Murz Note that Drupal 9 itself will not be updated to use Guzzle 7 as in this issue, because that would be a major version break and violate semver. This is a Drupal 10 readiness issue and onl filed against 9.3.x because 10.0.x is currently only a stub. For help using Composer to override/alias dependencies and such, I would suggest asking in the #composer channel in the Drupal Slack.

@Taran2L, I do not believe the merge you made was necessary. This branch is periodically rebuilt, but the previous MR was still passing. I'm removing issue credit for this pending an explanation. But thanks for your interest in this issue!

I think a separate issue should be filed for allowing Guzzle 7 in our constraint in D9. This issue is for D10. Thanks!

Thanks @Taran2L! For these dependency update issues, it is better to simply merge the non-composer-command commits and then re-run the composer command. I'll try to put instructions in the IS about this. Adding back credit -- thanks for the explanation.

Let's put this in the RTBC queue as a support request like #3044175: [DO NOT COMMIT] Vendor minimum testing issue to get retesting on it. That way we can keep an eye on our Guzzle 7 compatibility.

(For now.)

Posted #3225966: Consider loosening our constraint to allow sites to install Guzzle 6 or 7, or otherwise handle PHP 8.1 deprecations for Guzzle 6.

Unrelatedly, since Guzzle 6 is now security-only, upgrading to Guzzle 7 is now a beta blocker for D10.