🇧🇪Belgium @kristiaanvandeneynde

Perhaps ask in #contribute on Slack for some help

Looks better. Might get a merge conflict with the effort in core, though. So setting back to RTBC, but ideally this needs to be targeted vs 11.x and have the latest changes from 11.x merged in (or be rebased onto it)

kristiaanvandeneynde → created an issue.

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

Test coverage looks great, nice job.

Careful when testing with $this->rootUser, as it may no longer be an admin. See 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active for a meta issue where we're removing all use of that in child issues. Setting to NW for that.

Make sure to keep an eye on 📌 Try to provide an upgrade path from 2.x.x to 3.x.x Active

Waiting for 10.3 to come out with the fix to the update.php page, which is now RTBC here and should get committed next week. 🐛 Allow update.php to load when entity type info needs to be updated RTBC

Having said that, still need confirmation from some people who have tried this on their sites.

BramDriesen → credited kristiaanvandeneynde → .

Thanks for the many reviews, much appreciated.

Test fails in the D11 version seem completely unrelated, as if HEAD is broken

Think we have a D11 and D10.3 ready MR now. Changed the order of the optional params so that the deprecation would be nicer for D11 where the optional param was still last.

kristiaanvandeneynde → changed the visibility of the branch 3441222-11-without-deprecations to hidden.

kristiaanvandeneynde → changed the visibility of the branch 10.3.x to hidden.

See #3376101-2: [11.x] [Meta] Tasks to remove Book → for more info

Created 📌 Rename book.memory_cache to cache.book_memory Active as per my comment in #2.

kristiaanvandeneynde → created an issue.

Good changes IMO, thanks.

Asked around on Slack and turns out we don't need two CRs, so I added one here: https://www.drupal.org/node/3445054 →

Will update the code to point to it.

All green now, just need some guidance on #23

I'm guessing this needs a change record for the new setting?

But the real question is, does the deprecation have to point to that CR or the issue here? If all the CR focuses on is the new parameter, then it's kind of moot to point to that CR for new constructor arguments.

Tried to squeeze this in between other tasks. Hopefully the docs are better now. Took care of DI and changed the logic slightly as suggested by @alexpott

UserRolesCacheContext should be fixed in 🐛 UserRolesCacheContext can lead to poisoned cache returns for user 1 Active

Yeah didn't expect this to get set to RTBC without the DI being sorted out :) Currently a bit stretched for time, but will try to prioritize this when I do get some contrib/core time. Thanks for the reviews!

I did have one question, why don't you need to call $this->build(); again after the cache delete with the full module list?

We don't cache the full result in the "light version". So next time someone requests the registry, it will be calculated anew. If that happens outside of the update page, then it will be with all modules and that result will be cached.

Thanks for the review and the fix, I completely overlooked that one. Only thing I can see now that might be holding this back is that we're not injecting 'settings' and 'kernel' via DI.

Still has the random failure and the sniffer failure relating to a mismatching DB version, but many people are reporting that one on Slack. Any idea regarding the random failure?

Looks good to me. Ubernit on a grammar mistake, but that can be applied on commit.

Seeing some random unrelated test fails

This seems to be fully functional now. I added a new setting that you can use to either allow more modules than just system or simply allow ALL modules (old behavior).

Probably needs proper dependency injection for the kernel and settings services.

Looks great to me now. Leaving at NR for:

I think we need to improve this message. Maybe point to a page of Drupal.org about administering your site with no admin role.

But no other objections.

Fine, setting to NR again. From my POV it's +1 now, but @catch was also involved in the discussion so we should perhaps have him take a look at this too.

Alex just informed me there is no cache in the installer so I'm not mega-fussed about the missing cache context. On the one hand it sets a bad example, on the other it's @internal and I don't see us adding caches to the installer soon.

We should probably also document the plans for the future on 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active .

Some things we discussed were to remove the super user from regular Drupal but keep it in installer until we find a way to enable developers to update their code. This could be further exploring the isSyncing() route discussed in #4. Either way, if we commit the MR here we need to make sure it's kept track of in that meta issue.

