From d7d09debfec917dedd2df59e11c699dcc4254bed Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Wed, 14 May 2025 16:03:24 +0200
Subject: [PATCH 1/5] Just a proof of concept to get the ball rolling (and
tests failing), I feel like the Pathauto code can be improved upon a lot.
---
src/Entity/GroupRelationship.php | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/Entity/GroupRelationship.php b/src/Entity/GroupRelationship.php
index 18eed521..33ae5844 100644
--- a/src/Entity/GroupRelationship.php
+++ b/src/Entity/GroupRelationship.php
@@ -2,6 +2,7 @@
namespace Drupal\group\Entity;
+use Drupal\Core\Cache\Cache;
use Drupal\Core\Entity\Attribute\ContentEntityType;
use Drupal\Core\Entity\ContentEntityBase;
use Drupal\Core\Entity\EntityChangedTrait;
@@ -270,11 +271,24 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
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,
- // flush some caches containing the entity or perform other operations we
- // cannot possibly know about. Lucky for us, all of that behavior usually
- // happens when saving an entity so let's re-save the added entity.
- $this->getEntity()->save();
+ // as a grouped entity. Any code that runs "live" should get the new
+ // information, but we want to invalidate cache entries that listed the
+ // entity as a cacheable dependency.
+ Cache::invalidateTags($this->getEntity());
+
+ // One drawback to the above is that not everything gets cached. Some
+ // things get calculated and stored as entities (or other DB entries) of
+ // their own without any cacheable metadata attached. One example being
+ // the Pathauto module generating path aliases upon entity save.
+ //
+ // Because we cannot know about all of these implementations, and we also
+ // can't rely on their modules to listen to a Group event, we manually
+ // trigger the usual suspects. In this case, Pathauto.
+ //
+ // @todo Implement a more extensible way to do this.
+ if (\Drupal::moduleHandler()->moduleExists('pathauto')) {
+ \Drupal::service('pathauto.generator')->updateEntityAlias($this->getEntity(), 'update');
+ }
}
// If a membership gets updated, but the member's roles haven't changed, we
--
GitLab
From 63e219e96c2babb2796e96dddf4842be84655757 Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Thu, 15 May 2025 15:07:45 +0200
Subject: [PATCH 2/5] Obviously can't invalidate cacheable dependencies, but
cache tags instead :facepalm:
---
src/Entity/GroupRelationship.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/Entity/GroupRelationship.php b/src/Entity/GroupRelationship.php
index 33ae5844..4669c359 100644
--- a/src/Entity/GroupRelationship.php
+++ b/src/Entity/GroupRelationship.php
@@ -274,7 +274,7 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
// as a grouped entity. Any code that runs "live" should get the new
// information, but we want to invalidate cache entries that listed the
// entity as a cacheable dependency.
- Cache::invalidateTags($this->getEntity());
+ Cache::invalidateTags($this->getEntity()->getCacheTags());
// One drawback to the above is that not everything gets cached. Some
// things get calculated and stored as entities (or other DB entries) of
--
GitLab
From a87ca5321c74f5e7950bbcfe9a4cf4ee8e46de0a Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Thu, 15 May 2025 15:48:21 +0200
Subject: [PATCH 3/5] Update test that used to check for entity resave.
---
.../group_test/src/Hook/EntityHooks.php | 25 -------------------
tests/src/Kernel/GroupRelationshipTest.php | 22 ++++++++--------
2 files changed, 10 insertions(+), 37 deletions(-)
delete mode 100644 tests/modules/group_test/src/Hook/EntityHooks.php
diff --git a/tests/modules/group_test/src/Hook/EntityHooks.php b/tests/modules/group_test/src/Hook/EntityHooks.php
deleted file mode 100644
index 5eee1f06..00000000
--- a/tests/modules/group_test/src/Hook/EntityHooks.php
+++ /dev/null
@@ -1,25 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Drupal\group_test\Hook;
-
-use Drupal\Core\Hook\Attribute\Hook;
-use Drupal\user\UserInterface;
-
-/**
- * Entity hook implementations for Group test.
- */
-final class EntityHooks {
-
- /**
- * Implements hook_ENTITY_TYPE_update().
- */
- #[Hook('user_update')]
- public function testUserUpdate(UserInterface $user): void {
- if ($user->getChangedTime() == 123456789) {
- $user->setChangedTime(530496000)->save();
- }
- }
-
-}
diff --git a/tests/src/Kernel/GroupRelationshipTest.php b/tests/src/Kernel/GroupRelationshipTest.php
index db2109a2..da344745 100644
--- a/tests/src/Kernel/GroupRelationshipTest.php
+++ b/tests/src/Kernel/GroupRelationshipTest.php
@@ -21,7 +21,7 @@ class GroupRelationshipTest extends GroupKernelTestBase {
/**
* {@inheritdoc}
*/
- protected static $modules = ['group_test', 'group_test_plugin', 'node'];
+ protected static $modules = ['group_test_plugin', 'node'];
/**
* Tests that entity url templates are functional.
@@ -70,23 +70,21 @@ class GroupRelationshipTest extends GroupKernelTestBase {
}
/**
- * Tests that after adding an entity to a group, it gets saved again.
+ * Tests that adding an entity to a group invalidates its cache tags.
*
* @covers ::postSave
- *
- * @see group_test_user_update()
*/
- public function testSubjectResaved() {
- $changed = 123456789;
- $account = $this->createUser([], NULL, FALSE, ['changed' => $changed]);
+ public function testSubjectCacheTagsInvalidated() {
+ $cid = 'test_group_relationship';
+ $cache = \Drupal::cache();
+ $account = $this->createUser();
+
+ $cache->set($cid, 'hello', tags: $account->getCacheTags());
+ $this->assertNotFalse($cache->get($cid), 'Cache entry based on account cache tags exists.');
$group = $this->createGroup(['type' => $this->createGroupType()->id()]);
$group->addRelationship($account, 'group_membership');
-
- // All users whose changed time was set to 123456789 get their changed time
- // set to 530496000 in group_test_user_update() when the account is updated.
- $account_unchanged = $this->entityTypeManager->getStorage('user')->loadUnchanged($account->id());
- $this->assertEquals(530496000, $account_unchanged->getChangedTime(), 'Account was saved as part of being added to a group.');
+ $this->assertFalse($cache->get($cid), 'Cache entry based on account cache tags is flushed when account is added to a group.');
}
/**
--
GitLab
From c0d986c519829a1a2f2b80b21148e52d63aa43a4 Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Tue, 20 May 2025 14:46:14 +0200
Subject: [PATCH 4/5] Update to not special case Pathauto, defer that to a
support module in another issue.
---
src/Entity/GroupRelationship.php | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/Entity/GroupRelationship.php b/src/Entity/GroupRelationship.php
index 4669c359..a5e11621 100644
--- a/src/Entity/GroupRelationship.php
+++ b/src/Entity/GroupRelationship.php
@@ -274,21 +274,16 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
// as a grouped entity. Any code that runs "live" should get the new
// information, but we want to invalidate cache entries that listed the
// entity as a cacheable dependency.
- Cache::invalidateTags($this->getEntity()->getCacheTags());
-
+ //
// One drawback to the above is that not everything gets cached. Some
// things get calculated and stored as entities (or other DB entries) of
// their own without any cacheable metadata attached. One example being
// the Pathauto module generating path aliases upon entity save.
//
- // Because we cannot know about all of these implementations, and we also
- // can't rely on their modules to listen to a Group event, we manually
- // trigger the usual suspects. In this case, Pathauto.
- //
- // @todo Implement a more extensible way to do this.
- if (\Drupal::moduleHandler()->moduleExists('pathauto')) {
- \Drupal::service('pathauto.generator')->updateEntityAlias($this->getEntity(), 'update');
- }
+ // It is up to these modules to listen to hook_group_relationship_insert()
+ // and call ::getEntity() on the relationship to also run their code on
+ // the entity that got added to a group.
+ Cache::invalidateTags($this->getEntity()->getCacheTags());
}
// If a membership gets updated, but the member's roles haven't changed, we
--
GitLab
From df1fe5a8a18aa448e85d168038337a14a0a1b313 Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Thu, 5 Jun 2025 15:56:52 +0200
Subject: [PATCH 5/5] Also flush cache tags upon removal from a group
---
src/Entity/GroupRelationship.php | 19 +++++++++++--------
tests/src/Kernel/GroupRelationshipTest.php | 8 +++++++-
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/Entity/GroupRelationship.php b/src/Entity/GroupRelationship.php
index a5e11621..3d745ca6 100644
--- a/src/Entity/GroupRelationship.php
+++ b/src/Entity/GroupRelationship.php
@@ -25,6 +25,7 @@ use Drupal\group\Entity\Views\GroupRelationshipViewsData;
use Drupal\group\Form\GroupJoinForm;
use Drupal\group\Form\GroupLeaveForm;
use Drupal\user\EntityOwnerTrait;
+use Drupal\user\UserInterface;
/**
* Defines the relationship entity.
@@ -314,13 +315,14 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
foreach ($entities as $group_relationship) {
assert($group_relationship instanceof GroupRelationshipInterface);
if ($entity = $group_relationship->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 relationship entities.
- // @todo Revisit when https://www.drupal.org/node/2754399 lands.
- $entity->save();
+ // For the same reason we invalidate the cache tags of entities that are
+ // added to a group, we need to invalidate the cache tags of those that
+ // were removed from one. See ::postSave().
+ //
+ // We can only invalidate the cache tags of entities that still exist
+ // and not the ones that were just deleted and triggered the deletion of
+ // their group relationship entities.
+ Cache::invalidateTags($entity->getCacheTags());
// 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
@@ -328,7 +330,8 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
if ($group_relationship->getPluginId() == 'group_membership') {
$role_storage = \Drupal::entityTypeManager()->getStorage('group_role');
assert($role_storage instanceof GroupRoleStorageInterface);
- $role_storage->resetUserGroupRoleCache($group_relationship->getEntity(), $group_relationship->getGroup());
+ assert($entity instanceof UserInterface);
+ $role_storage->resetUserGroupRoleCache($entity, $group_relationship->getGroup());
}
}
}
diff --git a/tests/src/Kernel/GroupRelationshipTest.php b/tests/src/Kernel/GroupRelationshipTest.php
index da344745..07e5e594 100644
--- a/tests/src/Kernel/GroupRelationshipTest.php
+++ b/tests/src/Kernel/GroupRelationshipTest.php
@@ -83,8 +83,14 @@ class GroupRelationshipTest extends GroupKernelTestBase {
$this->assertNotFalse($cache->get($cid), 'Cache entry based on account cache tags exists.');
$group = $this->createGroup(['type' => $this->createGroupType()->id()]);
- $group->addRelationship($account, 'group_membership');
+ $group_relationship = $group->addRelationship($account, 'group_membership');
$this->assertFalse($cache->get($cid), 'Cache entry based on account cache tags is flushed when account is added to a group.');
+
+ $cache->set($cid, 'hello', tags: $account->getCacheTags());
+ $this->assertNotFalse($cache->get($cid), 'Cache entry based on account cache tags exists.');
+
+ $group_relationship->delete();
+ $this->assertFalse($cache->get($cid), 'Cache entry based on account cache tags is flushed when account is removed from a group.');
}
/**
--
GitLab