Node update hook is triggered upon group content create

#45 works for me.

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 🐛 Content Moderation fatals when a moderated entity is re-saved on hook_insert() Fixed 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: 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work

//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.3.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
    }
  }

}

that one is triggered one more time during the addRelationship() function as the following function is triggered (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();
    }
  ........

🐛 Group relationship saving/deletion resaves entities without using ::setSyncing() Active 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 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work

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 📌 Plugin config: Delete entity along with GroupContent Needs review 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();
   

For those unaware, it's on the roadmap for 4.0.0 ( 🌱 [Meta] Roadmap for Group 4.0.0 Active )

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, 💬 Get a token of a node's parent group to create a pathauto pattern Needs review 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 💬 Get a token of a node's parent group to create a pathauto pattern Needs review 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 🐛 Node update hook is triggered upon group content create RTBC
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.