Stop saving an entity when it gets added to or removed from a group

Forcing the content entity to be immediately re-saved in the GroupContent::postSave() method is a problematic architectural choice. One of the implications is that, when the content entity is saved, hook_ENTITY_TYPE_insert() and hook_ENTITY_TYPE_update() are both called on that entity.

In the case of the site on which I am working right now, we also have a custom field for selecting other groups where a content item can be cross-posted, and as a result, when an entity is placed in multiple groups, GroupContent::postSave() not only causes both hook_ENTITY_TYPE_insert() and hook_ENTITY_TYPE_update() to be called, but it also causes hook_ENTITY_TYPE_update() to be called repeatedly, once for each group where the entity is posted. We have some implementations of each of those hooks, which both call a method that adds messages to a queue for users who want to subscribe to get notifications for new or updated content in groups. The repeated invocations of these hooks on the same entity save operation causes duplicate items to be added to the messages queue.

At the moment we have to use a workaround to track an array of entity ids that we have already processed in a give request, so that we don't create duplicate queue entries for the same entity save operation, but this is less than ideal. This situation has also required us to write some less-than-ideal workarounds in our unit and functional tests.

I know that addressing this issue requires a deeper architectural review of options, but definitely want to add my +1 vote on the importance of this issue.

Also to note, the GroupContent entity is handling node saves the same for both these scenarios:

• existing content is added to a group from the group content page (in this case, fine, resave the node)
• new or edited node/content added to the group from the node edit form (no need to resave here as the caches, access, etc are already invalidated)

I also stumbled upon this problem. I have a temporary work-a-round where I patched the group module to mark the post save with an extra property on the entity. This way I can check in my update hook if this property is set, it is actually and update called by the group content entity:

  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    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.
      $entity = $this->getEntity();
      $entity->fromGroupModule = TRUE;
      $entity->save();
    }
  }

Then in my update hook

if(empty($entity->fromGroupModule)){
  // This is an actual update, not an insert after the group content saves the entity again.
}

This is the patch

Can anyone confirm this is the right approach?

Not sure about the approach, but totally agree that this is a bug. For revisionable entities it creates an extra revision when added to group and this screws up revisions history and confuses editors.

This might be as simple as having the GroupContent entity implement its own ::getCacheTags, which would then return the entity cache tag (eg, node:123, taxonomy:456, or whatever associated entity the group content is for):

  /**
   * {@inheritdoc}
   */
  public function getCacheTags() {
    return Cache::mergeTags(parent::getCacheTags(), $this->getEntity()->getCacheTags());
  }

Alternatively, or in addition to, it might need to implement getCacheTagsToInvalidate() as well. A test or two would be quite useful to determine the effect of this fix.

Thanks for the suggestion @jhedstrom.
I think this is a good way forward.

After additional testing I realize that my previous patch doesn't solve the issue. Yes there is no more extra entity save but also in case of nodes there is no more access grants update which is crucial.

Implementation of getCacheTags() is still probably a thing but it doesn't help to solve this issue.

+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheTags() {
+    if ($entity = $this->getEntity()) {
+      $this->addCacheableDependency($entity);
+    }
+
+    return parent::getCacheTags();
+  }

Should this be put in the separate issue?

So the main question remains - how to invoke all entity related insert/update operations without re-saving it?

New patch version.

If we can treat Entity::postSave() method as public API then this is probably a way forward to avoid extra save on entity added to group.

Re-roll.

This issue is actually causing circular logic in my application, since I want to add a node to a group up save based on certain conditions. I don't see how I would do that without using hook_node_update/insert but then kicking off an infinite loop of node updates calling node updates. I guess adding an arbitrary property to the entity and then looking for it the second time around would work? Is anyone using this kind of logic in their application?

@RoSk0 Why is this marked as "Needs Work" rather than "Needs Review"? It's not clear from the comments whether the issues you mentioned in #13 are totally resolved in #14? On cursory inspection the patch looks okay to me and seems to work.

EDIT: Disregard my original question, missed that it was automatically switched to "Needs Work" due to tests failing. The patch *does* seem to apply for me so I'm going to move it back to "Needs Review".

