ECA Content module breaks core workspaces

I'm not sure what the correct solution is here -- but I'm thinking that the behavior here is wrong. This is inside the update hook, after an entity has been saved -- it says it's preventing any further changes from creating another new revision. Is that necessary to prevent infinite recursion?

If not, it seems like maybe it's best to go ahead and create a new revision, since at this point the node has already been saved? Is this a concern in many models? Generally it's a really bad idea to save a node inside its own update hook -- is this too defensive to have here?

Another option would be to change the module weight to make the update hook run after Workspace's hook...

Great finding, thanks for analysing and reporting this @freelock.

Preventing new revisions should only really cover the processing by ECA when triggering the event. So, we should restore the status afterwards:

  public function entityUpdate(EntityInterface $entity): void {
    if ($entity instanceof ContentEntityInterface) {
      $isNewRevision = $entity->isNewRevision();
      if ($isNewRevision) {
        // Make sure the subsequent actions will not create another revision
        // when they save this entity again.
        $entity->setNewRevision(FALSE);
        $entity->updateLoadedRevisionId();
      }
      $this->triggerEvent->dispatchFromPlugin('content_entity:update', $entity, $this->entityTypes);
      if ($isNewRevision) {
        $entity->setNewRevision();
      }
    }
  }

Other approaches like either removing this safeguard or changing the processing order of hooks would potentially break existing models. With the approach above, we should be safe. What do you think?

Hi,

I don't think the code in #4 will fix this issue -- the issue is not with the setNewRevision(), it's with the updateLoadedRevisionId() call.

Workspaces compares the loadedRevisionId with the RevisionId. It might work if we remove the $entity->updateLoadedRevisionId(); line entirely.

I think the loadedRevisionId is meant to contain the revision_id before the entity got saved, so changing that during the update hook is the crux of the problem here.

You're right, it's the updateLoadedRevisionId call causing the issue. I was just investigating why we've put that in originally, but I can't find any trace. It's been like that since before the first alpha. Can't remember what caused us doing it, but together with the comment there must have been a specific scenario that caused an issue that we solved with it.

On the other hand, we now know that after an entity update event, ECA models shouldn't even try and save that entity again. It would cause a loop, that logs an error, and things should be done differently, when entities need to be changed by ECA models.

That said, I wonder i we shouldn't even get rid of all the revision handling in that update hook, and instead leave it to the system to deal with all of that.

We should assume that such a change could break existing models, and if we publish that, this needs to be documented in a CR.