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/media(_library)/tests/src/
    • Functional/MediaRequirementsTest.php
    • FunctionalJavascript/ContentModerationTest.php

Issue fork drupal-3439898

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.

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

simonbaese opened merge request !7555

simonbaese’s picture

simonbaese
Location Berlin
commented
Status: Active » Needs review

The ContentModerationTest made a distinction between the user 1 and an admin user. Therefore, all tests for rootUser were simply removed.

smustgrave’s picture

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

Seems like a good refactor.

alexpott’s picture

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

I'm not sure about the changes to core/modules/media_library/tests/src/FunctionalJavascript/ContentModerationTest.php it seems to be testing user 1 specific stuff. I think we need to carefully think about why the user 1 tests are here because we have the admin user here already.

smustgrave’s picture

smustgrave commented

But with the addition of #540008: Add a container parameter that can remove the special behavior of UID#1 should we be testing user1 stuff?

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

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

I asked in Slack, #media, about why the test was specifically testing user #1. Two media maintainers, marcoscano and seanb responded saying that they saw no reason for it. seanb also said that they probably copied the test from somewhere else and then made it work for media.

So, I think it is OK to remove the specific assertions with user #1. Therefor restoring the RTBC.

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

nod_ closed merge request !7555

nod_ reopened merge request !7555

nod_ credited marcoscano.

nod_ credited seanb.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

test failure that seems legit

Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testStandardPerformance
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     ],
     'CacheSetCount' => 45,
     'CacheDeleteCount' => 0,
-    'CacheTagChecksumCount' => 38,
-    'CacheTagIsValidCount' => 43,
+    'CacheTagChecksumCount' => 37,
+    'CacheTagIsValidCount' => 42,
     'CacheTagInvalidationCount' => 0,
     'CacheTagLookupQueryCount' => 21,
     'CacheTagGroupedLookups' => Array &2 [

core/tests/Drupal/Tests/PerformanceTestTrait.php:679
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:175
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:57
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community

appears random #3511123: Remove cache tag checksum assertions from performance tests

nod_ closed merge request !7555

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 0948fc8 and pushed to 11.x. Thanks!

  • nod_ committed 0948fc87 on 11.x 
    Issue #3439898 by simonbaese, quietone, smustgrave, alexpott, marcoscano...
nod_’s picture

nod_
Primary language French
Location Lille
commented

Status: Fixed » Closed (fixed)

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