Add default cookies in tests using a Guzzle handler

Not sure how to best trigger but still a valid task?

Yes it is still a valid task. Testing this involves ensure debug still works in tests.

The same patch still applies to 10.1.x and passes current coding standards :)

Gotcha can you update #2 for D10?

Not sure how that survived the review bot

Actually this patch allows us to remove some code duplication FTW.

Less code and we can remove even more in Drupal 11. Yay.

And any contrib test that uses the test http client to make requests will send xdebug cookie with needing to copy what Rest and JsonApi did. Nice.

Let's fix PHPStan FWIW...

Doh! Partially phpstan scans-- & alexpott--

The last submitted patch, 20: 3029750-2-20.patch, failed testing. View results →

Well fixing #20 was fun. Turns out our cookie handling in tests is all over the place and ResourceTestBase::decorateWithXdebugCookie() is extraordinarily badly named. This method is responsible for ensuring that \Drupal\Tests\rest\Functional\CookieResourceTestTrait works as expected.

Ended up having to consolidate all the cookie merging into one place - which I think will be really helpful if we ever have to go back down the rabbit hole.

Going to add some more tests as I've added some already but we can add more to ensure the behaviour is as expected.

Moar tests.

The last submitted patch, 22: 3029750-2-22.patch, failed testing. View results →

Interesting... I need to redo the issue summary here as this has become about much more than the xdebug cookie but how cookies are handled in BrowserTestBase tests. We can get rid of a tonne of calls to \Drupal\Tests\BrowserTestBase::getSessionCookies() - with this change it is just not necessary to use this method to get the cookies from login and use them to make a direct request using the Guzzle client. I'm pretty sure this will save contrib and custom test writers a tonne of pain as now you can do:

  $user = $this->drupalCreateUser();
  $this->drupalLogin($user);
  $client = $this->getHttpClient();
  $client->post(...

And the post request will be as a logged in user. Whereas ATM you'd need to copy the cookies around yourself.

The last submitted patch, 23: 3029750-2-23.patch, failed testing. View results →

Small improvement / bug fix.

Add to my list to review again. Glad you were able to pick up your train of thought from 4 years ago impressive!

Besides code review any suggestions on other things to test?

@smustgrave the big thing to test here is that you can still use Xdebug in BrowserTestBase tests and halt execution in the site under test. The easiest way to test this is set a breakpoint in index.php and run ./vendor/bin/phpunit -v core/tests/Drupal/FunctionalTests/BrowserTestBaseCookieTest.php (you might need to point that at a valid phpunit.xml config file). Note try it without the patch to make sure your debugging environment is set up correctly. The difference with the patch applied is execution will halt on $client->request() as well as $this->drupalGet(). HEAD will only break on requests from $this->drupalGet(). Note that this patch finally adds test coverage of setting the XDebug cookie so we kinda know that this works and will know if we break it in the future :)

