Merge pull request #480 from Gizra/invalidate-cache-on-membership-del…

…etion MembershipManager still returns a user's memberships after the user is deleted
Gizra · Apr 25, 2019 · a68f578 · a68f578
2 parents 8e5dd79 + d6baff9
commit a68f578
Show file tree

Hide file tree

Showing 9 changed files with 82 additions and 59 deletions.
diff --git a/og.module b/og.module
@@ -332,7 +332,6 @@ function og_invalidate_group_content_cache_tags(EntityInterface $entity) {
     if ($is_group_content && $original) {
       /** @var \Drupal\og\OgGroupAudienceHelperInterface $group_audience_helper */
       $group_audience_helper = \Drupal::service('og.group_audience_helper');
-      $audience_has_changed = FALSE;
       /** @var \Drupal\Core\Entity\FieldableEntityInterface $original */
       foreach ($group_audience_helper->getAllGroupAudienceFields($entity->getEntityTypeId(), $entity->bundle()) as $field) {
         $field_name = $field->getName();
@@ -342,15 +341,8 @@ function og_invalidate_group_content_cache_tags(EntityInterface $entity) {
           foreach ($original_field_item_list->referencedEntities() as $old_group) {
             $tags = Cache::mergeTags($tags, $old_group->getCacheTagsToInvalidate());
           }
-          $audience_has_changed = TRUE;
         }
       }
-      if ($audience_has_changed) {
-        // If at least a group has changed, we clear the static cache of the
-        // membership manager which stores such relations. This is important
-        // when the membership manager was used previously in the same request.
-        $membership_manager->reset();
-      }
     }
 
     foreach ($membership_manager->getGroups($entity) as $group_entity_type_id => $groups) {

diff --git a/og.services.yml b/og.services.yml
@@ -45,7 +45,7 @@ services:
     arguments: ['@config.factory', '@entity_type.manager', '@entity_type.bundle.info', '@event_dispatcher', '@cache.data', '@og.permission_manager', '@og.role_manager', '@router.builder', '@og.group_audience_helper']
   og.membership_manager:
     class: Drupal\og\MembershipManager
-    arguments: ['@entity_type.manager', '@og.group_audience_helper']
+    arguments: ['@entity_type.manager', '@og.group_audience_helper', '@cache.static']
   og.permission_manager:
     class: Drupal\og\PermissionManager
     arguments: ['@event_dispatcher']

diff --git a/src/Entity/OgMembership.php b/src/Entity/OgMembership.php
@@ -479,9 +479,6 @@ public function save() {
     Og::reset();
     \Drupal::service('og.access')->reset();
 
-    // Invalidate the group membership manager.
-    \Drupal::service('og.membership_manager')->reset();
-
     return $result;
   }
 

diff --git a/src/MembershipManager.php b/src/MembershipManager.php
@@ -3,6 +3,9 @@
 namespace Drupal\og;
 
 use Drupal\Component\Utility\NestedArray;
+use Drupal\Core\Cache\Cache;
+use Drupal\Core\Cache\CacheBackendInterface;
+use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Session\AccountInterface;
@@ -11,16 +14,16 @@
 use Drupal\og\Entity\OgMembership;
 
 /**
- * Membership manager.
+ * Service for managing memberships and group content.
  */
 class MembershipManager implements MembershipManagerInterface {
 
   /**
-   * Static cache of the memberships and group association.
+   * The static cache backend.
    *
-   * @var array
+   * @var \Drupal\Core\Cache\CacheBackendInterface
    */
-  protected $cache;
+  protected $staticCache;
 
   /**
    * The entity type manager.
@@ -43,10 +46,14 @@ class MembershipManager implements MembershipManagerInterface {
    *   The entity type manager.
    * @param \Drupal\og\OgGroupAudienceHelperInterface $group_audience_helper
    *   The OG group audience helper.
+   * @param \Drupal\Core\Cache\CacheBackendInterface $cache
+   *   The static cache backend.
    */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, OgGroupAudienceHelperInterface $group_audience_helper) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, OgGroupAudienceHelperInterface $group_audience_helper, CacheBackendInterface $cache) {
+    assert($cache instanceof CacheTagsInvalidatorInterface, 'The cache backend must support cache tag invalidation.');
     $this->entityTypeManager = $entity_type_manager;
     $this->groupAudienceHelper = $group_audience_helper;
+    $this->staticCache = $cache;
   }
 
   /**
@@ -85,26 +92,26 @@ public function getMemberships(AccountInterface $user, array $states = [OgMember
     // states.
     $states = $this->prepareConditionArray($states, OgMembership::ALL_STATES);
 
-    $identifier = [
+    $cid = [
       __METHOD__,
-      'user',
       $user->id(),
       implode('|', $states),
     ];
-    $identifier = implode(':', $identifier);
+    $cid = implode(':', $cid);
 
     // Use cached result if it exists.
-    if (!isset($this->cache[$identifier])) {
+    if (!$membership_ids = $this->staticCache->get($cid)->data ?? []) {
       $query = $this->entityTypeManager
         ->getStorage('og_membership')
         ->getQuery()
         ->condition('uid', $user->id())
         ->condition('state', $states, 'IN');
 
-      $this->cache[$identifier] = $query->execute();
+      $membership_ids = $query->execute();
+      $this->cacheMembershipIds($cid, $membership_ids);
     }
 
-    return $this->loadMemberships($this->cache[$identifier]);
+    return $this->loadMemberships($membership_ids);
   }
 
   /**
@@ -140,17 +147,17 @@ public function getGroupMembershipIdsByRoleNames(EntityInterface $group, array $
     $role_names = $this->prepareConditionArray($role_names);
     $states = $this->prepareConditionArray($states, OgMembership::ALL_STATES);
 
-    $identifier = [
+    $cid = [
       __METHOD__,
       $group->getEntityTypeId(),
       $group->id(),
       implode('|', $role_names),
       implode('|', $states),
     ];
-    $identifier = implode(':', $identifier);
+    $cid = implode(':', $cid);
 
     // Only query the database if no cached result exists.
-    if (!isset($this->cache[$identifier])) {
+    if (!$membership_ids = $this->staticCache->get($cid)->data ?? []) {
       $entity_type_id = $group->getEntityTypeId();
 
       $query = $this->entityTypeManager
@@ -169,10 +176,11 @@ public function getGroupMembershipIdsByRoleNames(EntityInterface $group, array $
         $query->condition('roles', $role_ids, 'IN');
       }
 
-      $this->cache[$identifier] = $query->execute();
+      $membership_ids = $query->execute();
+      $this->cacheMembershipIds($cid, $membership_ids);
     }
 
-    return $this->cache[$identifier];
+    return $membership_ids;
   }
 
   /**
@@ -206,29 +214,31 @@ public function getGroupIds(EntityInterface $entity, $group_type_id = NULL, $gro
       throw new \InvalidArgumentException('\Drupal\og\MembershipManager::getGroupIds() cannot be used for user entities. Use \Drupal\og\MembershipManager::getUserGroups() instead.');
     }
 
-    $identifier = [
+    $cid = [
       __METHOD__,
       $entity->getEntityTypeId(),
       $entity->id(),
       $group_type_id,
       $group_bundle,
     ];
 
-    $identifier = implode(':', $identifier);
+    $cid = implode(':', $cid);
 
-    if (isset($this->cache[$identifier])) {
+    if ($group_ids = $this->staticCache->get($cid)->data ?? []) {
       // Return cached values.
-      return $this->cache[$identifier];
+      return $group_ids;
     }
 
     $group_ids = [];
+    $tags = $entity->getCacheTagsToInvalidate();
 
     $fields = $this->groupAudienceHelper->getAllGroupAudienceFields($entity->getEntityTypeId(), $entity->bundle(), $group_type_id, $group_bundle);
     foreach ($fields as $field) {
-      $target_type = $field->getFieldStorageDefinition()->getSetting('target_type');
+      $target_type_id = $field->getFieldStorageDefinition()->getSetting('target_type');
+      $target_type_definition = $this->entityTypeManager->getDefinition($target_type_id);
 
       // Optionally filter by group type.
-      if (!empty($group_type_id) && $group_type_id !== $target_type) {
+      if (!empty($group_type_id) && $group_type_id !== $target_type_id) {
         continue;
       }
 
@@ -250,9 +260,9 @@ public function getGroupIds(EntityInterface $entity, $group_type_id = NULL, $gro
       // Query the database to get the actual list of groups. The target IDs may
       // contain groups that no longer exist. Entity reference doesn't clean up
       // orphaned target IDs.
-      $entity_type = $this->entityTypeManager->getDefinition($target_type);
+      $entity_type = $this->entityTypeManager->getDefinition($target_type_id);
       $query = $this->entityTypeManager
-        ->getStorage($target_type)
+        ->getStorage($target_type_id)
         ->getQuery()
         // Disable entity access check so fetching the groups related to group
         // content are not affected by the current user. Furthermore, when
@@ -267,10 +277,14 @@ public function getGroupIds(EntityInterface $entity, $group_type_id = NULL, $gro
         $query->condition($entity_type->getKey('bundle'), $group_bundle);
       }
 
-      $group_ids = NestedArray::mergeDeep($group_ids, [$target_type => $query->execute()]);
+      // Add the list cache tags for the entity type, so that the cache gets
+      // invalidated if one of the group entities is deleted.
+      $tags = Cache::mergeTags($target_type_definition->getListCacheTags(), $tags);
+
+      $group_ids = NestedArray::mergeDeep($group_ids, [$target_type_id => $query->execute()]);
     }
 
-    $this->cache[$identifier] = $group_ids;
+    $this->staticCache->set($cid, $group_ids, Cache::PERMANENT, $tags);
 
     return $group_ids;
   }
@@ -367,13 +381,6 @@ public function isMemberBlocked(EntityInterface $group, AccountInterface $user)
     return $this->isMember($group, $user, [OgMembershipInterface::STATE_BLOCKED]);
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function reset() {
-    $this->cache = [];
-  }
-
   /**
    * Prepares a conditional array for use in a cache identifier and query.
    *
@@ -400,6 +407,22 @@ protected function prepareConditionArray(array $value, array $default = NULL) {
     return array_unique($value);
   }
 
+  /**
+   * Stores the given list of membership IDs in the static cache backend.
+   *
+   * @param string $cid
+   *   The cache ID.
+   * @param array $membership_ids
+   *   The list of membership IDs to store in the static cache.
+   */
+  protected function cacheMembershipIds($cid, array $membership_ids) {
+    $tags = Cache::buildTags('og_membership', $membership_ids);
+    // Also invalidate the list cache tags so that if a new membership is
+    // created it will appear in this list.
+    $tags = Cache::mergeTags(['og_membership_list'], $tags);
+    $this->staticCache->set($cid, $membership_ids, Cache::PERMANENT, $tags);
+  }
+
   /**
    * Returns the full membership entities with the given memberships IDs.
    *

diff --git a/src/MembershipManagerInterface.php b/src/MembershipManagerInterface.php
@@ -273,9 +273,4 @@ public function isMemberPending(EntityInterface $group, AccountInterface $user);
    */
   public function isMemberBlocked(EntityInterface $group, AccountInterface $user);
 
-  /**
-   * Reset the internal cache.
-   */
-  public function reset();
-
 }
