Problem/Motivation

The Drupal.toolbar.collapsed is always set taking into account the $base_path, but the test does not take this into account and only checks for /, so if you run the tests in a subdirectory, the testToolbarCollapsedCookie fails.

These are the instances where the cookie can be set:
- toolbar.module

drupal_setcookie('Drupal.toolbar.collapsed', !_toolbar_is_collapsed(),
    array(
      'samesite' => 'Lax',
      'path' => $base_path,
    )
);

- toolbar.js

$.cookie(
    'Drupal.toolbar.collapsed',
    0,
    {
      // Workaround lack of support for the SameSite attribute in jQuery Cookie.
      path: Drupal.settings.basePath + '; SameSite=Lax',
      // The cookie should "never" expire.
      expires: 36500
    }
);

The base path is always set, so the test should match this too.

We are migrating DrupalCI to GitlabCI for Drupal 7, and if this is one of the blockers.

Steps to reproduce

See the failed run here in GitlabCI: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/j...
I added additional output messages here to see what was the reason: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/j...

Proposed resolution

Add the check for the base path in the tests.

Remaining tasks

MR to follow.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Issue fork drupal-3386055

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fjgarlin created an issue. See original summary.

fjgarlin opened merge request !4723

fjgarlin’s picture

fjgarlin commented
Priority: Normal » Critical
Status: Active » Needs review

Note how the issue is fixed here https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/j... after applying the same change that is in the MR.

This is a fix to the test, the code is totally fine.
The MR is here: https://git.drupalcode.org/project/drupal/-/merge_requests/4723/diffs

We've been setting the issues that are blockers for GitlabCI as critical.
Please review.

fjgarlin’s picture

fjgarlin commented
Issue summary: View changes
poker10’s picture

poker10 commented
Status: Needs review » Reviewed & tested by the community

I have checked this and yes, the Drupal.toolbar.collapsed is being set on two places:

1. in toolbar.js (two times)
2. in toolbar_toggle_page

On both places the base_path is used when setting the cookie. So it seems to be correct that the testToolbarCollapsedCookie() should use the base_path instead of hardcoded forward slash as well.

D7 on DrupalCI does not seems to run in a subdirectory (see the snippet from DrupalCI console: You are about to create a /var/www/html/sites/default/settings.php file and DROP all tables in your 'jenkins_drupal_d7_274437' database.), so this is probably the case why this was not failing in tests until now.

The changes looks good to me and passes all tests, so moving to RTBC. Thanks!

mcdruid’s picture

mcdruid commented
Issue tags: +RTBM

This makes sense; I think it's ready to commit. Thanks!

  • poker10 committed 605b36bd on 7.x 
    Issue #3386055 by fjgarlin: Cookie base path not check in the test but...
poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Committed, thanks everyone!

poker10 closed merge request !4723

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.