Well once everything is using recipes they can use standard's admin role recipe for this.

Oh, that would be perfect.

I had a quick review and the cache contexts on the new access policy aren't 100% correct. However, when introducing a new access policy, you need to make sure there are no cache entries, so this wouldn't be an issue. Seeing as after the installer the access policy goes away and caches should be cleared (right?), there is also no risk of polluting the cache with the wrong value.

But from a developer example point of view, we should make sure all of core's access policies show the right cache contexts. Setting to NW for that reason, but could be convinced otherwise.

The tests look really nice too, but I'd stick to one test and use a data provider to set the code, flag and expected results.
E.g.:

$cases['no-super-user-access'] = [
  'install_code' => <<<PHP
      <?php
      function {$this->profile}_install() {
        \$user = \Drupal\user\Entity\User::load(1);
        \Drupal::state()->set('admin_permission_in_installer', \$user->hasPermission('administer software updates'));
      }
      PHP,
  'super-user-policy' => FALSE,
  'expect_user_has_permission_after_install' => FALSE,
  'expect_user_has_permission_during_install' => TRUE,
  'expect_page_shows_no_access_warning' => TRUE,
];

Okay, how about a second container parameter: security.enable_super_user_during_install

So you can keep it on during install, but off after that. We change SuperUserAccessPolicyPass to account for it as such:

  public function process(ContainerBuilder $container): void {
    if ($container->getParameter('drupal.is_installing') === TRUE) {
      $enable_policy = $container->getParameter('security.enable_super_user_during_install');
    }
    else {
      $enable_policy = $container->getParameter('security.enable_super_user');
    }
    if ($enable_policy === FALSE) {
      $container->removeDefinition('access_policy.super_user');
      $container->removeAlias('Drupal\Core\Session\SuperUserAccessPolicy');
    }
  }

Then in core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php, rather than setting security.enable_super_user to TRUE, you set drupal.is_installing to TRUE.

The bonus with this approach is that we could still make it so contrib tests run with security.enable_super_user_during_install disabled for D11, but keep it enabled in D10.3

Having said all of the above, one thing I do very much like about this MR is the removal of the admin role from standard_install() and demo_umami_install() to move it someplace central. All installations should get this admin role out of the box and it should become an opt-out rather than the opt-in it currently is; i.e.: Profiles currently have to remember to ship with an admin role, which feels like poor DX.

Making it so all installations get an admin role would also be a way of somehow keeping the super user behavior during install: Simply assign said admin role to the current user being used for the install, which should almost always be UID 1.

Purely based on this the MR might need work because demo_umami_install() still has the same code as standard_install(). Also, both standard and demo_umami also still have the config entity in their config/install folder, we might need to remove those too then.

Okay so after a lengthy Slack conversation, a few things were brought up:

Summary

Whatever solution we come up with, we know that the super user access policy is one day going away and so will the accompanying security.enable_super_user parameter on the container. So whatever we decide to cook up here should preferably not rely on that parameter because it's more code we later on have to deprecate and/or remove again.

A counterpoint was that the installer will always have to run as a super user but not everyone agreed on that. However, if we do decide to keep that behavior, we could still remove the super user access policy, but move it's logic to an @internal access policy that is only turned on during the installer.

Another disputed point was also made that any content import, whether via default_content, migration, etc.) failing entity validation without user 1 should probably be fixed in the code that handles said import. We can see an example of this in the migrate module's EntityContentBase destination's validateEntity() method, which uses the account switcher before validating content.

Finally, the option was raised that the proper solution could be to set importing content to isSyncing(TRUE) and make sure that any import-supporting code such as the migrate module takes said flag into account. This would take time, tough, so for the time being we'd still need a solution for this issue, even if temporary.

Also keep in mind there still doesn't seem to be a documented consensus on what isSyncing means for content entities, see: 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work

Personal opinion

I still as if we should consider not changing any code right now. We advise people using content_import, migrate et al to not disable the super user access policy during their content import. For D11 we change the default behavior for contrib tests to not run with the super user access policy, which core tests already do, meaning modules such as default_content will now start seeing failing tests.

