Composite entities are not removed on host delete if reference was removed in prior revision

Yes, that's a known issue, but please provide a separate scenario for that in the test. There are several issues in paragraphs related to this.

The scenario should also test doing that with revisions and without, because the entity can only be deleted if all revisions that used it have been deleted.

See also #2771531: [META] Remove obsolete composite entities from existing storage (garbage collection)

Nice work! The database looks way cleaner with this patch when paragraphed articles are deleted. ;)

I would just add one or two nitpicks.

1. +++ b/entity_reference_revisions.module
   @@ -218,3 +220,68 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
   +    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($target_entity_type_id);
   ...
   +      $entities = \Drupal::entityTypeManager()->getStorage($target_entity_type_id)->loadMultiple($entity_ids);
   

   It would be nicer to pull out \Drupal::entityTypeManager() outside foreach loop in separate variable.

2. +++ b/entity_reference_revisions.module
   @@ -218,3 +220,68 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
   +    if ($target_entity_type->get('entity_revision_parent_type_field') && $target_entity_type->get('entity_revision_parent_id_field')) {
   +      $parent_type_field = $target_entity_type->get('entity_revision_parent_type_field');
   +      $parent_id_field = $target_entity_type->get('entity_revision_parent_id_field');
   +      $parent_name_field = $target_entity_type->get('entity_revision_parent_field_name_field');
   

   I don't know how the module works in details. So, I'm just concluding this from code => here we are checking for existence of entity_revision_parent_type_field and entity_revision_parent_id_field, but not for entity_revision_parent_field_name_field. Why not?

   And also, maybe there is no need for creating new variables.

In general, it looks good.

Rerolled patch #9

+++ b/entity_reference_revisions.module
@@ -221,6 +222,71 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
+
+  /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
+  $entity_reference_revisions_fields = entity_reference_revisions_load_field_storage_config($entity->getEntityTypeId());
+  foreach ($entity_reference_revisions_fields as $err_field) {
+    if ($entity->hasField($err_field->getName()) === FALSE) {
+      continue;
+    }

As we actually have an entity here, the usual pattern is to just do something like this:

foreach ($entity->getFieldDefinitions() as $field_name => $field_definition) {

I think the performance impact of that is minimal, it's a loop over an array of objects and a single method call on them.

if ($field_definition->getType() != 'entity_reference_revisions') {
continue;
}

Plus, it would also work in case entity types have base fields, like the paragraphs_library_item entity.

Addressing suggestions of comments above. :)

Adding test only patch again. :)

1. +++ b/entity_reference_revisions.module
   @@ -222,6 +223,45 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
   +    if ($target_entity_type->get('entity_revision_parent_type_field') && $target_entity_type->get('entity_revision_parent_id_field')) {
   +      $parent_type_field = $target_entity_type->get('entity_revision_parent_type_field');
   +      $parent_id_field = $target_entity_type->get('entity_revision_parent_id_field');
   +      $parent_name_field = $target_entity_type->get('entity_revision_parent_field_name_field');
   +      $entity_ids = \Drupal::entityQuery($target_entity_type_id)
   +        ->condition($parent_type_field, $entity->getEntityTypeId())
   +        ->condition($parent_id_field, $entity->id())
   +        ->condition($parent_name_field, $field_name)
   +        ->execute();
   +      if (empty($entity_ids)) {
   +        continue;
   +      }
   +      $entities = $entity_type_manager->getStorage($target_entity_type_id)->loadMultiple($entity_ids);
   +      foreach ($entities as $entity) {
   +        $entity->delete();
   +      }
   +    }
   

   Similar to the manual cleanup issue, weird things could happen if an entity changes its parent.

   Imagine a scenario where an entity switches the parent but then that new entity is deleted.

   So you have P(arent)1.1 referencing C(omposite)1.1 (id 1, revision 1).

   Now, for example using Paragraphs drag & drop, that composite entity is moved to a new parent P2, which is stored as a new revision, so now we have this:

   P1.1 => C1.1
   P2.1 => C1.2

   Then P2 is removing the reference again in a ne revision, resulting in P2.2 not referencing it.

   Now we delete P2, which triggers this hook which would find C1 as a referenced composite entity, but P1.1 still uses the old revision, which means we are not allowed to delete it.

   Another example could be an incorrectly cloned entity where two entities point to the same composite.

   So we would, again, need to check this revision-by-revison, but that could take a very long time. Or at least look for any other entity referencing it, also kinda slow.

   One option would be to move the actual deleting into a background queue process.

   Lets start by adding these cases as additional test coverage.

2. +++ b/tests/src/Kernel/Plugin/migrate/destination/EntityReferenceRevisionsDestinationTest.php
   @@ -32,6 +32,7 @@ class EntityReferenceRevisionsDestinationTest extends KernelTestBase implements
        'entity_reference_revisions',
        'entity_composite_relationship_test',
   +    'field',
        'user',
        'system',
        'text',
   @@ -242,7 +243,7 @@ class EntityReferenceRevisionsDestinationTest extends KernelTestBase implements
   
   @@ -242,7 +243,7 @@ class EntityReferenceRevisionsDestinationTest extends KernelTestBase implements
       * @dataProvider destinationFieldMappingDataProvider
       */
      public function testDestinationFieldMapping(array $data) {
   -    $this->enableModules(['node', 'field']);
   +    $this->enableModules(['node']);
        $this->installEntitySchema('node');
   

   this change is possibly not needed anymore now as we no longer call field.module functions.

Addressing changes from previous comments. :)

We will definitely need to create a revision checking for avoiding data lose.

Working on this. Will have a patch tomorrow. :)

