πŸ‡§πŸ‡ͺBelgium @kristiaanvandeneynde

Antwerp, Belgium
Account created on 31 May 2011, almost 13 years ago
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

If you are on D10.1 and upgrade VC to the latest version, clearing the container cache (which is probably broken by now) using the query "DELETE FROM cache_container;", your site should work. At that point you should be able to upgrade to D10.2

Downgrading to Major as this issue would have blown up by now if everyone ran into it, so it's probably not happening across the board.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Updated comment to stay below 80 character limit and renamed the CR so that the title reflects the changes properly. Looks great to me now, if I hadn't been so involved myself I'd RTBC.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Already dumping code here for safekeeping, but tests need one minor adjustment. Hoping to tackle that tomorrow.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Re #49 hard-coding Claro for update.php wouldn't necessarily be a bad thing either. We're actually lying right now by saying update.php's theme cannot be changed:

/**
 * A custom theme for the offline page:
 *
 * ...
 *
 * Note: This setting does not apply to installation and update pages.
 */
# $settings['maintenance_theme'] = 'claro';

History of the setting applying to update.php:

  1. #2250119: Run updates in a full environment β†’
  2. #3279640: Standard install profile uses Olivero for update.php β†’
  3. #3304285: Remove Seven from core β†’

Where 1. basically converted the old drupal_maintenance_theme() calls to checking the setting, even though the setting says it does not apply to update.php

The amount of affected sites might go up, though. Then again, it's quite reasonable to argue update.php should be as stable and minimalistic as possible because your website is unstable when you visit it.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

P.S.: Once there is a consensus, I will update the 10.3 branch also.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Pushed a change to the 11.x branch to see what tests have to say.

This does lead to a minor contradiction in a sense that we're introducing a setting that allows you to choose which modules are active on update.php, but then immediately ban you from using any theme that depends on a module. Why even offer the choice at that point?

I'm wondering if we shouldn't just hard-code the module limit to 'system' then instead of providing a setting that allows you to choose which modules remain active. Although that would obviously be a BC break.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I tend to agree with 2 as 3 would put us right back at square one if one of these dependencies turns out to be unstable.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I just tried this by setting Gin as the admin theme and it does not seem to style update.php at all.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Perhaps ask in #contribute on Slack for some help

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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)

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Thanks for the many reviews, much appreciated.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ changed the visibility of the branch 3441222-11-without-deprecations to hidden.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ changed the visibility of the branch 10.3.x to hidden.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Created πŸ“Œ Rename book.memory_cache to cache.book_memory Active as per my comment in #2.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

All green now, just need some guidance on #23

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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!

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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?

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Seeing some random unrelated test fails

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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,
];
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
+++ 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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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';
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ changed the visibility of the branch 3441222-trying-to-repair to active.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ changed the visibility of the branch 3441222-trying-to-repair to hidden.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
  /**
   * 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 :)

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
    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
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium
    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
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Awesome, thanks! Updated the IS.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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:

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ changed the visibility of the branch 540008-remove-the-special to hidden.

Production build 0.69.0 2024