We then encourage these modules to fix their code and tests in the D11 lifecycle and then we remove the super user access policy altogether in D12, provided we addressed everything in 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active by then.

This way, we don't have to touch the installer at all, we buy at least two more years to work on the isSyncing stuff and contrib gets test fails starting from D11, telling them their code only works because they rely on UID 1.

All in all, I still feel we can one day have a Drupal core where relying on a super user is a thing of the past. Relying on an admin role being perfectly fine, though.

+++ b/core/modules/node/src/Hooks/NodeHooks.php
@@ -0,0 +1,48 @@
+  public function __construct(
+    EntityTypeManagerInterface $entityTypeManager,
+    #[AutowireServiceClosure('module_handler')]
+    protected \Closure $moduleHandler,
+  ) {
+    $this->nodeStorage = $entityTypeManager->getStorage('node');
+  }

Not 100% sure about this autowiring attribute.

I got that you're doing this so we don't have to declare all of our Drupal\MODULE\Hooks classes as services and by using autowiring this way we can still inject dependencies from the container. However, if I understood correctly, AutowireServiceClosure is intended to be used for lazy loading of services or to declare a dependency that may not be available yet when the class is instantiated. But when hooks are invoked, we should have a fuly built container, no?

After all, you're registering the classes on the container with autowire turned on, so doesn't that mean we do not have to manually add the AutowireServiceClosure attribute? Seems like it would beat the whole purpose of autowiring if we still have to specify the attribute.

Aside from the above, I'm loving the approach you're taking here. Nice work.

EntityFieldManager::getFieldMap() still doesn't know about the field_config entities we moved, so that needs fixing.

Looks like it's actually working :o

It was failing on update.php because I forgot to drop tall tables before retrying. Gonna expand the tests now and see if I can make sure they work well too. Had some failures before.

Starting to look really nice, think I just need to get the field instances correct now.

The MR only works in the browser when using the fix from 🐛 Allow update.php to load when entity type info needs to be updated RTBC .

Still need to update all group content types to group relationship types now.

Re #4 it doesn't have to be mutually exclusive. While we technically could babysit users via the UI, we also have to make sure we still have a recovery tool for when someone inevitably breaks their site through code or DB manipulations. This issue is about that recovery mechanism, we could discuss prevention and awareness in another issue.

I like the suggestion in #5 as it's quite simple to implement. Every request or login we check for a flag and if set, we check if the user is still an admin and recover their admin role if not. However, it leaves a lot to be answered:

1. Do we choose the admin role name and machine name for them if it's gone? Or do we ask them to specify?
2. What if they specify a machine name that's already taken, but not an admin role? Do we throw an exception?

Perhaps we need a few settings:

// Set to a user ID to recover the admin role for said user upon login.
$settings['recover_admin_role_for_user'] = FALSE;
// If the administrator role was deleted, it will be recreated with this name and label.
// Throws an exception if the machine name is already taken but the role is not an admin role.
// Only takes effect while $settings['recover_admin_role_for_user'] is not FALSE.
$settings['recover_admin_role_machine_name'] = 'administrator';
$settings['recover_admin_role_label'] = 'Administrator';

Re #8 I am fully onboard with your idea about making Views do less and make it smarter for the things it still does do.

I'd keep the exception rather than a warning because these fields should fail loudly if their schema has invalid pieces. But we need to be able to make the schema valid again, which is currently not possible without my MR or drush updb

Regarding the changes to update.php: It's actually helpful to have a separate kernel for update.php as I could use that to make this MR work.

The current MR allows me to at least visit update.php already. Will check if it works properly.

kristiaanvandeneynde → changed the visibility of the branch 3441222-trying-to-repair to active.

kristiaanvandeneynde → changed the visibility of the branch 3441222-trying-to-repair to hidden.

Here's another WTF caused by Views that I stumbled upon 6 years ago: #2651974-44: field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes →

At this point I'm starting to wonder if we should just take away Views' car keys because it's obviously drunk. It's asking for so much information that, whenever the info it wants hasn't been built yet or is unstable, your system crashes.

