Contrib.social

Fix System tests that rely on UID1's super user behavior

Open on Drupal.org →
Created on 10 April 2024, 8 months ago
Updated 30 April 2024, 8 months ago

Problem/Motivation

In 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed 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

  1. core/modules/system/tests/src/
    • Functional/Bootstrap/DrupalMessengerServiceTest.php
    • Functional/Entity/EntityReferenceFieldCreationTest.php
    • Functional/File/FileSaveHtaccessLoggingTest.php
    • Functional/Menu/LocalTasksTest.php
    • Functional/Module/ClassLoaderTest.php
    • Functional/Module/GenericModuleTestBase.php
    • Functional/System/DateFormatsLockedTest.php
    • Functional/Theme/MaintenanceThemeUpdateRegistryTest.php
    • Functional/UpdateSystem/UpdateScriptTest.php
    • Kernel/DateFormatAccessControlHandlerTest.php
    • Kernel/Entity/EntityReferenceSelection/EntityReferenceSelectionAccessTest.php
    • Kernel/MenuAccessControlHandlerTest.php
📌 Task
Status

Fixed

Version

10.3 ✨

Component
System  →

Last updated 5 days ago

No maintainer
Created by

🇬🇷Greece vensires

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @vensires
  • Comment 8 months ago →
    🇮🇳India pradhumanjainOSL

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

  • Merge request !7588Issue #3439907: Remove $usesSuperUserAccessPolicy variable. → (Open) created by Unnamed author
  • Pipeline finished with Failed
    8 months ago
    Total: 990s
    #150345
  • First commit to issue fork.
  • Pipeline finished with Failed
    8 months ago
    Total: 178s
    #153162
  • Pipeline finished with Failed
    8 months ago
    #153172
  • Pipeline finished with Failed
    8 months ago
    Total: 1102s
    #153211
  • Pipeline finished with Success
    8 months ago
    Total: 990s
    #153340
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s
  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Left comments on MR.

  • Pipeline finished with Failed
    8 months ago
    Total: 1109s
    #153446
  • Pipeline finished with Success
    8 months ago
    Total: 993s
    #153475
  • Pipeline finished with Success
    8 months ago
    Total: 1698s
    #153898
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s
  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Left more feedback.

  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Also @pradhumanjain2311 why did you just remove $usesSuperUserAccessPolicy but not try and fix any of the tests? Not saving credit for that. Please if going to work on these try and fix them.

  • Pipeline finished with Failed
    8 months ago
    Total: 175s
    #155990
  • Pipeline finished with Success
    8 months ago
    Total: 992s
    #155998
  • Pipeline finished with Failed
    8 months ago
    Total: 1080s
    #156010
  • Pipeline finished with Failed
    8 months ago
    Total: 961s
    #156023
  • Pipeline finished with Success
    8 months ago
    Total: 1909s
    #156033
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s

    comments responded / resolved

  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    The scope of the tickets are to just add permissions for the logged in users.

    Adding additional assertions is out of scope and definitely shouldn’t be updating the performance test as nothing here should impact that.

  • Pipeline finished with Failed
    8 months ago
    Total: 8254s
    #157028
  • Pipeline finished with Canceled
    8 months ago
    Total: 770s
    #157151
  • Pipeline finished with Success
    8 months ago
    Total: 988s
    #157175
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s

    - reverted performance tests. worked now. looks like it was a random error in test the first time
    - removed one extra assert
    - keeping one assertion. If this assert is removed and the permission 'administer account settings', is also removed, the test passes but it would not be correct since the page that opens is access denied which doesn't have local tasks.

  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    A follow up should then be opened for expanding test coverage. Should stick within the scope of the ticket please

  • Pipeline finished with Success
    8 months ago
    Total: 1531s
    #157492
  • Pipeline finished with Success
    8 months ago
    Total: 1022s
    #157668
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s
  • Assigned to smustgrave
  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Will fix later changing to permissionless seems to be changing what's tested.

  • Pipeline finished with Success
    8 months ago
    Total: 1110s
    #157718
  • Issue was unassigned.
  • Status changed to RTBC 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    All I did was some cleanup, still green

  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    Now that user 1 is no longer special we can make the kernel tests much nicer here...

  • Pipeline finished with Success
    8 months ago
    Total: 1041s
    #160329
  • Status changed to RTBC 8 months ago
  • Comment 8 months ago →
    🇮🇳India sukr_s

    suggestions applied

  • Status changed to Fixed 8 months ago
  • Comment 8 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    Committed and pushed 8aeebfb32e to 11.x and 4ed57d454b to 10.3.x. Thanks!

  • Comment 8 months ago →
    System Message
    • alexpott → committed 4ed57d45 on 10.3.x 
      Issue #3439907 by sukr_s, smustgrave, vensires, alexpott: Fix System...
  • Comment 8 months ago →
    System Message
    • alexpott → committed 8aeebfb3 on 11.x 
      Issue #3439907 by sukr_s, smustgrave, vensires, alexpott: Fix System...
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024