I encountered an issue with forum nodes from the Drupal forum module that could belong to a group. On node save the forum module inserts a row to the forum_index table. This happens in Drupal\forum\ForumIndexStorage::createIndex(). It ends up trying to insert the row twice because of the weirdness caused by the extra save from this issue. Your patch fixes this.

I'm not sure if postSave() might cause similar problems but it certainly seems to cause fewer issues than save().

We've run into an issue with this patch where user account created e-mails are sent out when they shouldn't be. It may be related to some unusual aspects of our system since users are added to certain groups as soon as they are created. Here's what happens:

1. A user is added via the "Add user form" with the "Notify user of new account" box unchecked
2. In hook_user_insert() the user is added to a group or groups
3. GroupContent::postSave() is triggered, which in turn triggers User::postSave()
4. User::postSave() sends the notification e-mail (via _user_mail_notify) regardless of whether the box is checked

The e-mail being sent is the one labelled as "Account activation" in the Drupal user account settings at /admin/config/people/accounts.

In our case I think we can just disable that e-mail since it shouldn't be being sent out in general, but it's worth noting.

This should fix the tests of #16.

I think this change is problematic.

I agree that the resave is a problem for performance, but it is also important. Just calling the postSave() method on the entity is an arbitrary hack and doesn't cover hooks and so on for example.

One example that this patch would currently break, we have pathauto aliases based on the group (we only have single-group assignments), the update is necessary so that pathauto can generate the correct alias. Yes, it's definitely not optimal, because we had to implement a pathauto hook that prevents aliases (and then redirects) for content that is saved the first time.

@berdir, I actually found that out yesterday after adding the patch to our project. In the end we didn't use this patch for that exact reason. The problem we had was that saving a draft for the first time created 2 revisions. We now solved that bij overriding Drupal\content_moderation\Entity\Handler\ModerationHandler::onPresave() to remove $entity->setNewRevision(TRUE).

Which is a different issue obviously. Thanks for the feedback. Back to 'Needs work'.

Yeah, that's a core issue that we should get back to :)

#2803717: Allow 'syncing' content to be updated in content moderation without forcing the creation of a new revision

I'm tempted to move this to Critical but I'll refrain for now. Group module is very much an API module which encourages developers to implement own functionality on top. This re-save triggers all kinds of unwanted/unexpected hooks.

This actually looks like a critical issue.

New project and I'm facing same bug again. This time this extra revision produced on second save is actually ruing translation workflow for us, because this new extra revision have "revision_translation_affected" flag unset.

It looks like core recognised similar use case as a bug and has a fix for it.
Unfortunately it's not going to be included in 8.7, fortunately you can just apply that patch from [2803717#48] with Composer.

In conjunction with this small patch it all works as it should.

@RoskO, shouldn't we do it also in postDelete ?

Sure, content entity sync was introduced only in 8.7.

That is a sweet core update! If this works out well, I can see if we can bump Group to require Drupal 8.7.

Also, this is far from critical. Please read the https://www.drupal.org/core/issue-priority carefully. Moving back to Major, as it is still a "feature" that causes a lot of people problems, it seems.

There is another unfortunate side effect to this save that isn't solved by the patch above (ideas welcome).
By calling save on the nodes, it means we are invalidating the node tags on delete and create. This means that even though nothing changed on the node, it still gets flushed.
Now, I understand this is important for those of us with access checks to ensure nodes are accessible outside the Groups, but that's not my case unfortunately.
Is there any way we can achieve a similar result without resaving the node at all?

setSyncing(TRUE) looks like a good idea but this issue is about getting rid of the re-save entirely. Its brutal for performance during a migration, and also the various pains people have mentioned above.

Can someone explain to me what the exact purpose of this is?
We are having such sever impact on cache invalidation that I might just need to override the GroupContent class and override this code.
I just need to understand what impact that would have.

#33 does not work, it still triggers hook_entity_insert that cause many problems. By far #22 is working at least to me though it still feels tweaked by invoking postSave. A formal solution needs to make it clear what the exact purpose of entity save behaviour and fix it instead of "workaround" it.
I'm rerolling #22 to rc5 version as a temporary solution.

