From 76503b557cecabe8a548c46140e13d36a2ec895c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 7 Jul 2020 12:36:05 +0300 Subject: [PATCH 1/3] Remove the static cache in OgAccess::userAccess(). This cache has little value since all the methods that we call that access the database already have a static cache. --- src/Entity/OgRole.php | 5 -- src/OgAccess.php | 126 +++++++------------------------------- src/OgAccessInterface.php | 4 ++ 3 files changed, 27 insertions(+), 108 deletions(-) diff --git a/src/Entity/OgRole.php b/src/Entity/OgRole.php index 669705a03..ef90b88ad 100644 --- a/src/Entity/OgRole.php +++ b/src/Entity/OgRole.php @@ -213,9 +213,6 @@ public function save() { $this->setId($prefix . $this->getName()); } - // Reset access cache, as the role might have changed. - $this->ogAccess()->reset(); - parent::save(); } @@ -257,8 +254,6 @@ public function delete() { throw new OgRoleException('The default roles "non-member" and "member" cannot be deleted.'); } - // Reset access cache, as the role is no longer present. - $this->ogAccess()->reset(); parent::delete(); } diff --git a/src/OgAccess.php b/src/OgAccess.php index 101c0c32b..e0bdaaa94 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -4,7 +4,6 @@ use Drupal\Core\Access\AccessResult; use Drupal\Core\Cache\CacheableMetadata; -use Drupal\Core\Cache\RefinableCacheableDependencyInterface; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Extension\ModuleHandlerInterface; @@ -33,16 +32,6 @@ class OgAccess implements OgAccessInterface { */ const UPDATE_GROUP_PERMISSION = 'update group'; - /** - * Static cache that contains cache permissions. - * - * @var array - * Array keyed by the following keys: - * - alter: The permissions after altered by implementing modules. - * - pre_alter: The pre-altered permissions, as read from the config. - */ - protected $permissionsCache = []; - /** * The config factory. * @@ -178,60 +167,42 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface } } - $pre_alter_cache = $this->getPermissionsCache($group, $user, TRUE); - $post_alter_cache = $this->getPermissionsCache($group, $user, FALSE); - - // To reduce the number of SQL queries, we cache the user's permissions. - if (!$pre_alter_cache) { - $permissions = []; - $user_is_group_admin = FALSE; - if ($membership = $this->membershipManager->getMembership($group, $user->id())) { - foreach ($membership->getRoles() as $role) { - // Check for the is_admin flag. - /** @var \Drupal\og\Entity\OgRole $role */ - if ($role->isAdmin()) { - $user_is_group_admin = TRUE; - break; - } - - $permissions = array_merge($permissions, $role->getPermissions()); + $permissions = []; + $user_is_group_admin = FALSE; + if ($membership = $this->membershipManager->getMembership($group, $user->id())) { + foreach ($membership->getRoles() as $role) { + // Check for the is_admin flag. + if ($role->isAdmin()) { + $user_is_group_admin = TRUE; + break; } - } - elseif (!$this->membershipManager->isMember($group, $user->id(), [OgMembershipInterface::STATE_BLOCKED])) { - // User is a non-member or has a pending membership. - /** @var \Drupal\og\Entity\OgRole $role */ - $role = OgRole::loadByGroupAndName($group, OgRoleInterface::ANONYMOUS); - $permissions = $role->getPermissions(); - } - - $permissions = array_unique($permissions); - $this->setPermissionCache($group, $user, TRUE, $permissions, $user_is_group_admin, $cacheable_metadata); + $permissions = array_merge($permissions, $role->getPermissions()); + } + } + elseif (!$this->membershipManager->isMember($group, $user->id(), [OgMembershipInterface::STATE_BLOCKED])) { + // User is a non-member or has a pending membership. + /** @var \Drupal\og\Entity\OgRole $role */ + $role = OgRole::loadByGroupAndName($group, OgRoleInterface::ANONYMOUS); + $permissions = $role->getPermissions(); } - if (!$skip_alter && !in_array($operation, $post_alter_cache)) { - // Let modules alter the permissions. So we get the original ones, and - // pass them along to the implementing modules. - $alterable_permissions = $this->getPermissionsCache($group, $user, TRUE); + $permissions = array_unique($permissions); + if (!$skip_alter && !in_array($operation, $permissions)) { + // Let modules alter the permissions. $context = [ 'operation' => $operation, 'group' => $group, 'user' => $user, ]; - $this->moduleHandler->alter('og_user_access', $alterable_permissions['permissions'], $cacheable_metadata, $context); - - $this->setPermissionCache($group, $user, FALSE, $alterable_permissions['permissions'], $alterable_permissions['is_admin'], $cacheable_metadata); + $this->moduleHandler->alter('og_user_access', $permissions, $cacheable_metadata, $context); } - $altered_permissions = $this->getPermissionsCache($group, $user, FALSE); - - $user_is_group_admin = !empty($altered_permissions['is_admin']); - - if (($user_is_group_admin && !$ignore_admin) || in_array($operation, $altered_permissions['permissions'])) { + if (($user_is_group_admin && !$ignore_admin) || in_array($operation, $permissions)) { // User is a group admin, and we do not ignore this special permission // that grants access to all the group permissions. - return AccessResult::allowed()->addCacheableDependency($altered_permissions['cacheable_metadata']); + return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } return AccessResult::forbidden()->addCacheableDependency($cacheable_metadata); @@ -351,62 +322,11 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac return AccessResult::neutral()->addCacheableDependency($cacheable_metadata); } - /** - * Set the permissions in the static cache. - * - * @param \Drupal\Core\Entity\EntityInterface $group - * The entity object. - * @param \Drupal\Core\Session\AccountInterface $user - * The user object. - * @param bool $pre_alter - * Determines if the type of permissions is pre-alter or post-alter. - * @param array $permissions - * Array of permissions to set. - * @param bool $is_admin - * Whether or not the user is a group administrator. - * @param \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheable_metadata - * A cacheable metadata object. - */ - protected function setPermissionCache(EntityInterface $group, AccountInterface $user, $pre_alter, array $permissions, $is_admin, RefinableCacheableDependencyInterface $cacheable_metadata) { - $entity_type_id = $group->getEntityTypeId(); - $group_id = $group->id(); - $user_id = $user->id(); - $type = $pre_alter ? 'pre_alter' : 'post_alter'; - - $this->permissionsCache[$entity_type_id][$group_id][$user_id][$type] = [ - 'is_admin' => $is_admin, - 'permissions' => $permissions, - 'cacheable_metadata' => $cacheable_metadata, - ]; - } - - /** - * Get the permissions from the static cache. - * - * @param \Drupal\Core\Entity\EntityInterface $group - * The entity object. - * @param \Drupal\Core\Session\AccountInterface $user - * The user object. - * @param bool $pre_alter - * Determines if the type of permissions is pre-alter or post-alter. - * - * @return array - * Array of permissions if cached, or an empty array. - */ - protected function getPermissionsCache(EntityInterface $group, AccountInterface $user, $pre_alter) { - $entity_type_id = $group->getEntityTypeId(); - $group_id = $group->id(); - $user_id = $user->id(); - $type = $pre_alter ? 'pre_alter' : 'post_alter'; - - return isset($this->permissionsCache[$entity_type_id][$group_id][$user_id][$type]) ? $this->permissionsCache[$entity_type_id][$group_id][$user_id][$type] : []; - } - /** * {@inheritdoc} */ public function reset() { - $this->permissionsCache = []; + trigger_error('OgAccessInterface::reset() is deprecated in og:8.1.0-alpha6 and is removed from og:8.1.0-beta1. The static cache has been removed and this no longer server any purpose. See https://github.com/Gizra/og/issues/654', E_USER_DEPRECATED); } } diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 04cd05da2..eb6e934e7 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -83,6 +83,10 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac /** * Resets the static cache. + * + * @deprecated in og:8.x-1.0-alpha6 and is removed from og:8.x-1.0-beta1. + * The static cache has been removed and this method no longer serves any + * purpose. Any calls to this method can safely be removed. */ public function reset(); From e914e49e3395fb5bd5a7110d416669915f44e968 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 7 Jul 2020 13:09:23 +0300 Subject: [PATCH 2/3] Avoid calling deprecated code. --- src/Entity/OgMembership.php | 1 - src/OgAccessInterface.php | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Entity/OgMembership.php b/src/Entity/OgMembership.php index 68896c894..34f28a750 100644 --- a/src/Entity/OgMembership.php +++ b/src/Entity/OgMembership.php @@ -508,7 +508,6 @@ public function save() { // Reset internal cache. Og::reset(); - \Drupal::service('og.access')->reset(); return $result; } diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index eb6e934e7..60b0c50a3 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -87,6 +87,7 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac * @deprecated in og:8.x-1.0-alpha6 and is removed from og:8.x-1.0-beta1. * The static cache has been removed and this method no longer serves any * purpose. Any calls to this method can safely be removed. + * @see https://github.com/Gizra/og/issues/654 */ public function reset(); From 9da6b4ef170f2ba61b2f734f4b27c282cd5e1eb5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 7 Jul 2020 13:25:55 +0300 Subject: [PATCH 3/3] Allow deprecation warnings in comments to refer to Github issues. --- phpcs-ruleset.xml.dist | 1 + 1 file changed, 1 insertion(+) diff --git a/phpcs-ruleset.xml.dist b/phpcs-ruleset.xml.dist index 252b62c83..046ef46de 100644 --- a/phpcs-ruleset.xml.dist +++ b/phpcs-ruleset.xml.dist @@ -16,6 +16,7 @@ +