Static cache permissions

But it is also not called a lot.

Previously, we had static caching in Simpletest and it was extremely annoying, because when you added things like node types or filters, or something else that dynamically creates permissions, you had to call a weird, magic simpletest method to clear that cache.

It might be a bit better if it's part of the official API, but still there is a reason we do not do static caching at the moment.

Updated issue summary, added beta template

Here's a patch

one day I'll remember to run unit tests first

+++ b/core/modules/user/src/PermissionHandler.php
@@ -94,32 +101,49 @@ public function getPermissions() {
+   *   permissions for all modules.
+   * @return \array[] Each return permission is an array with the following keys:
+   * Each return permission is an array with the following keys:

Blank comment line missing between @param and @return also desc of @return is not correctly indent.

1. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -51,6 +51,13 @@ class PermissionHandler implements PermissionHandlerInterface {
   +  protected $permisisonsCache;
   

   typo - permisisonsCache

   its used consistently though ...

2. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -94,32 +101,49 @@ public function getPermissions() {
   +    else {
   +      // Use the default discovery - which handles all enabled modules.
   +      $discovery = $this->getYamlDiscovery();
   +    }
   

   If we want to do that, getYamlDiscovery() should take a modules parameter as well - instead of copying logic directly in here.

As Berdir I am not yet convinced on this, how often is the static cache used anyway?

This needs some up-front profiling, I think.

In general it makes sense, but its in the details ...

Right, but does that actually run multiple times, the admin role does't exist until the profile configuration is installed, which is the last module of an installation?

I'd say lets wait and see if #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed gets in, if so, we can probably close this as won't fix/duplicate?

For user_modules_installed() this would not have helped in the form it is here.

I think that was intended for the admin usage in system_admin_tasks or such ...

But yes, lets wait.

Now that #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed is in, is this still needed? I came across this issue from the TODO in the code that is being worked on over at #2342229: Add permissionByModule to user.permissions service. If this is now resolved, that TODO should be removed.

Patch from #6 still applies. I profiled admin/index with and without the patch:

        | Incl. Wall Time | IWall% | Excl. Wall Time |
Without |        1,892,381|  100.0%|             373 |
With    |         561,759 |  100.0%|             351 |

I think we should do this :)

Sorry, CNW per review from #8

Once again, wondering how much of an improvement remains here when we have the file cache issue in. We need to get that done :)

This should address #8.

We will load the permission list also here #2571235: [regression] Roles should depend on objects that are building the granted permissions. So I think we need not only a static but a persistent cache :(

Is this patch usable for a production site?

Every time i access to extend page i get a timeout. Normally i only comment the lines of permission in ModulesListForm.php (lines 224 - 232).

We have a few cases now where this would be useful, for example the mentioned #2571235: [regression] Roles should depend on objects that are building the granted permissions and also #2934812: Use render caching on permissions UI table that I just opened. And those cases actually need the permissions, not just whether or not a module has permissions.

So lets try to statically cache the actual permissions list. I have an idea on how to solve the testing problem (basically just reset the cache whenever we validate permissions in tests).

Based on some quick profiling, the previous patch was 54% faster than HEAD on admin/index, this one is about 7% faster again but not sure if I trust that number and that would also only be true if moduleProvidesPermissions() is called for most/all installed modules, but if so, then getting the permissions and sorting them just once is a slight advantage. And not calling it multiple times for the same module because I removed the per-module static cache.

One advantage of this is also that it becomes trivial and doesn't require any test or interface additions.

Like this. We could also add an explicit method, but then we need an interface change again.

I think we've done enough profiling?

So a single actual test fail and that's a update test that tests that new permissions are added. I think that's OK to break that?

We should probably add a resetCache() method or something, so that it's possible to clear the static cache explicitly if needed...no?

I thought about that, not sure. That would require an additional method on the interface.

The only use cases to do that are tests IMHO, for that resetting the container might be sufficient?

The only use cases to do that are tests IMHO, for that resetting the container might be sufficient?

I could imagine possible use cases outside of tests, like if your code has installed or uninstalled a module and needs to refresh the set of available permissions for some reason. I think it would be worthwhile to add the method (which is allowed in a minor release as long as there's a base implementation), because without it, any code that finds itself needing to refresh the list of permissions in a non-testing context is up you-know-which creek without a paddle. :)

> like if your code has installed or uninstalled a module and needs to refresh the set of available permissions

1. Installing/uninstalling module creates a new container => new permission handler object, cache is gone

2. We don't need the list of permissions to check if you have a permission, we only need it for validation in tests and some UI elements.. the permission page and the modules/admin index page tocheck if a a module has permissions or not.

I'm not against it, just think that we should have a good reason to extend an interface with a method, even if it's very unlikely that someone has an alternative implementation.

Agree

This should be a permanent cache, not static.

The use case is places in the UI that refer to a permission by its human-readable label, and which currently violate DRY by repeating the label that's defined in permissions.yml.

For example, on the user admin settings page:

      '#description' => $this->t('Users with the %select-cancel-method or %administer-users <a href=":permissions-url">permissions</a> can override this default method.', ['%select-cancel-method' => $this->t('Select method for cancelling account'), '%administer-users' => $this->t('Administer users'), ':permissions-url' => $this->url('user.admin_permissions')]),

(For at least a while, the permission label here didn't match the one in user.permissions.yml! That bug is fixed in 8.8.x at least!)

A persistent cache causes a lot of problems with dynamic permission, I'm not convinced that's worth it because nothing uses permission labels in the critical path and that UI not using the label is a different issue IMHO.

We were getting locked out of the /admin/modules page, and applying #29 on 8.8.6 fixed this for us. We will try the other patches too. Just wanted to bump this a bit, as without this patch, we couldn't enable modules.

Adding patch for 8.9 and 9.1 branch.

I'm currently building a rather large site with quite a lot of modules enabled. With this patch enabled, /admin/modules went from timeout out after 70s to loading in 23s (both with profiling enabled). The profiling led me to this patch via the @TODO comment in moduleProvidesPermissions.

The patch itself looks good to me, but needs some tests and therefore needs work.

Regards,

Nigel

I had a problem with a site that I upgraded from 9.0.6 => 9.3.3 I had not worked on it prior to the project's completion, but before I upgraded I did access the /admin/module page at least once.

After updating, I just got 504 Gateway Timeout, but locally and on the staging server, only when accessing the Module list page. After some hefty debugging, I was able to find a comment that lead me to this page. I applied #47 with composer and now it works just fine. It's still relatively slow, and I'm not sure why this is happening on this project alone, but at least, I can confirm that the patch fixes my error, and from my behalf, it has been tested by the community.

With patch
Drupal 10.1 gets a heap of weird issues on installing modules.
Specifically with Drupal\Component\DependencyInjection\ReverseContainer

Service Container has a null value for generateServiceIdHash.

Running a drush cim that would install a module, occasionally throws a delightfully difficult error to trace.

It's a result of \Drupal::getContainer()->set('user.permissions', NULL);

TypeError: Drupal\Component\DependencyInjection\ReverseContainer::generateServiceIdHash(): Argument #1 ($object) must be of type object, null given in /app/site/web/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php on line 87 #0 [internal function]: Drupal\Component\DependencyInjection\ReverseContainer->generateServiceIdHash(NULL)
#1 /app/site/web/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php(75): array_map(Array, Array)
#2 /app/site/web/core/lib/Drupal/Core/DrupalKernel.php(903): Drupal\Component\DependencyInjection\ReverseContainer->recordContainer()
#3 /app/site/vendor/drush/drush/src/Drupal/DrupalKernelTrait.php(70): Drupal\Core\DrupalKernel->initializeContainer()
#4 /app/site/web/core/lib/Drupal/Core/DrupalKernel.php(816): Drush\Drupal\DrupalKernel->initializeContainer()
#5 /app/site/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php(608): Drupal\Core\DrupalKernel->updateModules(Array, Array)
#6 /app/site/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php(244): Drupal\Core\Extension\ModuleInstaller->updateKernel(Array)
#7 /app/site/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83): Drupal\Core\Extension\ModuleInstaller->install(Array, false)
#8 /app/site/web/core/lib/Drupal/Core/Config/ConfigImporter.php(872): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, false)
#9 /app/site/web/core/lib/Drupal/Core/Config/ConfigImporter.php(624): Drupal\Core\Config\ConfigImporter->processExtension('module', 'install', 'graphql_compose...')
#10 /app/site/web/core/lib/Drupal/Core/Config/ConfigImporter.php(561): Drupal\Core\Config\ConfigImporter->processExtensions(Array)
#11 /app/site/vendor/drush/drush/src/Drupal/Commands/config/ConfigImportCommands.php(300): Drupal\Core\Config\ConfigImporter->doSyncStep('processExtensio...', Array)
#12 /app/site/vendor/drush/drush/includes/drush.inc(122): Drush\Drupal\Commands\config\ConfigImportCommands->doImport(Object(Drupal\Core\Config\StorageComparer))
#13 /app/site/vendor/drush/drush/includes/drush.inc(113): drush_call_user_func_array(Array, Array)
#14 /app/site/vendor/drush/drush/src/Drupal/Commands/config/ConfigImportCommands.php(271): drush_op(Array, Object(Drupal\Core\Config\StorageComparer))
#15 [internal function]: Drush\Drupal\Commands\config\ConfigImportCommands->import(Array)
#16 /app/site/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array(Array, Array)
#17 /app/site/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#18 /app/site/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#19 /app/site/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(391): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#20 /app/site/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#21 /app/site/vendor/symfony/console/Application.php(1081): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#22 /app/site/vendor/symfony/console/Application.php(320): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#23 /app/site/vendor/symfony/console/Application.php(174): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#24 /app/site/vendor/drush/drush/src/Runtime/Runtime.php(124): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#25 /app/site/vendor/drush/drush/src/Runtime/Runtime.php(51): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#26 /app/site/vendor/drush/drush/drush.php(79): Drush\Runtime\Runtime->run(Array)
#27 /app/site/vendor/drush/drush/drush(4): require('/app/site/vendo...')
#28 {main}
TypeError: Drupal\Component\DependencyInjection\ReverseContainer::generateServiceIdHash(): Argument #1 ($object) must be of type object, null given in Drupal\Component\DependencyInjection\ReverseContainer->generateServiceIdHash() (line 87 of /app/site/web/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php).

This shouldn't use a cache property. It should use a memory cache backend that falls back to a permanent cache backend.

Like JSON:API does:

  # Cache.
  cache.jsonapi_memory:
    class: Drupal\Core\Cache\MemoryCache\MemoryCache
    public: false
  cache.jsonapi_resource_types:
    class: Drupal\Core\Cache\BackendChain
    calls:
      - [appendBackend, ['@cache.jsonapi_memory']]
      - [appendBackend, ['@cache.default']]
    tags: [{ name: cache.bin }]

This allows for invalidating the in-memory cache within one request.

Just read down and was thinking the same as #59, and then there was the comment so I didn't have to write it :)

Although given #41 I think we should add the memory backend here and then move persistent caching, if it's needed at all, to a follow-up.

Pushed an MR that adds a MemoryCache.

Lots of test fails for this. I assume we need to work out where the cache needs to be cleared.