+++ b/core/tests/Drupal/Tests/DrupalTestBrowser.php
@@ -239,4 +236,65 @@ protected function createResponse(ResponseInterface $response) {
+    $cookies = static::extractCookiesFromRequest(\Drupal::request());

+++ b/core/tests/Drupal/Tests/XdebugRequestTrait.php
@@ -23,7 +23,7 @@ trait XdebugRequestTrait {
-  protected function extractCookiesFromRequest(Request $request) {
+  protected static function extractCookiesFromRequest(Request $request) {

This bit bugs me for a couple of reasons - using \Drupal and the change of the function to static.

In fact we don't need to do this. The reason we use the request here is because we used to support running tests from the browser and so had to copy the xdebug cookie from there but since simpletest left core and never got a Drupal 9 release we really don't have to think about this anymore.

Addressing my code concerns outlined in #29.

Updating the CR for #30 made me realise we should use a better name for the new static method.

So after applying patch #31 I can confirm I can still use xdebug and breakpoints stop.

I'm using ddev v1.21.3 integrated with phpstorm 2022.3.1 where I have my debugger setup.

Placed a breakpoint in index.php and loaded a random page and it stopped correctly as before.

The code from what I can see looks good and see the change record has been updated.

Probably one of the more complex tickets I've looked at so this was fun!

I think the ddev default config for xdebug means that it's either always on or always off, so I'm not sure that proves that this is working. I think you need to modify the config so start with request is set to trigger. See https://github.com/drud/ddev/issues/3731 for some discussion on this and how to enable it.

I toggled if off/on

It does start as off by default but they have built in commands to toggle

It's still not quite the same. In trigger mode it will try and detect the cookie or environment variable. But running ddev xdebug on does not use trigger mode, unless you've overridden php config. Sorry I can give you more details but I'm afk at the moment.

Specifically I think following this comment should be adequate.

So I deleted my ddev container
Added a php.ini file with

[php]
xdebug.start_with_request=trigger

Rebuilt the container with xdebug on.

I'm still able to use breakpoints.

Can do additional testing if needed.

That's great. The only other thing I would do to confirm this is to comment out this line and ensure that breakpoints are no longer reached.

$handler_stack->before('cookies', DrupalTestBrowser::copyCookiesGuzzleMiddleware($this));

So I commented out that line.
Restarted ddev
Cleared cache

But the breakpoint is still stopped

FYI putting the breakpoint on index.php $kernel->terminate($request, $response);

Tested locally with xdebug.start_with_request=trigger, results below.

Test: \Drupal\Tests\file\Functional\FileFieldWidgetTest::testWidgetElement
Chose this as it makes a call to \GuzzleHttp\Client::request directly. Added a breakpoint in this method, and another in \Drupal\file\Controller\FileWidgetAjaxController::progress

Before patch - ✅ test breakpoint reached, ❌ controller breakpoint not reached.
After patch - ✅ test breakpoint reached, ✅ controller breakpoint reached.

Test: \Drupal\Tests\node\Functional\PageViewTest::testPageView
This one just does a simple drupalGet call. Added a breakpoint in this method and another in \Drupal\node\NodeForm::form. Both breakpoints reached before and after patch, which is expected.

RTBC+1

The last submitted patch, 31: 3029750-2-31.patch, failed testing. View results →

Random test faiure due to JS tests.

The last submitted patch, 31: 3029750-2-31.patch, failed testing. View results →

1. +++ b/core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php
   @@ -43,7 +43,6 @@ protected function renderNewCommentsNodeLinks(array $node_ids) {
   -      'cookies' => $this->getSessionCookies(),
   

   Lovely to no longer see this sprinkled everywhere!

   The implementation of the called method was added in 2018's #2983504: Add a way to easily set the cookies in a request done using the Guzzle client → .

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -23,6 +22,9 @@
   +  use ApiRequestTrait {
   +    makeApiRequest as request;
   +  }
   

   Yay for less duplication 👍

   2022's #3227824: Move the linkset functionality from the decoupled menus contributed module to core's system module → (by the decoupled menus team!) added ApiRequestTrait, which was lifted out of 2019's JsonApiRequestTestTrait #2843147: Add JSON:API to core as a stable module → (by yours truly).

   That explains why this was not yet being used by ResourceTestBase: that base test class is used by the REST module, and it was the later addition of the JSON:API module that introduced the more general helper.

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -323,36 +325,6 @@ protected function grantPermissionsToTestedRole(array $permissions) {
   -  protected function request($method, Url $url, array $request_options) {
   -    $request_options[RequestOptions::HTTP_ERRORS] = FALSE;
   -    $request_options[RequestOptions::ALLOW_REDIRECTS] = FALSE;
   -    $request_options = $this->decorateWithXdebugCookie($request_options);
   -    $client = $this->getHttpClient();
   -    return $client->request($method, $url->setAbsolute(TRUE)->toString(), $request_options);
   -  }
   

   Written in 2016's #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method → by yours truly. Good riddance! 👍

4. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -456,32 +428,6 @@ protected function assertResourceErrorResponse($expected_status_code, $expected_
   -  protected function decorateWithXdebugCookie(array $request_options) {
   -    $session = $this->getSession();
   -    $driver = $session->getDriver();
   -    if ($driver instanceof BrowserKitDriver) {
   -      $client = $driver->getClient();
   -      foreach ($client->getCookieJar()->all() as $cookie) {
   -        if (isset($request_options[RequestOptions::HEADERS]['Cookie'])) {
   -          $request_options[RequestOptions::HEADERS]['Cookie'] .= '; ' . $cookie->getName() . '=' . $cookie->getValue();
   -        }
   -        else {
   -          $request_options[RequestOptions::HEADERS]['Cookie'] = $cookie->getName() . '=' . $cookie->getValue();
   -        }
   -      }
   -    }
   -    return $request_options;
   -  }
   

   Introduced by 2017's #2851746: Support xdebug header in ResourceTestBase and move htttpClient property to right place → by yours truly. Good riddance! 👍

5. +++ b/core/tests/Drupal/Tests/Core/Test/XdebugRequestTraitTest.php
   @@ -16,6 +16,42 @@ class XdebugRequestTraitTest extends UnitTestCase {
   +  public function extractServerDataProvider() {
   +    yield 'no XDEBUG_CONFIG' => [[], NULL];
   +    yield 'empty string XDEBUG_CONFIG' => [['XDEBUG_CONFIG' => ''], NULL];
   +    yield 'only space string XDEBUG_CONFIG' => [['XDEBUG_CONFIG' => ' '], NULL];
   +    yield 'invalid XDEBUG_CONFIG' => [['XDEBUG_CONFIG' => 'invalid_config'], NULL];
   +    yield 'idekey XDEBUG_CONFIG' => [
   +      ['XDEBUG_CONFIG' => 'idekey=XDEBUG_KEY'],
   +      'XDEBUG_KEY',
   +    ];
   +    yield 'idekey with another key XDEBUG_CONFIG' => [
   +      ['XDEBUG_CONFIG' => 'foo=bar  idekey=XDEBUG_KEY '],
   +      'XDEBUG_KEY',
   +    ];
   

   🤩👏

6. +++ b/core/tests/Drupal/Tests/XdebugRequestTrait.php
   @@ -34,20 +40,36 @@ protected function extractCookiesFromRequest(Request $request) {
   +  protected static function extractXdebugIdeKeyFromServer(iterable $server): ?string {
   +    if (isset($server['XDEBUG_CONFIG'])) {
          // $_SERVER['XDEBUG_CONFIG'] has the form "key1=value1 key2=value2 ...".
   -      $pairs = array_filter(explode(' ', $server->get('XDEBUG_CONFIG')), function ($value) {
   +      $pairs = array_filter(explode(' ', $server['XDEBUG_CONFIG']), function ($value) {
   

   👏 for reducing risk by reusing existing logic and adding explicit test coverage for it, that did not have test coverage before!

7. +++ b/core/tests/Drupal/Tests/ApiRequestTrait.php
   @@ -38,11 +38,12 @@ trait ApiRequestTrait {
   -    $request_options = $this->decorateWithXdebugCookie($request_options);
   -    $client = $this->getSession()->getDriver()->getClient()->getClient();
   -    return $client->request($method, $url->setAbsolute(TRUE)->toString(), $request_options);
   +    return $http_client->request($method, $url->setAbsolute()->toString(), $request_options);
   
   +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -230,9 +230,11 @@ protected function initMink() {
          $handler_stack = $client->getConfig('handler');
   +      $handler_stack->before('cookies', DrupalTestBrowser::copyCookiesGuzzleMiddleware($this));
          $handler_stack->push($this->getResponseLogHandler());
    
          $driver->getClient()->setClient($client);
   @@ -249,16 +251,6 @@ protected function initMink() {
   
   @@ -249,16 +251,6 @@ protected function initMink() {
    
        $this->initFrontPage();
    
   -    // Copies cookies from the current environment, for example, XDEBUG_SESSION
   -    // in order to support Xdebug.
   -    // @see BrowserTestBase::initFrontPage()
   -    $cookies = $this->extractCookiesFromRequest(\Drupal::request());
   -    foreach ($cookies as $cookie_name => $values) {
   -      foreach ($values as $value) {
   -        $session->setCookie($cookie_name, $value);
   -      }
   -    }
   -
   
   +++ b/core/tests/Drupal/Tests/DrupalTestBrowser.php
   @@ -98,13 +101,7 @@ protected function doRequest($request): object {
   -    $cookies = CookieJar::fromArray(
   -      $this->getCookieJar()->allRawValues($request->getUri()),
   -      parse_url($request->getUri(), PHP_URL_HOST)
   -    );
   

   Simplification in so many places! Less opportunity for mistakes 👍

Also: the diffstat is +299, -119, but ~180 of the additions are for new test coverage and another ~20 for the deprecations.

Looking at the actual code changes, there's a net decrease in code, thanks to this now all being handled in a single consistent way.

Confirming RTBC. #31 still applies cleanly.

The last submitted patch, 31: 3029750-2-31.patch, failed testing. View results →