Stumbled upon this, as well. In particular the extra revisions created when using Group and Content Moderation for the same entities is pretty annoying. Since that seems like a fairly concrete bug to me, here's a test for that - on top of patch #33. I don't mean to play patch ping-pong with #40 and I can also see that e.g. per #38 the saving itself is still problematic even with #33, but I do think this is a fairly straightforward fix we could do now while pursuing the other, larger issue afterwards.

Maybe @kristiaanvandeneynde you can make a determination in terms of process, what you want to discuss here and/or if you want a separate issue for either of the two aspects.

Here's a quick interdiff for the coding style violations in #41.

I think this can proceed to RTBC. We're using it on 2 widely different sites that I'm aware of. And the flag we are adding

+++ b/src/Entity/GroupContent.php
@@ -185,6 +185,7 @@ class GroupContent extends ContentEntityBase implements GroupContentInterface {
       // cannot possibly know about. Lucky for us, all of that behavior usually
...
+      $this->getEntity()->setSyncing(TRUE);

I was about to RTBC this, but then I got to thinking... we really should check that ::getEntity() returns an instanceof SynchronizableInterface. GropuContentInterface doc hints that it will return EntityInterface. Which does not union with SynchronizableInterface

Back to NW for an instanceof check.

Thanks for the endorsement @heddn!

Fair enough about the interface, it doesn't hurt to be explicit even though it's unlikely anyone would ever run into this.

Updated patch attached (which also includes the interdiff from #43).

I was just trying this patch with content moderation enabled on nodes and I got a database error from content moderation complaining about duplicate keys. Seems like calling entity->save() might be trying to reinsert a moderation state entry?

There's other weird things happening on the site in question so I wouldn't call it a clean test but I'd check to see if this causes an issue with content moderation if you haven't done so already.

The patch works well. Tested with content moderation enabled on the given node.

#45 works for me.

I am importing users from LDAP and during the user creation also assigning to the group.
Without the patch it was creating a loop of user update.

With the patch, it's fine.

Re @minorOffense: Are your able to share more details about your setup? At the very least a backtrace of the error would be helpful as well as any contrib (or custom) modules that might be doing something on node save in order to be track down if the patch is causing an issue there.

Note that #3181439: Content Moderation fatals when a moderated entity is re-saved on hook_insert() was committed to core so anyone getting duplicate entry issues with this patch, try the latest 9.5 or 10.0 dev version to see if that fixes the issue.

I presume that was also your issue @minorOffense. Sorry, I had forgotten about the core bug report when I wrote the previous comment.

Hi!

Thanks for the original patch!
I has facing a similar issue, as i have in a custom module a hook_node_update that was being called couple of times, just to notice that group was triggering the function during some relationship updates. Unfortunately, the original patch only works with v1.x and now v2.x (and 3.x) are released, making the patch incompatible.

Here is (what i think it should be fully compatible) the patch for the v2.x of the module (sorry, i have not tested the test).

I am not sure if i should change the version of the metadata to v2.x or to any other version, so I leave in 1.x for now

Hi!
Fixing a small error in the test, that was causing a fatal error.

Sorry, not familiar with tests (I hope now is fixed once for all)

The approach taken here is not sufficient for a few reasons. It seems to work around the code found in workspaces' EntityOperations class:

    /** @var \Drupal\Core\Entity\ContentEntityInterface|\Drupal\Core\Entity\EntityPublishedInterface $entity */
    if (!$entity->isNew() && !$entity->isSyncing()) {
      // Force a new revision if the entity is not replicating.
      $entity->setNewRevision(TRUE);

The issue is that this code, as the interface instructs, checks for synchronizing content, i.e.: a migration or default content when a module is first installed. We should not be abusing this by setting isSyncing() to TRUE ourselves.

Furthermore, the current patch will still break on entities that are revisionable but do not implement the synchronizable interface. An edge case, because all entity base classes implement the interface, but an important thing to note nonetheless.

Having said that, the current patch works. So I would understand if people used it until we got something more robust in place. I just can't commit it in the current state as it uses an interface for the wrong reasons.

See https://www.drupal.org/node/3052114:

Note: the concept of a syncing content entity is still being explored and defined in core. You should carefully evaluate if your use-case fits the definition of a syncing content entity before using this API. This is being discussed in more detail here: #3057483: Better describe how SynchronizableInterface should be used for content entities

//This means we may need to update access records,
// flush some caches containing the entity or perform other operations we
// cannot possibly know about.

if we go back to this comment, may i suggest to add new method to Drupal\group\Plugin\Group\Relation\GroupRelationInterface
which takes care of invoking exactly the kind of hooks required for each entity instead of calling $entity->save() ?

Hi!

I have also noted in 3.2.x the issue is still active.

When doing a basic code such as:

  $new_location = \Drupal::service('entity_type.manager')->getStorage('node')->create([my_fields]);
  $new_location->save();
  $new_event->addRelationship($new_location, 'plugin');

if the node entity contains any postSave() function, such as:

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE): void {

    // PostSave as expected.
    parent::postSave($storage, $update);

    // Do this only in an update; the insert should be done via hook too.
    if ($update) {
      // Let's do here some redirection here for example
    }
  }

}

the postSave() is executed one more time (cause the second time the $update value is TRUE) during the addRelationship() function as the following function is triggering (in GroupRelationship.php):

  /**
   * {@inheritdoc}
   */
  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->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,
      // 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();
    }
  ........

