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/comment/tests/src/
    • Functional/CommentStatisticsTest.php

Issue fork drupal-3439830

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.

vensires’s picture

vensires
Location Greece
commented
Title: Fix all tests that rely on UID1's super user behavior - Comments » Fix Comments tests that rely on UID1's super user behavior

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

thebumik opened merge request !7423

thebumik’s picture

thebumik commented
Status: Active » Needs review

Would greatly appreciate it if someone could review my pull request for this. Any feedback would be invaluable!

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Webuser2 has a duplicate permission, can you clean that up please?

thebumik’s picture

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

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Small nitpick: I would keep the order of the user properties like it was. Now you have $webUser2 first with a comment calling it "a secondary user" before you even configured your primary user. It's a bit confusing in this order.

Suggestion: $adminUser above $webUser2.

smustgrave’s picture

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

Seems feedback has been addressed and think these permissions should be good

larowlan’s picture

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

Left a minor comment on the MR, fine to self RTBC once fixed

thebumik’s picture

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

Applied required changes. Should be fine now.

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

alexpott’s picture

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

I've added some comments to the MR... I don't think we should be overwriting the adminUser with a new user.

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

thebumik’s picture

thebumik commented
Status: Needs work » Needs review

MR comments were addressed.

smustgrave’s picture

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

Not a huge fan of "more_admin_role" but it's a test so not a big deal to me.

alexpott’s picture

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

Committed and pushed 2b0321c170 to 11.x and d35edb34b2 to 10.3.x. Thanks!

Made the following changes on commit

diff --git a/core/modules/comment/tests/src/Functional/CommentStatisticsTest.php b/core/modules/comment/tests/src/Functional/CommentStatisticsTest.php
index a8d5909e68..9a309f16e0 100644
--- a/core/modules/comment/tests/src/Functional/CommentStatisticsTest.php
+++ b/core/modules/comment/tests/src/Functional/CommentStatisticsTest.php
@@ -7,7 +7,6 @@
 use Drupal\comment\CommentInterface;
 use Drupal\comment\CommentManagerInterface;
 use Drupal\comment\Entity\Comment;
-use Drupal\user\UserInterface;
 
 /**
  * Tests comment statistics on nodes.
@@ -21,7 +20,7 @@ class CommentStatisticsTest extends CommentTestBase {
    *
    * @var \Drupal\user\UserInterface
    */
-  protected UserInterface $webUser2;
+  protected $webUser2;
 
   /**
    * {@inheritdoc}
@@ -35,12 +34,11 @@ protected function setUp(): void {
     parent::setUp();
 
     // Add more permissions the admin user.
-    $more_admin_role = $this->drupalCreateRole([
+    $this->adminUser->addRole($this->drupalCreateRole([
       'administer permissions',
       'access administration pages',
       'administer site configuration',
-    ]);
-    $this->adminUser->addRole($more_admin_role)->save();
+    ]))->save();
     // Create a second user to post comments.
     $this->webUser2 = $this->drupalCreateUser([
       'post comments',

Removed the unnecessary variable and reverted out of scope changes.

  • alexpott committed d35edb34 on 10.3.x 
    Issue #3439830 by thebumik, alexpott, pradhumanjain2311, vensires,...

  • alexpott committed e4e23732 on 11.x 
    Issue #3439830 by thebumik, alexpott, pradhumanjain2311, vensires,...

Status: Fixed » Closed (fixed)

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