I think the first suggestion in the IS is probably the safest to explore, work with an allowlist rather than the inverse. But if we were going to go with a blocklist, we'd basically end up saying "Views bad" as it would be the only thing on that list and we'd run into the same issue again once another core (or contrib) modules starts asking for a lot of information.

For posterity, at the time of writing the first suggestion was:

Limit the hook_theme invocations when using the UpdateKernel to just the bare minimum: system and the active theme for update.php

Regarding the $field_ui_base_route discussion: What if we move those to constants (or properties) with attributes on the entity type's class?

We could declare those as Attribute::TARGET_CLASS_CONSTANT and only look for them in classes that are tagged with the EntityType attribute.

E.g.:

#[ContentEntityType(
  id: 'node',
  label: new TranslatableMarkup('Content'),
  label_collection: new TranslatableMarkup('Content'),
  // ... other stuff but not field_ui_base_route
)]
class Node extends EditorialContentEntityBase implements NodeInterface {

  #[FieldUiBaseRoute]
  public const FIELD_UI_BASE_ROUTE = 'entity.node_type.edit_form';

Then we can do away with putting non-core properties on the EntityType attribute and immediately have a way for other modules to declare their own third-party properties on entity type definitions.

An example of this crash can be seen here: https://git.drupalcode.org/project/group/-/jobs/1337503#L914

kristiaanvandeneynde → created an issue.

  /**
   * Gets the active domain.
   *
   * This method should be called by external classes using the negotiator
   * service.
   *
   * @param bool $reset
   *   Reset the internal cache of the active domain.
   *
   * @return \Drupal\domain\DomainInterface
   *   The active domain object.
   */
  public function getActiveDomain($reset = FALSE);

It can never be empty(), phpstan is right here :)

tim-diels → credited kristiaanvandeneynde → .

Imagine you have a module that checks for a permission to "manage the webshop", you have the following:

• A button to open or close the shop
• A catalog where you can edit various products
• A page where you can adjust the filters of the webshop
• ...

All of these check for the permission "manage the webshop", whether via route access, permission checks in controllers, entity access, ...

Good luck trying to adjust all of that logic to account for the scenario: "Managers may not change the webshop during the weekend if they're in their trial period". You'd have to copy the same logic to all these different places.

With access policies, you'd simply write one access policy that revokes the "manage the webshop" permission under these conditions and all of these access checks across your website would instantly pick up on the change.

Added a fixture, but could not get a garbled DB table name in it (longer than 48 characters becomes a hash), sadly enough. We'll have to make sure we call DefaultTableMapping::generateFieldTableName() with the old and new field storage to know which tables to rename to what.

kristiaanvandeneynde → created an issue.

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.

Okay so it was a random test failure. RTBC then, but keeping an eye on this random failure.

    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

Not sure how that one is related to the code here, though. Double-checking.

    1)
    Drupal\Tests\file\Functional\DownloadTest::testPrivateFileTransferWithoutPageCache
    Correctly denied access to a file when file_test sets the header to -1.
    Failed asserting that 200 is identical to 403.
    
    /builds/issue/drupal-3439893/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
    /builds/issue/drupal-3439893/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
    /builds/issue/drupal-3439893/core/modules/file/tests/src/Functional/DownloadTest.php:138
    /builds/issue/drupal-3439893/core/modules/file/tests/src/Functional/DownloadTest.php:76
    /builds/issue/drupal-3439893/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

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.

I was with larowlan in #8, meaning I didn't care either way as long as it was clear and consistent. After reading up on #17 I can't but agree that PascalCase seems like the only sensible way forward.

Not seeing much value in adding even more names to the IS so I'll leave that as is.

Awesome, thanks! Updated the IS.

Awesome, thanks! 🎉

Thanks for working on this!

Hahaha I can imagine. 15 years and 375 comments later...

Updated the CR and created a few new issues. All of them are now grouped under the following meta issue: 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active

kristiaanvandeneynde → created an issue. See original summary → .

kristiaanvandeneynde → created an issue.

Implemented suggestion verbatim bar the negation, which was needed. Tests go green on CI, confirmed that core tests run correctly locally. So back to RTBC I suppose.

