Orphan Purger not triggered when deleting revisions

A test would be useful to demonstrate this, but looks sensible and simple enough.

I created an MR based on Remco Hoeneveld patch allowing to delete orphaned targeted entities when a specific revision entity having some composite fields is deleted.

Our use case: a multi-level hierarchy of paragraphs, some paragraphs containing other paragraphs, etc.

When deleting a specific node revision containing several imbricated paragraphs revisions, those paragraphs revision entities are never automatically deleted if the patch is not applied. You need to run the orphan purger batch multiple times depending on the number of level you have in your hierarchy of paragraphs.

For those interested, here is a custom presave entity hook we implemented to add "removed" paragraphs entities to the orphan purger queue to cleanup all the new potential orphaned entities:

/**
 * Implements hook_entity_update().
 */
function mymodule_entity_presave(EntityInterface $entity) {
  if ($entity->isNew()) {
    return;
  }
  if (!$entity instanceof FieldableEntityInterface) {
    return;
  }
  $entity_type_manager = \Drupal::entityTypeManager();
  /** @var \Drupal\Core\Field\FieldTypePluginManager $field_type_manager */
  $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
  foreach ($entity->getFieldDefinitions() as $field_name => $field_definition) {
    $field_class = $field_type_manager->getPluginClass($field_definition->getType());
    if ($field_class == EntityReferenceRevisionsItem::class || is_subclass_of($field_class, EntityReferenceRevisionsItem::class)) {
      // Get the original (pre-update) entity.
      $original_entity = \Drupal::entityTypeManager()
        ->getStorage($entity->getEntityTypeId())
        ->loadUnchanged($entity->id());
      // Check for differences between the original and updated values.
      $original_values = _mymodule_get_target_ids($original_entity, $field_name);
      $updated_values = _mymodule_get_target_ids($entity, $field_name);
      $removed_values = array_diff($original_values, $updated_values);

      // Add removed entities to orphan purger queue.
      $target_entity_type_id = $field_definition->getSetting('target_type');
      foreach ($removed_values as $removed_id) {
        \Drupal::queue('entity_reference_revisions_orphan_purger')->createItem([
          'entity_id' => $removed_id,
          'entity_type_id' => $target_entity_type_id,
        ]);
      }
    }
  }
}
function _mymodule_get_target_ids(EntityInterface $entity, string $field_name) {
  return array_map(
    fn($v) => $v['target_id'] ?? NULL,
    $entity->get($field_name)->getValue()
  );
}

It works perfectly fine and allows us to keep our paragraph revisions entities volume under control without to have to run the manual batch tool periodically.

I think it should get merged.

Updated patch from MR to fix small typo in function name: refrence -> reference.

Instead of a a prefixed "internal" function, what if we already convert this to the new #Hook + #LegacyHook hook syntax? A single method can then implement both hooks on 11.1+ and the two old hooks can be marked as #LegacyHook. That's fully forward compatible and will not need to be touched anymore until D10 support is dropped.

https://www.drupal.org/node/3442349

Tried something with MR 62.

Nice, reviewed.

Thanks for the quick turnaround. Tests would be nice, but I'm OK with merging this as it is now.

Note: \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::deleteRevision() already exists, so deleting a revision should in most cases directly delete that revision, so this will add considerably extra load on deleting revisions as it will expand the queue :-/

Reopening this for now, considering to revert this because this is a considerable overhead when doing everything twice. I'd like to understand the scenario when this is needed better. One option would be to remove revisionDelete().

Hi @berdir, what do you mean exactly by "doing everything twice"?

I mean that we'd immediately delete the revision but then also register a queue item to check whether or not we should delete that revision.

So we need to figure out when and why deleteRevision() doesn't do its job, a test for that would be very helpful.

The reason I commented and re-opened is that I noticed today on a client project that a node had 5000 revisions and node_revision_delete was somehow not running properly. So I did run the manual batch, and then, knowing about this issue, did run the manual ERR orphan purger. But it barely did anything and upon closer inspection I realized that basically all the revisions already had been deleted. So it was working as it should, for me anyway.

Our use case is to be able to properly delete a specific revision of a node, not the whole node, without having to run the orphan purger.

The problem might be that be that we add several times the same entity to the orphan purger queue, possibly many times if we have a lot of revisions and we delete the node entirely.

Could it be possible to store in a request cache the entities already added to the orphan purger queue so we don't add them several times?

Ok, I think I see what you mean when you say that everything is done twice.

When running the orphan purger batch we don't need to add the processed entities to the purger queue.

Is there a way to know that the hook are being called from the batch processing?

If so, we could just skip the processing in such a situation.

Anyway, I tried to work on a static cache to avoid processing multiple times the same entity in the same request. It seems to avoid a lot of unneeded revision hook processing when running the orphan purger batch with a lot of revisions.

You can check the MR added to this issue.

The change in this issue has caused #3512911: Make Composer version requirements match the info file.

