Deprecate GoutteDriver use in core and use Behat\Mink\Driver\BrowserKitDriver directly instead

tag to file issue to remove Goutte from dependencies

+1 to removing the dependency here.

Removing Goutte here will also unblock #3104353: Upgrade to Guzzle 7

This doesn't look like it is going to be simple. The test http client uses Guzzle and if we switch to BrowserKit driver we'd need to use something that implements \Symfony\Contracts\HttpClient\HttpClientInterface. I think given Goutte works on PHP 8 with no issues it might be best to do PR to allow it's use on PHP 8.

Yes, it require to add new dev dependency on symfony/http-client ^4.4 || ^5.1

Fix status, btw tests could use \Symfony\Component\HttpKernel\Client and existing middleware will provide logging

The related has collision with #6 but https://symfony.com/blog/new-in-symfony-4-3-httpclient-component

Moreover SF client is async out of box

PS: the comparison https://www.reddit.com/r/PHP/comments/hub30w/guzzle_vs_symfony_http_client/

#10/11 seems like a follow-up.

composer require --dev "symfony/http-client ^4.4 || ^5.1"
composer require --dev behat/mink-browserkit-driver
composer remove --dev behat/mink-goutte-driver

That fixes the tests for me. But I get other cruft in composer.lock, so I'm not posting the patch

$ composer-lock-diff --no-links
+--------------------------+--------+---------+
| Dev Changes              | From   | To      |
+--------------------------+--------+---------+
| behat/mink-goutte-driver | v1.2.1 | REMOVED |
| fabpot/goutte            | v3.3.0 | REMOVED |
| symfony/http-client      | NEW    | v5.1.7  |
+--------------------------+--------+---------+

$ composer why behat/mink-browserkit-driver
drupal/drupal  9.1.x-dev  requires (for development)  behat/mink-browserkit-driver (^1.3)  

Attaching a patch following steps in #12

Copying a tiny bit of Goutte's code into our codebase allows at least some of the tests to run successfully.

This is a little bit more green for me. Here I copy all of the client code from Goutte and GoutteDriver\Client into our own DrupalTestBrowser. I think this might be an acceptable way to move forward. We can then open a follow-up issue to use SF's HttpClient in the future with less rush.

Fixing BrowserTestBaseTest

