Use a memory cache in EntityAccessControlHandler

Created on 15 October 2024, about 1 month ago

Problem/Motivation

EntityAccessControlHandler uses an internal (non-static) cache. This cache does not respect cacheability metadata, sometimes leading to incorrect results especially in tests.

Proposed resolution

Use a memory cache. This will ensure proper caching of access results from both hooks and the handler.

Remaining tasks

Implement.

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

📌 Task
Status

Active

Version

11.0 🔥

Component

entity system

Created by

🇬🇧United Kingdom jonathanshaw Stroud, UK

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

Comments & Activities

  • Issue created by @jonathanshaw
  • 🇬🇧United Kingdom jonathanshaw Stroud, UK
  • 🇬🇧United Kingdom jonathanshaw Stroud, UK
  • 🇬🇧United Kingdom jonathanshaw Stroud, UK
  • 🇫🇷France abdei Paris, Ile de France

    Hello,
    I've created an initial patch for this issue, but please note that it's currently missing the required test coverage. I'm actively working on writing the tests and will update the patch once they are ready.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Great!

    1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
      @@ -14,11 +15,11 @@
      +   * @var \Drupal\Core\Cache\MemoryCache\MemoryCache
      
      @@ -53,12 +54,13 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce
      +    $this->memoryCache = new MemoryCache();
      

      I think we need a service for this. See https://www.drupal.org/node/3409455 for the correct pattern. Without that the cache won't get properly invalidated when dependencies are updated.

      The same service is shared between access handlers, which will make resetCache unecessarily aggressive but that doesn't seem like a big problem.

    2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
      @@ -76,15 +78,13 @@ public function access(EntityInterface $entity, $operation, ?AccountInterface $a
             $cid .= ':' . $entity->getRevisionId();
      

      I'm not very comfortable with this combination of creating part of the cache key here and part in the helper method. I think it might be best to pass the entity to the helper method and build the whole cache key there, with conditional logic for 'create' bundle where necessary.

    3. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
      @@ -76,15 +78,13 @@ public function access(EntityInterface $entity, $operation, ?AccountInterface $a
      -      // It is not possible to delete or revert the default revision.
      

      Looks like an accidental removal.

    4. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
      @@ -221,17 +223,36 @@ protected function setCache($access, $cid, $operation, $langcode, AccountInterfa
      +  protected function generateCacheKey($cid, $operation, $langcode, AccountInterface $account) {
      

      Please pass the full $context array to generaateCacheKey(). This will help other issues that want to use that in the cache key.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Also, Gitlab MRs are preferred - maybe even required - rather than patches now.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK
  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    On reflection, I think this will be better with 2 helper methods, one for access and one for createAccess. The createAccess one will get enhanced in a follow up issue to better handle $context.

  • 🇳🇿New Zealand quietone

    Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Production build 0.71.5 2024