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.