#3478088: Group relationship saving/deletion resaves entities without using ::setSyncing() is a related issue in this space.

One example that this patch would currently break, we have pathauto aliases based on the group (we only have single-group assignments), the update is necessary so that pathauto can generate the correct alias. Yes, it's definitely not optimal, because we had to implement a pathauto hook that prevents aliases (and then redirects) for content that is saved the first time.

Sounds like that project needs a custom group content/relationship insert/update hook implementation that saves the node or trigger pathauto generation.

I've just run into this issue tracking down why a post update was taking a long time and agree the saves should be removed outright. It might need to happen in a groups minor release with a change record or something.

I've just run into this issue tracking down why a post update was taking a long time and agree the saves should be removed outright.

That would be a behavioral change, which would rightfully lead to complaints from people who relied on this functionality. Having said that, I am more than willing to accommodate during this major by using the syncing flag, but as I've already mentioned in this issue I am hesitant because core has yet to define what the expectations and limitations of said flag are.

It might need to happen in a groups minor release with a change record or something.

Same as previous point, it could be considered a BC break. However, 4.x.x is about to be tagged and we can definitely fully remove it there if need be.

All in all, my personal preference would still be to keep the save, but perhaps with the syncing flag. An alternative would be to grab the target entity's cache tags and invalidate those. Now that the entity storage is using a proper cache internally, we could do that.

Either way, it would be really nice if we could get some movement/clarity on #3057483: Better describe how SynchronizableInterface should be used for content entities

Yes, this one bit me too. Bit me hard...

@kristiaanvandeneynde said:
> An alternative would be to grab the target entity's cache tags and invalidate those. Now that the entity storage is using a proper cache internally, we could do that.
> [But] That would be a behavioral change

What about the legacy-settings pattern:
- Add a setting "Re-save entity on group content create (instead of only invalidating caches)"
- Default it to falsej for new projects.
- Set it to true for legacy projects.

What about the legacy-settings pattern:
- Add a setting "Re-save entity on group content create (instead of only invalidating caches)"
- Default it to falsej for new projects.
- Set it to true for legacy projects.

Yes we've started doing this with empty modules in core + a moduleExists() call recently, but whether it's that or a new config option would allow existing sites to choose, and new sites to default to the new behaviour. And either deprecating the feature flag module or a hook_requirements() checking the config value would then make it possible to notify site owners when the option is going to be removed later.

I add #2754399: Plugin config: Delete entity along with GroupContent as a related issue due to the following comments in code:

1. GroupRelationship::preSave()(L.259-264)
   

   // 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();
   

2. GroupRelationship::postDelete()(L.295-301)
   

   // 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();
   

These first comments in the ::preSave() also show why this save operation is not as "unnecessary" as it it's implied in this issue's subject.

For those unaware, it's on the roadmap for 4.0.0 (#3493574: [Meta] Roadmap for Group 4.0.0)