diff --git a/src/Og.php b/src/Og.php
@@ -342,9 +342,6 @@ public static function invalidateCache() {
     \Drupal::entityTypeManager()->clearCachedDefinitions();
     \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
 
-    // Invalidate the group membership manager.
-    \Drupal::service('og.membership_manager')->reset();
-
     // Let other OG modules know we invalidate cache.
     \Drupal::moduleHandler()->invokeAll('og_invalidate_cache');
   }

diff --git a/tests/src/Kernel/Entity/CacheInvalidationOnGroupChangeTest.php b/tests/src/Kernel/Entity/CacheInvalidationOnGroupChangeTest.php
@@ -86,11 +86,6 @@ public function testCacheInvalidationOnGroupChange() {
     $tags = Cache::buildTags('og-group-content', $group1->getCacheTagsToInvalidate());
     $bin->set($cid, $this->randomString(), Cache::PERMANENT, $tags);
 
-    // Change the group content entity group. We're clearing first the static
-    // cache of membership manager because in a real application, usually, the
-    // static cache is warmed in a previous request, so that in the request
-    // where the audience is changed, is already empty.
-    $this->container->get('og.membership_manager')->reset();
     $group_content
       ->set(OgGroupAudienceHelperInterface::DEFAULT_FIELD, $group2->id())
       ->save();

diff --git a/tests/src/Kernel/Entity/GroupMembershipManagerTest.php b/tests/src/Kernel/Entity/GroupMembershipManagerTest.php
@@ -88,6 +88,7 @@ protected function setUp() {
     $this->installEntitySchema('og_membership');
     $this->installEntitySchema('user');
     $this->installSchema('system', 'sequences');
+    $this->installSchema('user', 'users_data');
 
     $this->entityTypeManager = $this->container->get('entity_type.manager');
     $this->membershipManager = $this->container->get('og.membership_manager');
@@ -575,6 +576,20 @@ protected function doTestGetGroupMembershipsByRoleNames($method_name, callable $
         }
       }
     }