Fixing the other BrowserTestBaseTest :)

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/BrowserWithJavascriptTest.php
   @@ -151,9 +151,9 @@ protected function drupalGetWithAlert($path, array $options = [], array $headers
   -    // Log only for WebDriverTestBase tests because for Goutte we log with
   -    // ::getResponseLogHandler.
   -    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof GoutteDriver)) {
   +    // Log only for WebDriverTestBase tests because for BrowserKitDriver we log
   +    // with ::getResponseLogHandler.
   +    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof BrowserKitDriver)) {
   
   +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
   @@ -107,9 +107,9 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   -    // Log only for WebDriverTestBase tests because for Goutte we log with
   -    // ::getResponseLogHandler.
   ...
   +    // Log only for WebDriverTestBase tests because for tests using
   +    // BrowserKitDriver we log with ::getResponseLogHandler.
   +    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof BrowserKitDriver)) {
   
   @@ -338,9 +338,9 @@ protected function drupalGet($path, array $options = [], array $headers = []) {
   -    // Log only for WebDriverTestBase tests because for Goutte we log with
   -    // ::getResponseLogHandler.
   -    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof GoutteDriver)) {
   +    // Log only for WebDriverTestBase tests because for BrowserKitDriver we log
   +    // with ::getResponseLogHandler.
   +    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof BrowserKitDriver)) {
   
   @@ -464,9 +464,9 @@ protected function drupalUserIsLoggedIn(AccountInterface $account) {
   -    // Log only for WebDriverTestBase tests because for Goutte we log with
   -    // ::getResponseLogHandler.
   -    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof GoutteDriver)) {
   +    // Log only for WebDriverTestBase tests because for BrowserKitDriver we log
   +    // with ::getResponseLogHandler.
   +    if ($this->htmlOutputEnabled && !($this->getSession()->getDriver() instanceof BrowserKitDriver)) {
   

   This is a pragmatic compromise but it's not really correct - we should be ensuring that the mink driver's client is not our DrupalTestBrowser. But this does work... hmmm. Note we can't do $this->getSession()->getDriver()->getClient() because the selenium driver doesn't implement getClient().

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -219,7 +219,7 @@ abstract class BrowserTestBase extends TestCase {
   -    if ($driver instanceof GoutteDriver) {
   +    if ($driver instanceof BrowserKitDriver) {
          // Turn off curl timeout. Having a timeout is not a problem in a normal
          // test running, but it is a problem when debugging. Also, disable SSL
          // peer verification so that testing under HTTPS always works.
   

   This feels a bit odd. If we were using the HttpBrowser with BrowserKitDriver (the norm) we'd be setting up the SF http client here. I wonder if we should move this to \Drupal\Tests\BrowserTestBase::getDefaultDriverInstance()

   I think we need to be explicit about not supporting the Goutte driver anymore. Tricky.

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -537,7 +540,7 @@ protected function getSessionCookies() {
   -    if ($mink_driver instanceof GoutteDriver) {
   +    if ($mink_driver instanceof BrowserKitDriver) {
          return $mink_driver->getClient()->getClient();
        }
   

   This should do check that the driver's client is an instance of DrupalTestBrowser. If we were using HttpBrowser and therefore sf's http client this would not work.

4. +++ b/core/tests/Drupal/Tests/DrupalTestBrowser.php
   @@ -0,0 +1,253 @@
   +// @codingStandardsIgnoreFile
   

   We need to remove this before commit. And reformat the code to our standards and document where the code is from.

Another question posed by this issue is what are we losing by not using the GoutteDriver. That provides:

• \Behat\Mink\Driver\GoutteDriver::prepareUrl() - looks like \Behat\Mink\Driver\BrowserKitDriver::prepareUrl() is going to do the same thing as long as if (!$this->removeHostFromUrl && !$this->removeScriptFromUrl) { evals to true (which is does ATM).
• \Behat\Mink\Driver\GoutteDriver::reset() - this also calls $this->getClient()->resetAuth(); - I think we're fine because BrowserKitDriver calls $this->client->restart(); and that resets Auth too.
• \Behat\Mink\Driver\GoutteDriver::getClient is fine - it's a wrapper for type hinting purposes.
• \Behat\Mink\Driver\GoutteDriver::setBasicAuth() is a worry - \Behat\Mink\Driver\BrowserKitDriver::setBasicAuth() does not do the same thing at all. I'm not sure though.

At least nothing in contrib is using setBasicAuth() - see http://grep.xnddx.ru/search?text=setBasicAuth&filename= and we're not in core either. But we're probably losing some functionality here (which might not be being used by anyone).

Fixing #24.3 and part of #24.4

I think we should consider removing the auth and headers stuff from DrupalTestBrowser. I don't think it is needed. We're doing the header setting oursevles in \Drupal\Tests\basic_auth\Traits\BasicAuthTestTrait for example.

Here's what it'd look like without the suspiciously-unnecessary code.

Re #25/#23.1 the difference is that the Goutte mink driver is guaranteed atm (on v3) to have a guzzle client. Now we're using the browserkit driver it is not guaranteed at all.

Re #25.2 I really don't think so. Or it'd be an extreme edge case. As \Drupal\Tests\basic_auth\Traits\BasicAuthTestTrait shows we support headers directly in drupalGet() and that's the way you should be doing that.

FWIW I've done a bunch of custom tests around basic auth and have never used these methods. It works as expected in drupalGet() or if you need more control over the request then you do it the same way we do in the REST tests, call getHttpClient() and set everything up yourself.

#29 yeah the no change option is to ask @fabpot to make a Goutte v3 release that is PHP 8 compatible - it is compatible it is only composer that is not allowing it.

Doing the finally @todos and addressing #23.1

Changing vendor hardening may need to clean-up tests from https://github.com/minkphp/MinkBrowserKitDriver/tree/v1.3.4/tests

Otherwise looks ready

@andypost we already do that... the browser kit driver is a dependency of the goutte driver. We're not actually adding any new code to vendor here. We're removing 2 libs from vendor.

RE #32

+++ b/composer/Plugin/VendorHardening/Config.php
@@ -22,7 +22,6 @@ class Config {
   protected static $defaultConfig = [
     'behat/mink' => ['tests', 'driver-testsuite'],
     'behat/mink-browserkit-driver' => ['tests'],
-    'behat/mink-goutte-driver' => ['tests'],
     'behat/mink-selenium2-driver' => ['tests'],
     'composer/composer' => ['bin'],
     'drupal/coder' => [

You can see it the list already in the diff... :)

Misread patch, and indirect dependency case(

It may need CR or release nite snippet

I think we might need something. Looking at http://grep.xnddx.ru/search?text=Goutte&filename=&page=1 there behat config that'll have to change. And there's stuff like http://grep.xnddx.ru/node/30614854#line-219 which won't work.

Drafted CR https://www.drupal.org/node/3177235

With https://github.com/FriendsOfPHP/Goutte/commit/abc34af, is this still needed?
Or even if it's not *needed*, is it that we still should do this?

+++ b/core/tests/Drupal/Tests/DrupalTestBrowser.php
@@ -0,0 +1,263 @@
+      throw new \InvalidArgumentException('Setting a path in the Guzzle "base_uri" config option is not supported by Goutte yet.');

Seems unlikely anyone will trigger this, but mentioning Goutte in the message is confusing if we no longer use it.

Good point @longwave - and we can remove it from the dictionary :)

A random failure, re-queued

It looks ready

We need to tell people what to do re the situations in #37 - we also need to update the CR with those solutions. Ie. what to do if their behat config is using goutte and also what to do if their test code makes direct references to goutte code. I think this situation makes maybe means we should only remove the library in 9.2.x and leave it in 9.1.x? But we could still add DrupaTestBrowser to Drupal 9.1.x so they have something to swap to.

Here's a new patch the de-yoda's the ifs and uses Drupal snake case for variable names.

Fix tags

I think this situation makes maybe means we should only remove the library in 9.2.x and leave it in 9.1.x? But we could still add DrupaTestBrowser to Drupal 9.1.x so they have something to swap to.

Does that mean people won't be able to composer install on PHP 8 with Drupal 9.1.x? I had somewhat given up on full PHP 8 support in 9.1 a couple of weeks ago, but it seems theoretically possible this week.

#47 looks good.

@catch given this is only a dev dependency they would be able to install on PHP 8 as long as the don't include the core-dev package. I think if this was the last remaining dependency issue then maybe it'd be tempting but the doctrine one feel more intractable. It'd be nice to know exactly what is the impact of this change on the projects using behat testing.

With @bircher's help I've done a little more research on the behat situation. The great news is that the behat extension depends on goutte-driver already so as long as we don't break goutte using goutte in core then we're okay.

The one functional change is that the following code will result in test browser output not working on goutte anymore.

+++ b/core/tests/Drupal/Tests/UiHelperTrait.php
@@ -552,4 +552,18 @@ protected function cssSelect($selector) {
+  /**
+   * Determines if test is using DrupalTestBrowser.
+   *
+   * @return bool
+   *   TRUE if test is using DrupalTestBrowser.
+   */
+  protected function isTestUsingDrupalTestBrowser() {
+    $driver = $this->getSession()->getDriver();
+    if ($driver instanceof BrowserKitDriver) {
+      return $driver->getClient() instanceof DrupalTestBrowser;
+    }
+    return FALSE;
+  }
+

... perhaps this is acceptable. I'm going to test https://www.drupal.org/project/drupalextension with this patch and see what happens.

I have successfully run a behat test after applying this patch.

Steps to reproduce:

1. run composer require drupal/drupal-extension
2. run ./vendor/bin/behat --init
3. add a behat.yml in the root dir with the contents:
   

   default:
     suites:
       default:
         contexts:
           - FeatureContext
           - Drupal\DrupalExtension\Context\DrupalContext
           - Drupal\DrupalExtension\Context\MinkContext
           - Drupal\DrupalExtension\Context\MessageContext
           - Drupal\DrupalExtension\Context\DrushContext
     extensions:
       Drupal\MinkExtension:
         goutte: ~
         selenium2: ~
         base_url: REPLACE_ME_WITH_BASE_URL
       Drupal\DrupalExtension:
         api_driver: 'drupal'
         drupal:
           drupal_root: ""
   

   Replacing REPLACE_ME_WITH_BASE_URL with the base url of the site.

4. Create a file features/d8.feature with the contents
   

   @api
   Feature: Example feature
     In order to have a example of behat test
     As an anonymous
     I need to be able to see the homepage
   
     Scenario: See the homepage
       Given I am on the homepage
   

5. Run a the test ./vendor/bin/behat -v

No changes we necessary due to this patch. Given the drupal extension includes goutte and we;ve not broken it I think we're good from that perspective. Yes the drupal extension will need to do work to be PHP 8 compatible but that's okay - with the new DrupalTestDriver they can be - or they can only support other drivers on PHP 8.

So looking at http://grep.xnddx.ru/search?text=Goutte&filename= this leaves:

• authorization_code http://grep.xnddx.ru/node/30850213
• autologout http://grep.xnddx.ru/node/30850540
• link preview http://grep.xnddx.ru/node/30945625
• lti_tool_provider http://grep.xnddx.ru/node/30947615
• redirect http://grep.xnddx.ru/node/30987088
• search_api_autocomplete http://grep.xnddx.ru/node/30994581
• stratoserp http://grep.xnddx.ru/node/31010470
• tmgmt_smartling http://grep.xnddx.ru/node/31017900

As the affected modules...

This will affect DTT as well so tagging.

I've asked @fabpot to consider backporting the PHP 8.0 compatible fix to Goutte v3 - I don't think that means we shouldn't do something like the above but it gives us more time and we can do it in a deprecated way rather than a breaking change in a minor version. Note in the original github issue @fabpot said:

I can do changes on the latest 3.3 version if that could help; I just need to know what does not work right now to evaluate the amount of work.

https://github.com/FriendsOfPHP/Goutte/issues/429#issuecomment-707761283

So it sounds like it might be possible.

https://github.com/FriendsOfPHP/Goutte/releases/tag/v3.3.1 is out

The issue still makes sense to get rid of deprecated dependency

I think now this issue can be used to prep the ground to remove the dependency on goutte & gouttedriver in Drupal 10.

Changing parent

Given that Goutte and therefore GoutteDriver are only "supported" in quotes ( https://github.com/FriendsOfPHP/Goutte/issues/429#issuecomment-720059965 ) we still need to move off it. Here's a patch for Drupal 9 that allows us to move forward whilst still supporting tests using GoutteDriver (and therefore Goutte) - the listed projects in #52 and Drupal Test Traits (#53) won't break hard in D9 but they might still need tweaks to keep exactly the same functionality.

Slightly more test coverage.

Fixing the tests. The mocking being done in \Drupal\Tests\Core\Test\BrowserTestBaseTest::mockBrowserTestBaseWithDriver() is being unnecessarily restrictive. $this->once() is unnecessary - on BrowserTestBase this method will alway return the given object and there is no need to unit test the number of calls to these methods here.

Also we can add back in the test coverage for getting the guzzle client from the Goutte driver since we still support doing that.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -220,8 +221,11 @@ abstract class BrowserTestBase extends TestCase {
     if ($driver instanceof GoutteDriver) {
+      @trigger_error('Using \Behat\Mink\Driver\GoutteDriver is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. The dependencies behat/mink-goutte-driver and fabpot/goutte will be removed. See https://www.drupal.org/node/3177235', E_USER_DEPRECATED);
+    }
+
+    if ($driver instanceof BrowserKitDriver) {

Do we need to keep the behaviour inside the second if statement for GoutteDriver as well?

No, because GoutteDriver extends BrowserKitDriver anyway.

Ignore the above, I was wrong - this looks good to me otherwise.

+++ b/core/tests/Drupal/Tests/UiHelperTrait.php
@@ -552,4 +553,22 @@ protected function cssSelect($selector) {
+  /**
+   * Determines if test is using DrupalTestBrowser.
+   *
+   * @return bool
+   *   TRUE if test is using DrupalTestBrowser.
+   */
+  protected function isTestUsingDrupalTestBrowser() {

I think this should have a name like isTestUsingGuzzleClient() because that's what this really is.

Is it backportable to 9.1?

I think #68 is a good suggestion, let's change that.

Spotted and opened while reviewing this: #3181329: BrowserHtmlDebugTrait::getResponseLogHandler() is duplicated as BrowserTestBase::getResponseLogHandler()

Made the changes suggested in #68. Please review.

1. +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
   @@ -559,7 +559,7 @@ protected function cssSelect($selector) {
   -  protected function isTestUsingDrupalTestBrowser() {
   +  protected function isTestUsingGuzzleClient() {
   

   Occurrences of isTestUsingDrupalTestBrowser() should be replaced instead of renaming only.

2. +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
   @@ -552,4 +553,22 @@ protected function cssSelect($selector) {
   diff --git a/interdiff.txt b/interdiff.txt
   
   diff --git a/interdiff.txt b/interdiff.txt
   new file mode 100644
   

   Unexpected interdiff file in patch

#68 was addressed, back to RTBC.

Rerolled

cat composer.lock.rej
diff a/composer.lock b/composer.lock    (rejected hunks)
@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
         "This file is @generated automatically"
     ],
-    "content-hash": "4e5fce0517520b4e4adeb0505f0642b1",
+    "content-hash": "1d1d4008ca3a2889c9d5f021e814c637",
     "packages": [
         {
             "name": "asm89/stack-cors",

Committed/pushed to 9.2.x, thanks!

I think we should try to get this into 9.1.x too, but it'll need a re-roll for composer.lock

Change record is fine now.

Discussed briefly with @alexpott - this is fine in 9.2.x, it's not need for PHP 8 compatibility any more due to the Goutte release which we already updated to, so it's just getting us ready for 10.x at this point

Updated the change record to mention 9.2.0 instead of 9.1.0, the patch was already targeting 9.2.0 so that bit's fine.

Created a new patch in #3104353-31: Upgrade to Guzzle 7 to test Guzzle 7 now that removing this dependency is an option.