If that turns out fine we could look into backporting it, but I'm not planning on that myself. The upgrade from 3 to 4 should be quite easy compared to previous major version releases, so you'd have the option to move to Group 4 if you want to stop this saving of entities.

This is now actionable: We remove the behavior from v4 and use cache tag invalidation instead. When I first wrote Group, the entity layer still used an internal property cache and I was weary of the ramifications of only invalidating cache tags so I went with the re-save.

Years later, I feel more confident in saying that if anything relies on a given entity, it should add said entity as a dependency and therefore the invalidation of said entity should trigger a recalculation on the consumer's end.

The only thing I'm slightly concerned about is hook_entity_update() and similar entity class methods as people may use that to trigger certain things when an entity is saved and said hook does not run on a simple cache tag invalidation. So if people do something there that does not somehow get cached (with the entity as a dependency), we might be in trouble.

Then again, if it persists across requests then it is cached and it should have the dependency. If it doesn't persist, then invalidating won't hurt because the next lookup of said calculations will run them anew with the entity now being recognized as grouped.

So let's go ahead and remove the old behavior in favor of cache tag invalidation.

> > An alternative would be to grab the target entity's cache tags and invalidate those. Now that the entity storage is using a proper cache internally, we could do that.

The entity cache, static or persistent does not use cache tags. It's a 1:1 relationship, we do not add cache tags as that would require an extra cache tag lookups for every cached entity you want to load. You can invalidate it, like you could already before, but I'm not even sure that's necessary or useful, as AFAIK nothing is added to that entity on load.

> The only thing I'm slightly concerned about is hook_entity_update() and similar entity class methods as people may use that to trigger certain things when an entity is saved and said hook does not run on a simple cache tag invalidation. So if people do something there that does not somehow get cached (with the entity as a dependency), we might be in trouble.