+
+    // Test that the correct memberships are returned when one of the users is
+    // deleted. We are using the second node group as a test case. This group
+    // has one pending administrator: the user with key '2'.
+    $group = $this->groups['node'][1];
+    $role_names = [OgRoleInterface::ADMINISTRATOR];
+    $states = [OgMembershipInterface::STATE_PENDING];
+    $memberships = $this->membershipManager->$method_name($group, $role_names, $states);
+    $this->assertCount(1, $memberships);
+
+    // Delete the user and check that it no longer appears in the result.
+    $this->users[2]->delete();
+    $memberships = $this->membershipManager->$method_name($group, $role_names, $states);
+    $this->assertCount(0, $memberships);
   }
 
   /**

diff --git a/tests/src/Unit/CreateMembershipTest.php b/tests/src/Unit/CreateMembershipTest.php
@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\og\Unit;
 
+use Drupal\Core\Cache\MemoryCache\MemoryCacheInterface;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityStorageInterface;
@@ -43,6 +44,13 @@ class CreateMembershipTest extends UnitTestCase {
    */
   protected $groupAudienceHelper;
 
+  /**
+   * The mocked memory cache backend.
+   *
+   * @var \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface|\Prophecy\Prophecy\ObjectProphecy
+   */
+  protected $staticCache;
+
   /**
    * The entity storage prophecy used in the test.
    *
@@ -98,6 +106,7 @@ public function setUp() {
     $this->entityTypeManager = $this->prophesize(EntityTypeManagerInterface::class);
     $this->entityTypeRepository = $this->prophesize(EntityTypeRepositoryInterface::class);
     $this->groupAudienceHelper = $this->prophesize(OgGroupAudienceHelperInterface::class);
+    $this->staticCache = $this->prophesize(MemoryCacheInterface::class);
 
     $this->entityTypeManager->getStorage('og_membership')
       ->willReturn($this->entityStorage->reveal());
@@ -139,7 +148,7 @@ public function setUp() {
    * @covers ::createMembership
    */
   public function testNewGroup() {
-    $membership_manager = new MembershipManager($this->entityTypeManager->reveal(), $this->groupAudienceHelper->reveal());
+    $membership_manager = new MembershipManager($this->entityTypeManager->reveal(), $this->groupAudienceHelper->reveal(), $this->staticCache->reveal());
     $membership = $membership_manager->createMembership($this->group->reveal(), $this->user->reveal());
     $this->assertInstanceOf(OgMembershipInterface::class, $membership);
   }