Done, see 📌 Decide on when/how we will run contrib/custom tests without the super user access policy Active

kristiaanvandeneynde → created an issue.

Switched to an event subscriber. left a bunch of @todo comments.

• We might want to cache the outcome of the event.
• The entity type one is still hard-coded but should be dynamic
• This might need tests

Reason I'm using:

    calls:
      - [setEventDispatcher, ['@event_dispatcher']]

Is because if I tried to add it as a proper argument Drupal wouldn't even boot because DrupalKernel::$defaultBootstrapContainerDefinition contains cache_tags_provider.container and at that point we do not have a container yet, so event_dispatcher doesn't resolve. Trying to add 'event_dispatcher' to that array just caused more errors where the autowired 'session' closure wasn't being passed in. Using a setter made all of these issues go away.

Perhaps we can group the list in the IS by module/subsystem, updated as such.

block.module:
comment.module: 1 test
config.module: 2 tests
content_moderation.module:

Re #367 I actually did that to get the list of tests for the meta issue but forgot to commit :)

kristiaanvandeneynde → changed the visibility of the branch 540008-remove-the-special to hidden.

Updated the CR: https://www.drupal.org/node/2910500 →

As far as I am concerned this issue is now "done". It:

• Allows people to turn off the super user on their website
• Turns off the super user for all browser, kernel and nightwatch tests unless opted out
• Opts out all failing core tests, so they can be taken care of in the followup meta issue

The simplicity of turning off UID1 now compared to the old patches/MR is a testament to how powerful the new Access Policy API is.

Followup created and added as an @see to all @todos: 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active

kristiaanvandeneynde → created an issue.

All green now, will leave the Nightwatch stuff to the JavaScript experts in a dedicated followup.

Hmm, so I figured out where Nightwatch is going wrong but I'm not sure I can fix it. I tried adding an extra argument to TestSiteUserLoginCommand as a stopgap, but that's not being picked up it seems.

Okay, found the culprit:

exports.command = function drupalLoginAsAdmin(callback) {
  const self = this;
  this.drupalUserIsLoggedIn((sessionExists) => {
    if (sessionExists) {
      this.drupalLogout();
    }
    const userLink = execSync(
      commandAsWebserver(
        `php ./scripts/test-site.php user-login 1 --site-path ${this.globals.drupalSitePath}`,
      ),
    );

Ends up calling TestSiteUserLoginCommand, which is intended for logging in a user, not logging in an admin user. So this part of every Nighwatch test is relying on UID 1 being an admin, which it no longer is unless TestSiteApplication uses the standard or umami_demo profile, where user 1 is assigned the admin role.

So let's see if we can either assign UID 1 the admin role or rethink whether we should use drupalLoginAsAdmin() at all. Perhaps that rethinking can be done in a follow-up.

Think the nightwatch tests are failing because of this:

exports.command = function drupalInstallModule(module, force, callback) {
  const self = this;
  this.drupalLoginAsAdmin(() => {
    this.drupalRelativeURL('/admin/modules')

I've never worked with Nightwatch tests before so trying to figure out how to get to the PHP version of drupalLoginAsAdmin

There are 8 files actually being changed, meaning we have found 71 broken tests so far. That's going to be a tall followup list in the meta issue.

So currently these tests only go green because they accidentally test with user 1. The changes I made makes these tests go red, but if you add the lines:

  /**
   * {@inheritdoc}
   *
   * @todo Remove and fix test to not rely on super user.
   */
  protected $usesSuperUserAccessPolicy = TRUE;

The super user access policy is turned on for that test again, making it go green once again.

This enables us to commit the changes here without bloating the MR with dozens of test changes. It's already at 40 files changed and I have yet to change even more Functional tests (but ran out of time Friday evening). I'll get to that on Tuesday probably or later this week.

Then, we indeed need a meta issue to track all of the needed changes to these tests, probably one issue per test (hopefully tagged as Novice).

Agreed. Code looks good to me now, so RTBC.

I submitted a session on this subject, would be cool if it got accepted.

Aha, now we're seeing progress. The browser test fails I was expecting are coming through in droves