I think the primary example for this is still pathauto, which creates/updates aliases. Everyone who creates aliases based on group assignment of an entity will be affected by that as their aliases will no longer work. (Side note, #2774827: Get a token of a node's parent group to create a pathauto pattern has 100+ comments and followers, I think that's mostly for pathauto integration).

If you just remove this, this will affect the workflow for a lot of people. But I'm also well aware that this is a very expensive solution to trigger pathauto. One option might be to explicitly call the pathauto API in that case. There might be fancier options, but anything hook/event based is always a question of who implements the hook for whom, and personally, I think a factor is how common a module is. everyone uses pathauto. I can't add dozens of integrations to pathauto, but it's feasible to add a service/module check and call \Drupal::service('pathauto.generator')->updateEntityAlias($entity, 'update');

You can invalidate it, like you could already before, but I'm not even sure that's necessary or useful, as AFAIK nothing is added to that entity on load.

Right, was too quick in my previous comment. It's indeed not the entity data that triggers any changes, but the group relationship to said entity data. So we're all good on that front, you're right.

Regarding Pathauto, I see your point. Either I trigger it, or I fire an event and expect Pathauto to follow my event. Given how Pathauto has far more reported users, it might be more prudent for me to trigger Pathauto. I'll have to look into that.

Good insights, thanks.

Posted a first stab at this to see which tests fail. Also not sure about the approach and comments re Pathauto. Feels like this needs to be done in a hook or event or something, even if Pathauto won't be the one implementing it.

Okay, I've given this some thought and I'm going to move the Pathauto thingy to a hook implementation in a submodule (group_support_pathauto). I really don't like privileged code and if I were to put Pathauto stuff in GroupRelationship, then it won't be long before others start asking if they can be in there too.

That module can be part of a follow-up.

Looks good enough to me, gonna leave at NR for a few days and then commit. Will add CR now.

CR added here: https://www.drupal.org/node/3526255

Fair enough on a separate module for pathauto, do you plan to maintain that in this project or a separate? I already mentioned, but #2774827: Get a token of a node's parent group to create a pathauto pattern is IMHO a really useful issue for anyone using group specific aliases. The second thing is that we implemented a custom hook that prevents an alias if content isn't in a group yet. Which they initially on the first save always are:

/**
 * Implements hook_pathauto_alias_alter().
 */
function YOURMODULE_pathauto_alias_alter(&$alias, array &$context) {
  if (isset($context['data']['node'])) {
    // Prevent that the initial save of a node in a group creates top-level
    // alias without a group prefix.
    if (strpos($context["pattern"]->getPattern(), ':group:') !== FALSE && !GroupRelationship::loadByEntity($context['data']['node'])) {
      $alias = '';
    }
  }
}

This might be a useful feature in that integration module too, but it will likely need to be configurable per pattern instead of the token check we use. I also removed one condition we have that then overrides that logic again for one specific case.

My idea was that if the submodule is small enough, I could put it in this project. I'll have a look at the link and code you provided and try to make a decision. Thanks for the info.

i still believe the best correct solution is in #57
https://www.drupal.org/project/group/issues/2872697#comment-15542438
any custom entity which requires special operations, it should extend that specialized method in the relation interface .

Re #78:

if we go back to this comment, may i suggest to add new method to Drupal\group\Plugin\Group\Relation\GroupRelationInterface which takes care of invoking exactly the kind of hooks required for each entity instead of calling $entity->save() ?

The issue with that is that we cannot know about people's implementations of those hooks. So one might very much want us to invoke the hook, whereas the other absolutely does not want us to. Then we're back at square one where we can't do things right for everyone.

Clearing caches and then expecting people to respond to the creation of a group relationship makes a lot more sense and we will no longer be doing any harm by guessing what people want.

Clearing caches and then expecting people to respond to the creation of a group relationship makes a lot more sense

on other hand, all that the hooks implementation knows is that cache has been cleared without context about group related operation .

the purpose of new method on relationship interface is not doing one thing for everyone, but each relationship type will override that logic and adapt it to its type of data .
if there is a module out there that doesn't like the implementation, then they can create new relationship plugin which customize that method execution to their specific needs .

when you are clearing cache, you are actually saying : you don't need to know what has happened about creating new relationship instance, just go ahead an make new calculation about the underlying data you are interested in .

on other hand, all that the hooks implementation knows is that cache has been cleared without context about group related operation .

No, they would have to implement hook_group_relationship_insert(). Which tells you nothing about a cache being cleared and everything about an entity being created.

I would also strongly advise against adjusting or adding group relation plugins to achieve the desired result, as they are what power group relationship types. Those are bundles and so you'd have to be very careful with changing a plugin for another as then you'd have to update the group relationship entities' bundle, which is a total pain in the ass.

can you please explain little more about what you mean by "have to update the group relationship entities' bundle" ?

Say you have a group relation plugin called group_media_foo to enable adding media entities to a group. This plugin can be installed on a group type and by doing so a group relationship type will be created for that unique group_type-plugin combo. Any media added to groups of that type will be using a group_relationship entity of the automatically created group relationship type (aka bundle).

After a while you install pathauto and you need it to work with grouped media entities. If supporting new functionality would require a change in plugin to , say, group_media_bar, then you would not be able to switch existing relationships for media over to that plugin as it would inherently mean you'd have to swap out their bundle.

That's why group relation plugins serving as an extension point is a bad idea. Changing the plugin definition to point to the extending class also doesn't work because then only one module can ever do that.

Seems like this MR also needs to tackle similar code in ::postDelete()

Added code that also invalidates cache tags when an entity is removed from a group and expanded the test case.

Unless anyone has any further objections, I'm thinking of committing this this week.

thank you for your work, this is very good improvement .

CR looks nice, MR looks nice. Running tests once more now that Drupal 11.2 is officially out. If all is well, I will merge this in.

Assigning credit already.

Okay CI is still running 11.1, let's not have that stop us as the 4.x branch will only support 11.2 and up anyway.

Follow-up for Pathauto here: #3530980: Consider a submodule to support pathauto

I just realized we're not taking list cache tags into account here. This could become a security issue if released like this. So we need to create a follow-up that makes sure we tackle list cache tag invalidation too. The only downside is that core makes EntityBase::getListCacheTagsToInvalidate() protected. Would have been really nice to be able to just call that.

why can't we just call EntityBase::postSave ?
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...