I realize this is awkward after so long, but there hasn't been a release and I'd like to better understand what problem this actually solves, so I decided to revert this. What I'd like to see per #21 is a test that fails by pointing out a revision that has not been deleted as expected without this.

Hi @berdir, could you have a look at my test in MR 76?

Looks like I was able to reproduce the bug we are encountering when deleting nested revisions.

EDIT: Actually the test is probably bogus as it still fails even with this issue's patch applied. Investigating.

EDIT2: In fact the test passes fine if you run the purger batch a second time as described in #6.

I finally wrote two tests (MR 76):

• The first one passes without the patch and demonstrates the need to manually run the purger batch multiple times, depending on the nesting level.
• The second one passes successfully with the patch applied and shows that a single cron run is sufficient to delete all orphaned revisions.

@berdir is there any thing we should add?

Thanks for sticking with this. Added some comments but didn't fully review the tests yet. You can include the fixes too, MR's have the test only job that should allow us to verify the test fails without the changes. And we can possibly also extend the test to ensure that we're not doing more queues than we have to on top of the fixes.

Re-added the fixes in MR 76.

"Test only" job is now failing as expected.

Added a static cache in the OrphanPurger queue worker to ensure that we will not reprocess already deleted entities.

BTW the EntityReferenceRevisionsOrphanRemovalTest seems to be intermittently failing on the "previous minor" test environment. Test is expecting 8 deleted revisions, we are sometimes getting 7.

Thanks for sticking with this. I've updated the comment on why those revisions of node rev2 aren't deleted (It's because those are the default revisions, so more checks are needed to verify that all revisions of the reference are unused, in case there are any.

This made me realize what I think the issue here is that you're seeing. Exactly that default revision check. I think we can do this more efficiently and avoid those extra queries on revision deletions by instead adding them to the queue in that case. With that change, your tests are passing for me without the revision delete hook.

I'd like to commit this, but the test seems to consistently fail on next minor which I can't reproduce locally and also possibly seems to make random fails happen more frequently.

I can reproduce the test fails now and it's pretty complicated. It happens in the setup when the number of revisions created diverge in 11.3 compared to 11.2.

On line 248, instead of 3/3 we now have 3/4 revisions, because B also gets an new revision through A when the node is saved. This might be a bug in core. It has to be related to entity revision cache, but I couldn't exactly pinpoint the change and reason yet.

But what's essentially happens is that when A is saved, and its field_composite_entity field is checked for hasTranslations, which then goes to \Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList::hasAffectingChanges, which checks if the target has affected changes. Because the target (composite B) isn't being saved (yet), \Drupal\Core\Entity\ContentEntityBase::hasTranslationChanges() has no preloaded original. Because a new revision for composite B was just created, but the revision referenced by this field *was* a default revision, it's comparing it against the new default revision, sees a change and then decides to resave the referenced revision.

This doesn't add up of course, since it's comparing it against the wrong version. I'm not entirely certain what we should do in this case and if core is wrong (and it should always load a revision) or if ERR should set the original in this scenario since we're explicitly checking changes on an old revision. These checks are expensive, and it again comes down to missing change tracking of a loaded entity. It's way too late to continue on this, just writing down my findings.

That's some amazing investigation work, thanks @berdir.

Worked around the problem with hasTranslationChanges(), tests are now green on all core versions. Opened #3562759: ContentEntityBase::hasTranslationChanges() must use loadRevisionUnchanged()

The move of the hook is a bit out of scope now as we're not actually changing that, but we need to do it eventually.

Merged this again, lets hope it sticks this time!

I forgot about the random fails and the test change there.

Had two random fails, but also had thata random fail on the last two weekly pipelines.

That was a tricky one. Thanks @berdir!

Hi everyone,

I ran into an issue after updating entity_reference_revisions to the latest version.

Error: Call to a member function getTranslation() on null in Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList->hasAffectingChanges() (line 188 of /app/web/modules/contrib/entity_reference_revisions/src/EntityReferenceRevisionsFieldItemList.php).

The following line seems to be the cause of the issue:

$entity = $this->entity;

In this context, $this->entity can be NULL. Since the code is iterating over referenced entities, it seems that $item->entity should be used instead.

Current code:

      // If it is the same entity, only consider it as having affecting changes
      // if the target entity itself has changes.
      if ($item->entity && $item->entity->hasTranslation($langcode)) {
        $entity = $this->entity;
        assert($entity instanceof ContentEntityInterface);
        // Ensure it is compared against the loaded revision on 11.3+.
        if (version_compare(\Drupal::VERSION, '11.2.99', '>') && !$entity->getOriginal()) {
          $storage = \Drupal::entityTypeManager()->getStorage($entity->getEntityTypeId());
          assert($storage instanceof RevisionableStorageInterface);
          $entity->setOriginal($storage->loadRevisionUnchanged($entity->getLoadedRevisionId()));
        }
        return $entity->getTranslation($langcode)->hasTranslationChanges();

Thank you, that a very awkward mistake. Could you open a new issue so we can commit it with proper context and contribution credits?

Done #3563886