First attempt with a cron approach.

Forgot the interdiff.

OK, updating tests, updating logic for adding the composite entities. :) One scenario failing since we are not yet using allRevisions in the query when adding to the queue. :) Will fix tomorrow.

1. +++ b/src/Plugin/QueueWorker/AggregatorCompositeRevisions.php
   @@ -0,0 +1,66 @@
   +    // Check the usage of data and remove when not used.
   +    /** @var \Drupal\entity_reference_revisions\EntityReferenceRevisionsOrphanPurger $purger_service */
   +    $purger_service = \Drupal::service('entity_reference_revisions.orphan_purger');
   ...
   +    $composite_type = \Drupal::entityTypeManager()->getDefinition($data['entity_type_id']);
   

   this is a plugin, so we can use createInstance() to inject this properly.

2. +++ b/src/Plugin/QueueWorker/AggregatorCompositeRevisions.php
   @@ -0,0 +1,66 @@
   +        $count = $composite_storage->getQuery()
   +          ->allRevisions()
   +          ->condition($composite_type->getKey('id'), $composite_revision->id())
   +          ->count()
   +          ->accessCheck(FALSE)
   +          ->execute();
   +        if ($count <= 1) {
   +          $composite_revision->delete();
   

   this query now also exists twice, so we could refactor it into a dedicated method as well, can also wait until the other issue is committed if that's easier.

3. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -21,6 +22,7 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
    
      use ContentTypeCreationTrait;
      use NodeCreationTrait;
   +  use CronRunTrait;
   

   this is to run cron through a http request, so we don't need that here.

4. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -420,4 +431,202 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +  /**
   +   * Tests the composite entity is not deleted when having multiple parents.
   +   *
   +   * Includes revisions on the host entity.
   +   */
   +  public function testCompositeDeleteAfterChangingParent() {
   

   Another case that we need is the reverse of this. Same until the end, but then delete the first node *first* and then run cron.

   The entity should still exist, *but* the first revision should get deleted as that one is no longer in-use.

5. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -420,4 +431,202 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +
   +    // Delete the parent of the previous revision.
   +    $node = $this->getNodeByTitle('First node');
   +    $node->delete();
   

   loading it again doesn't seem necesary, you still have $node from before.

6. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -420,4 +431,202 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +    // Test that the composite entity is not deleted when the duplicate is
   +    // deleted.
   +    $duplicate_node->delete();
   +    $composite = EntityTestCompositeRelationship::load($composite->id());
   +    $this->assertNotNull($composite);
   

   what happens if we do this *without* first deleting the reference to it?

   Does it get deleted then?

   That would be a bug and would indicate that we want to adjust the existing delete logic to also use the new queue.

   And if it doesn't get deleted at all, then we should also adjust it and add it to the queue there.

So addressing changes of previous comments.

About 6, yes, it gets deleted, so switching now to be added to the queue.

Also, fixed the first test method by running cron.

Found a problem with the tests, when creating a composite referenced by Parent1, then remove the reference in a new revision of Parent1, then create Parent2 with the revision. Both revisions of the composite get their parent id updated. Not sure how that even happens.

Both revisions of the composite get their parent id updated.

That would mean that we can not trust the parent id and all Paragraphs that have been moved across hierarchy have stale inconsistent data. This would confirm also why i experienced issues with reverting, but never figured out how to fully reproduce.

However, a follow-up could then also be to recalculate the parent field data in revisions. Otherwise we need to implement more delete-skip conditions to avoid stale references don't lead to data loss...

We just found out that the fields for the composite test entity are not revisionable. Will work on that too.

And yes, we need to recalculate parent fields, however we would need to check all parent entities with ERR fields because if there was a change of parent, the parent_type field updated all revisions so we might not know who and what type the previous parent was.

Connecting with the related Paragraphs issues.

Rerolled #28, fixed code style issues, fixed dependency inejction issues, cleanup.

This depends on #3016388: Manual cleanup process for obsolete composite entities so I am connecting it as a related issue.

Fixed the queue ID.

This patch should fix the test fails.

1. +++ b/entity_reference_revisions.module
   @@ -285,3 +287,47 @@ function entity_reference_revisions_entity_revision_create(ContentEntityInterfac
   +  foreach ($entity->getFieldDefinitions() as $field_name => $field_definition) {
   +    if ($field_definition->getType() != 'entity_reference_revisions') {
   +      continue;
   +    }
   

   Should we maybe also improve the check here to support modules extending from us?

   If so, I'm again worried about performance at this point, as we'd have to actually check every field. Maybe it's actually better to check the class and that it is an instanceof ERRItem?

2. +++ b/src/Plugin/QueueWorker/EntityReferenceRevisionsAggregator.php
   @@ -0,0 +1,131 @@
   +/**
   + * Removes composite revisions that are no longer used.
   + *
   + * @QueueWorker(
   + *   id = "entity_reference_revisions_aggregator",
   + *   title = @Translation("Entity Reference Revisions Aggregator"),
   + *   cron = {"time" = 60}
   + * )
   + */
   +class EntityReferenceRevisionsAggregator extends QueueWorkerBase implements ContainerFactoryPluginInterface {
   +
   

   strange class name/id, I guess copied from aggregator module. Maybe entity_reference_revisions_orphan_purger as ID and just OrphanPurgerWorker as class name?

3. +++ b/src/Plugin/QueueWorker/EntityReferenceRevisionsAggregator.php
   @@ -0,0 +1,131 @@
   +
   +    // Get the next batch of revision ids from the selected entity type.
   +    // @todo Entity queries on all revisions with a revision ID condition are
   +    //   not working, use a direct sql query
   +    $entity_revision_ids = $this->database->select($composite_type->getRevisionTable(), 'r')
   +      ->fields('r', [$composite_revision_key])
   

   this comment is probably copied, we don't have a batch here. Also would be good to reference the core issue.

   We're loading all revisions of a single paragraph here, so wondering if in some extreme cases with hundreds of revisions, this could be an issue, but this is cron/queue, so it should be fine if it takes a few seconds. We've seen in the batch profiling that even with blackfire on, we can process ~300 paragraph revisions in about a second.

4. +++ b/src/Plugin/QueueWorker/EntityReferenceRevisionsAggregator.php
   @@ -0,0 +1,131 @@
   +      // If this is the default revision of the composite entity, check if there
   +      // are other revisions. If there are not, delete the composite entity.
   +      if ($composite_revision->isDefaultRevision()) {
   +        $count = $composite_storage->getQuery()
   +          ->allRevisions()
   +          ->condition($composite_type->getKey('id'), $composite_revision->id())
   +          ->count()
   +          ->accessCheck(FALSE)
   +          ->execute();
   +        if ($count <= 1) {
   +          $composite_revision->delete();
   +        }
   +      }
   +      else {
   +        // Delete the revision if this is not the default one.
   +        $composite_storage->deleteRevision($composite_revision->getRevisionId());
   

   this part is almost identical to the code in \Drupal\entity_reference_revisions\EntityReferenceRevisionsOrphanPurger::deleteOrphansBatchOperation(), apart from the batch progress tracking there. Maybe we can put it in a public method on the service and return if we deleted and if just revision or the whole entity?

5. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -439,8 +452,269 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +   * Tests the composite entity is not deleted when having multiple parents.
   +   *
   +   * Includes revisions on the host entity.
   +   */
   +  public function testCompositeDeleteRevisionAfterChangingParent() {
   

   We seem to have several very similar/identical method descriptions, maybe we can improve that a bit?

Thanks for the review!

#39:

1. ✔️Fixed.
2. ✔️Fixed.
3. ✔️Improved
4. ✔️Makes sense. I extracted it to a new method on the service.
5. Hopefully improved/cleared tests.

Looks good, committed.