user_modules_installed calls $role->save() and PermissionsHandlerInterface::getPermissions 38x in standard install

Looks good. Similarly, a huge offender is our compile+dump of the container for each module that gets enabled. Needs a different solution though.

    // Some permissions call the url generator, so ensure that the routes are
    // up to date.
    \Drupal::service('router.builder_indicator')->setRebuildNeeded();
 

If this runs end of request, we should should be able to remove this too - looks like a bad workaround for a race condition on install - I'd probably have just said no links allowed in permissions descriptions tbh.

Looks great.

I wonder if we can get rid of user_modules_installed() altogether, listening for a configuration system event instead that indicates the core.extension config was modified?

1. +++ b/core/modules/user/src/EventSubscriber/AdminPermissionsRebuilder.php
   @@ -0,0 +1,117 @@
   +  protected $rebuildNeeded;
   

   Should default to FALSE.

2. +++ b/core/modules/user/src/Tests/UserPermissionsTest.php
   @@ -89,7 +89,12 @@ function testAdministratorRole() {
   -    $this->assertTrue($this->admin_user->hasPermission('administer news feeds'), 'The permission was automatically assigned to the administrator role');
   

   The message is no longer there, which is fine, but it should be converted into a comment to allow people in the future to understand more easily what exactly this is testing.

Confirmed #6 is correct - see test run on https://www.drupal.org/node/2396325

I think the call was necessary before we made ModuleInstaller::install() automatically call rebuildIfNeeded(). Agreed that it can be removed.

Did you test this with drush or with the web installer? Because in the web installer, this will still run once per batch page.

Also, the comparison isn't really fair. You also need to include the time the service now takes :)

And last, if most of the time spent in there is inside YamlDiscovery, then most of that will go away with #2395143: YAML parsing is very slow, cache it with FileCache.

Help me out here, why don't we just use $permission_handler->moduleProvidesPermissions() with the new installed $modules?
This would allows us to just scan the modules, which are actually installed.

Yes, except I'm pretty sure that event fires 38 times too - so we'd still need the terminate event.

Of course!

@larowlan
Is there a specific reason why we can't the internal implementation of PermissionHandler to perform better for cases when moduleProvidesPermissions is called? I can't see a reason why it shouldn't be possible.

1. +++ b/core/modules/user/src/EventSubscriber/AdminPermissionsRebuilder.php
   @@ -0,0 +1,106 @@
   +  protected $userPermissions;
   

   Nitpick, but if that is a PermissionHandler, why not $permissionHandler ?
   Current name sounds like it stores a list of permissions for some user.

2. +++ b/core/modules/user/src/EventSubscriber/AdminPermissionsRebuilder.php
   @@ -0,0 +1,106 @@
   +    if ($this->rebuildNeeded && $this->userPermissions && $this->role) {
   ...
   +          $this->role->grantPermission($permission);
   ...
   +        $this->role->save();
   

   $this->role ? isn't that rather $this->adminRole ?
   Surprised this is green :-)

i am really suprised that not a single test relies upon that the admin role is actually directly set. Note: for a simpletest the destruct() will never be called.

+++ b/core/modules/user/user.services.yml
@@ -64,6 +64,11 @@ services:
+  user.admin_permissions_rebuilder:
+    class: Drupal\user\EventSubscriber\AdminPermissionsRebuilder
+    arguments: ['@user.permissions']
+    tags:
+      - { name: event_subscriber' }

Just add a needs_destruction tag as well as implement the DestructableInterface
With that you could skip implement the event subscriber again. Maybe there is a reason why you haven't implemented it.

Just wondering - #2230637: Create a Language field widget and the related formatter just added the following in language_modules_installed($modules) :

if (in_array('language', $modules)) {
  // Load and resave all existing 'entity_view_display'
  // & 'entity_form_display' config entities.
}

Does it mean it will need similar treatment, or is it OK because it's only done if 'language' is one of the modules enabled ?

#33: Can you elaborate, you say this should use DestructableInterface instead and not be an event subscriber?

Are there other examples using DestructableInterface for end-of-request building? e.g. router builder?

CNW based on #33 to get some progress here.

@36

Are there other examples using DestructableInterface for end-of-request building? e.g. router builder?

Note: DestructableInterface is simply a wrapper around KernelEvents::TERMINATE with some nicer API, see KernelDestructionSubscriber

This will not save any time nor memory now that standard profile installs the admin_role setting, so this is only called after standard profile is installed.

It would also not save any time for non-drush applications as module installation is per batch anyway, so while I liked the idea this just introduces complexity for no win.

- The call to ->setRebuildNeeded() should be removed generically though. ModuleInstaller already does that.
- The call to ->getPermissions in standard.profile should be removed.

But the 37 calls will only do a Config::get by now (to check the admin_role setting which takes 0.5 ms) and is fine.

What _could_ be done here (though its a little inconvenient) is to only add permissions of the newly added modules, but then standard.profile would again need to install all permissions, so this would again not save any time in the installer.

And the biggest 2.5 s and costly (10 MB) part of this is not the role YAML parsing, but the route rebuilding of the filter permissions, which is then saved for the next request.

Putting to active as the patch is (outdated).

I guess we can mark this issue as duplicate of #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed

Closed as duplicate of #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed.

Thanks for the work that did go into this issue @all!