Problem/Motivation

In #540008: Add a container parameter that can remove the special behavior of UID#1 an approach was taken where we can simply flag tests that are failing if we turn off user 1's super user powers, so that they can be taken care of in a followup. This issue is to collect all of these followups.

The goal is to have no tests in Drupal core that rely on UID1's special privileges so that we:

  1. Know these tests are correctly assigning the necessary permissions to run
  2. Can turn off the super user access policy in D11, knowing it won't break core
  3. Can remove the super user access policy in D12, providing an admin account recovery tool to replace it

Steps to reproduce

Go into any of the tests flagged with:

  /**
   * {@inheritdoc}
   *
   * @todo Remove and fix test to not rely on super user.
   * @see https://www.drupal.org/project/drupal/issues/3437620
   */

And:

  1. Remove the code below that sets the usesSuperUserAccessPolicy to TRUE.
  2. Run the test to see which test methods are failing

Proposed resolution

Assign the right permissions to make the test go green without the super user access policy. Those few tests that specifically test said policy can obviously stay, but will be removed along with the policy in D12.

Remaining tasks

  • core/modules/path/tests/src/
    • Functional/PathContentModerationTest.php
CommentFileSizeAuthor
#5 3439904-nr-bot.txt1.49 KBneeds-review-queue-bot

Issue fork drupal-3439904

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

vensires created an issue. See original summary.

thebumik made their first commit to this issue’s fork.

thebumik opened merge request !7429

thebumik’s picture

thebumik commented
Status: Active » Needs review

Fixed path tests. Please review

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3439904-nr-bot.txt1.49 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented 
    There were 2 errors:
    
    1)
    Drupal\Tests\path\Functional\PathContentModerationTest::testNodePathAlias
    Behat\Mink\Exception\ElementNotFoundException: Form field with
    id|name|label|value "path[0][alias]" not found.
    
    /builds/issue/drupal-3439904/vendor/behat/mink/src/WebAssert.php:731
    /builds/issue/drupal-3439904/vendor/behat/mink/src/WebAssert.php:768
    /builds/issue/drupal-3439904/core/tests/Drupal/Tests/WebAssert.php:968
    /builds/issue/drupal-3439904/core/modules/path/tests/src/Functional/PathContentModerationTest.php:111
    /builds/issue/drupal-3439904/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    2)
    Drupal\Tests\path\Functional\PathContentModerationTest::testTranslatedModeratedNodeAlias
    Behat\Mink\Exception\ElementNotFoundException: Button with
    id|name|label|value "Save (this translation)" not found.
    
    /builds/issue/drupal-3439904/core/tests/Drupal/Tests/WebAssert.php:151
    /builds/issue/drupal-3439904/core/tests/Drupal/Tests/UiHelperTrait.php:78
    /builds/issue/drupal-3439904/core/modules/path/tests/src/Functional/PathContentModerationTest.php:207
    /builds/issue/drupal-3439904/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
thebumik’s picture

thebumik commented
Status: Needs work » Needs review
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Left a comment, not sure if we want to switch from relying on UID1 to relying on being able to bypass all node permissions. The goal is that our tests prove that whoever has the right permissions (without super user stuff), can use the website as intended for their role.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment but not sure we need all those modules installed for this test

thebumik’s picture

thebumik commented
Status: Needs work » Needs review
smustgrave’s picture

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

Reviewed the permissions and believe those should be fine, didn't seem to have a crazy one to me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left a comment on the MR

thebumik’s picture

thebumik commented
Status: Needs work » Needs review
smustgrave’s picture

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

Feedback addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Added a couple of comments to the MR. There's no need to add permissions as a class property to the test.

pradhumanjain2311 made their first commit to this issue’s fork.

thebumik’s picture

thebumik commented

Addressed the comments from PR.

thebumik’s picture

thebumik commented
Status: Needs work » Needs review
smustgrave’s picture

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

Thanks for addressing @thebumik.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed eeb7e60c30 to 11.x and 7e4ccefc08 to 10.3.x. Thanks!

  • alexpott committed 7e4ccefc on 10.3.x 
    Issue #3439904 by thebumik, pradhumanjain2311, smustgrave,...

  • alexpott committed eeb7e60c on 11.x 
    Issue #3439904 by thebumik, pradhumanjain2311, smustgrave,...

Status: Fixed » Closed (fixed)

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