From 65b9436f2adf5887e014d018041e0eaf722f29c7 Mon Sep 17 00:00:00 2001 From: Kristiaan Van den Eynde Date: Thu, 23 Nov 2017 11:11:47 +0100 Subject: [PATCH] Issue #2896603 by kristiaanvandeneynde, kevin.dutra, mikran: Static caching issue when loading group roles by user & group --- group.module | 17 ++++ src/Entity/GroupContent.php | 39 +++++++-- src/Entity/Storage/GroupRoleStorage.php | 22 +++-- src/Entity/Storage/GroupRoleStorageInterface.php | 11 +++ tests/src/Kernel/GroupRoleStorageTest.php | 104 +++++++++++++++++++++++ 5 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 tests/src/Kernel/GroupRoleStorageTest.php diff --git a/group.module b/group.module index 046b178..d4f02bb 100644 --- a/group.module +++ b/group.module @@ -319,6 +319,23 @@ function _group_mass_reassign_to_super_user(array $ids, &$context) { } /** + * Implements hook_ENTITY_TYPE_update(). + */ +function group_user_update(AccountInterface $account) { + // If a user's roles change, we need to reset their group roles cache. + $new = array_unique($account->getRoles()); + $old = array_unique($account->original->getRoles()); + sort($new); + sort($old); + + if ($new != $old) { + /** @var \Drupal\group\Entity\Storage\GroupRoleStorageInterface $storage */ + $storage = \Drupal::entityTypeManager()->getStorage('group_role'); + $storage->resetUserGroupRoleCache($account); + } +} + +/** * Implements hook_ENTITY_TYPE_delete(). */ function group_user_delete(EntityInterface $account) { diff --git a/src/Entity/GroupContent.php b/src/Entity/GroupContent.php index ca91e98..fbd59de 100644 --- a/src/Entity/GroupContent.php +++ b/src/Entity/GroupContent.php @@ -200,6 +200,10 @@ class GroupContent extends ContentEntityBase implements GroupContentInterface { public function postSave(EntityStorageInterface $storage, $update = TRUE) { parent::postSave($storage, $update); + // For memberships, we generally need to rebuild the group role cache for + // the member's user account in the target group. + $rebuild_group_role_cache = $this->getContentPlugin()->getPluginId() == 'group_membership'; + if ($update === FALSE) { // We want to make sure that the entity we just added to the group behaves // as a grouped entity. This means we may need to update access records, @@ -208,6 +212,22 @@ class GroupContent extends ContentEntityBase implements GroupContentInterface { // happens when saving an entity so let's re-save the added entity. $this->getEntity()->save(); } + + // If a membership gets updated, but the member's roles haven't changed, we + // do not need to rebuild the group role cache for the member's account. + elseif ($rebuild_group_role_cache) { + $new = array_column($this->group_roles->getValue(), 'target_id'); + $old = array_column($this->original->group_roles->getValue(), 'target_id'); + sort($new); + sort($old); + $rebuild_group_role_cache = ($new != $old); + } + + if ($rebuild_group_role_cache) { + /** @var \Drupal\group\Entity\Storage\GroupRoleStorageInterface $role_storage */ + $role_storage = \Drupal::entityTypeManager()->getStorage('group_role'); + $role_storage->resetUserGroupRoleCache($this->getEntity(), $this->getGroup()); + } } /** @@ -216,16 +236,25 @@ class GroupContent extends ContentEntityBase implements GroupContentInterface { public static function postDelete(EntityStorageInterface $storage, array $entities) { parent::postDelete($storage, $entities); - // For the same reasons we re-save entities that are added to a group, we - // need to re-save entities that were removed from one. See ::postSave(). /** @var GroupContentInterface[] $entities */ foreach ($entities as $group_content) { - // We only save the entity if it still exists to avoid trying to save an - // entity that just got deleted and triggered the deletion of its group - // content entities. if ($entity = $group_content->getEntity()) { + // For the same reasons we re-save entities that are added to a group, + // we need to re-save entities that were removed from one. See + // ::postSave(). We only save the entity if it still exists to avoid + // trying to save an entity that just got deleted and triggered the + // deletion of its group content entities. // @todo Revisit when https://www.drupal.org/node/2754399 lands. $entity->save(); + + // If a membership gets deleted, we need to reset the internal group + // roles cache for the member in that group, but only if the user still + // exists. Otherwise, it doesn't matter as the user ID will become void. + if ($entity->getContentPlugin()->getPluginId() == 'group_membership') { + /** @var \Drupal\group\Entity\Storage\GroupRoleStorageInterface $role_storage */ + $role_storage = \Drupal::entityTypeManager()->getStorage('group_role'); + $role_storage->resetUserGroupRoleCache($entity->getEntity(), $entity->getGroup()); + } } } } diff --git a/src/Entity/Storage/GroupRoleStorage.php b/src/Entity/Storage/GroupRoleStorage.php index 4bd9cbc..8de61a4 100644 --- a/src/Entity/Storage/GroupRoleStorage.php +++ b/src/Entity/Storage/GroupRoleStorage.php @@ -24,8 +24,6 @@ class GroupRoleStorage extends ConfigEntityStorage implements GroupRoleStorageIn /** * Static cache of a user's group role IDs. * - * @todo Perhaps we need to be able to clear this cache during runtime? - * * @var array */ protected $userGroupRoleIds = []; @@ -86,8 +84,9 @@ class GroupRoleStorage extends ConfigEntityStorage implements GroupRoleStorageIn public function loadByUserAndGroup(AccountInterface $account, GroupInterface $group, $include_implied = TRUE) { $uid = $account->id(); $gid = $group->id(); + $key = $include_implied ? 'include' : 'exclude'; - if (!isset($this->userGroupRoleIds[$uid][$gid])) { + if (!isset($this->userGroupRoleIds[$uid][$gid][$key])) { $ids = []; // Get the IDs from the 'group_roles' field, without loading the roles. @@ -115,10 +114,23 @@ class GroupRoleStorage extends ConfigEntityStorage implements GroupRoleStorageIn } } - $this->userGroupRoleIds[$uid][$gid] = $ids; + $this->userGroupRoleIds[$uid][$gid][$key] = $ids; } - return $this->loadMultiple($this->userGroupRoleIds[$uid][$gid]); + return $this->loadMultiple($this->userGroupRoleIds[$uid][$gid][$key]); + } + + /** + * {@inheritdoc} + */ + public function resetUserGroupRoleCache(AccountInterface $account, GroupInterface $group = NULL) { + $uid = $account->id(); + if (isset($group)) { + unset($this->userGroupRoleIds[$uid][$group->id()]); + } + else { + unset($this->userGroupRoleIds[$uid]); + } } } diff --git a/src/Entity/Storage/GroupRoleStorageInterface.php b/src/Entity/Storage/GroupRoleStorageInterface.php index 0269586..3700b92 100644 --- a/src/Entity/Storage/GroupRoleStorageInterface.php +++ b/src/Entity/Storage/GroupRoleStorageInterface.php @@ -27,4 +27,15 @@ interface GroupRoleStorageInterface extends ConfigEntityStorageInterface { */ public function loadByUserAndGroup(AccountInterface $account, GroupInterface $group, $include_implied = TRUE); + /** + * Resets the internal, static cache used by ::loadByUserAndGroup(). + * + * @param \Drupal\Core\Session\AccountInterface $account + * The account to reset the cached group roles for. + * @param \Drupal\group\Entity\GroupInterface $group + * (optional) The group to reset the user's cached group roles for. Leave + * blank to reset the user's roles in all groups. + */ + public function resetUserGroupRoleCache(AccountInterface $account, GroupInterface $group = NULL); + } diff --git a/tests/src/Kernel/GroupRoleStorageTest.php b/tests/src/Kernel/GroupRoleStorageTest.php new file mode 100644 index 0000000..cd7fb0f --- /dev/null +++ b/tests/src/Kernel/GroupRoleStorageTest.php @@ -0,0 +1,104 @@ +storage = $this->entityTypeManager->getStorage('group_role'); + + $this->group = $this->createGroup(); + $this->account = $this->createUser(); + + $this->storage->create([ + 'id' => 'default-editor', + 'label' => 'Default editor', + 'weight' => 0, + 'group_type' => 'default', + ])->save(); + } + + /** + * Tests the loading of group roles by user and group. + * + * @covers ::loadByUserAndGroup + */ + public function testLoadByUserAndGroup() { + $this->compareMemberRoles([], FALSE, 'User has no explicit group roles as they are not a member.'); + $this->compareMemberRoles(['default-outsider'], TRUE, 'User initially has implicit outsider role.'); + + // Grant the user a new site role and check the storage. + $this->entityTypeManager->getStorage('user_role')->create(['id' => 'publisher'])->save(); + $this->account->addRole('publisher'); + $this->account->save(); + $group_role_id = _group_role_synchronizer()->getGroupRoleId('default', 'publisher'); + $this->compareMemberRoles([], FALSE, 'User has no explicit group roles as they are not a member.'); + $this->compareMemberRoles([$group_role_id, 'default-outsider'], TRUE, 'User has implicit and synchronized outsider roles.'); + + // From this point on we test with the user as a member. + $this->group->addMember($this->account); + $this->compareMemberRoles([], FALSE, 'User still has no explicit group roles.'); + $this->compareMemberRoles(['default-member'], TRUE, 'User has implicit member role now that they have joined the group.'); + + // Grant the member a new group role and check the storage. + // @todo This displays a desperate need for addRole() and removeRole(). + $membership = $this->group->getMember($this->account)->getGroupContent(); + $membership->group_roles[] = 'default-editor'; + $membership->save(); + $this->compareMemberRoles(['default-editor'], FALSE, 'User has the editor group role.'); + $this->compareMemberRoles(['default-editor', 'default-member'], TRUE, 'User also has implicit member role.'); + } + + /** + * Asserts that the test user's group roles match a provided list of IDs. + * + * @param string[] $expected + * The group role IDs we expect the user to have. + * @param bool $include_implied + * Whether to include internal group roles. + * @param string $message + * The message to display for the assertion. + */ + protected function compareMemberRoles($expected, $include_implied, $message) { + $group_roles = $this->storage->loadByUserAndGroup($this->account, $this->group, $include_implied); + $actual = array_keys($group_roles); + sort($actual); + sort($expected); + $this->assertEquals($expected, $actual, $message); + } + +} -- 2.8.1