Deleting a translation deletes composite references on translatable fields

Worked on tests, also converted to a kernel test.

So, the good part is that this is not as bad as I feared. In fact, it only affects translatable fields (Note that fields by default are translatable, which is not what we expect in paragraphs and composite fields in general).

The bad part is that it *is* resulting in data loss in case of translatable fields. And I'm not sure how to deal with it properly. How do we decide when to delete and when to not delete a reference? We need to know if there are other translations that reference the same entity. It might even be in a different delta or so. And I honestly don't know how to track that.. Only idea i have right now is to disable deleting the data as a quickfix for translatable fields. Or maybe move it to hook_entity_delete(), so we can see when the entity is actually deleted.

Also something that we haven't talked about yet but might be even worse is deleting revisions. I suspect that actually fails consistently, for all kinds of fields. But probably better to do a new issue for that.

With translatable refrences, my proposal was always to start with non-shared paragraphs. Each entity would be only used in one language. Deleting the child entity would then be fine.

A mixed mode can only work if we do reference counting. However i'm not sure how much this is worth it.

Right, but how would we enforce that exactly? There is a translation_created hook but there doesn't seem to be something on the field level. So we probably need to implement hook_entity_delete(), hook_entity_translation_delete() and hook_entity_translation_create().

Also wrote tests for revisions. Also there, it is not as bad as I feared, as deleting a revision does call a separate method. Only problem is that we don't delete old revisions, but withou enforcing that we always have a unique revision per host revision, for which we have some issues, we can't really do that. So we comment that part of the test out.

Also added a check for deleting a translation when the parent deletes it.

That means we now have the following 3 failures:

* Deleting a translation on a translatable field deletes the target even though it might still be in use by other translations
* Deleting a translation on a non-translatable field does not delete the translation of the composite
* Deleting a revision does not delete the revision of the composite

Also had to make entity_test_composite actually translatable, it wasn't before. Pretty weird that you can add translations that are then silently dropped if the entity is not translatable. But you can still translate configurable fields, so that's tricky.

I started working a patch however one of the problems I came across is that the field item delete method is not called on non-translatable fields when deleting a translation on the parent.
@see \Drupal\Core\Entity\ContentEntityStorageBase::invokeFieldMethod().

Beside that my approach looks like this :

  public function delete() {
    parent::delete();
    if ($this->entity && $this->entity->getEntityType()->get('entity_revision_parent_type_field') && $this->entity->getEntityType()->get('entity_revision_parent_id_field')) {
      $child = $this->entity;
      $parent = $this->getEntity();
      if ($this->getFieldDefinition()->isTranslatable()) {
        if ($parent->isDefaultTranslation()) {
          $child->delete();
        }
        else {
          $langcode = $this->getLangcode();
          if ($child->hasTranslation($langcode)) {
            $child->removeTranslation($langcode);
            $child->save();
          }
        }
      }
      else {
        $langcodes = array_keys($parent->getTranslationLanguages());
        $original_langcodes = array_keys($parent->original->getTranslationLanguages());
        if (count($langcodes) == count($original_langcodes)) {
          $child->delete();
        }
        else {
          foreach (array_diff($original_langcodes, $langcodes) as $removed_langcode) {
            if ($child->hasTranslation($removed_langcode)) {
              $child->removeTranslation($removed_langcode);
            }
          }
          $child->save();
        }
      }
    }
  }

But I guess the else branch for a non-translatable field should be moved to the method ::preSave.

The fact that delete() on the field doesn't run for non-translatable fields actually prevents us from losing data in more cases, as we don't support translatable fields in paragraphs yet anyway.

My current plan is to do a quickfix here that simply disables deletion for translatable fields and then we open separate issues for the supporting that problem and the other issues. I imagine it will be easier to support those things from the translation_delete and similar hooks instead of tryin to do it from within the field.

I will try to provide a patch later today for the latest test in this issue.

So I have a solution and the test provided by @berdir is green now but for this to happen you have to apply the core patch from #2834384: Inconsistency: in FieldItem::delete we do not know if we are deleting the parent or only removing a translation.

Oups I've forgotten to attach the test to the patch :).

1. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -135,7 +135,8 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   -    $this->assertFalse($composite->hasTranslation('de'));
   +    // @todo Support deleting translations of a composite reference. See #2834314.
   +    //$this->assertFalse($composite->hasTranslation('de'));
   

   syntax is not consistent for the URL, also should be on a separate line as it is too long.

2. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -235,7 +237,8 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
        // Ensure that the composite revision was deleted as well.
        $composite_revision = \Drupal::entityTypeManager()->getStorage('entity_test_composite')->loadRevision($composite_original_revision_id);
   -    $this->assertNull($composite_revision);
   +    // todo Support host revision delete https://www.drupal.org/node/2771523.
   +    //$this->assertNull($composite_revision);
   

   Missing @todo ;)

Also, you can work on splitting up the different fixes from @hchonov's patch (thanks!) into the corresponding issues, at least post it as a commont or so. And once this is in, you can at least post a failing patch in each of those issues, simply by commenting out the relevant test line.

Even if we wouldn't have to depend on the core issue, some of those changes can be problematic, for example we currently don't enforce new revisions when the host gets a new one, so all you need to do is $node->setNewRevision(); $node->save(); and then delete that revision and your paragraphs are gone :) But we have an issue to enforce that, so we can wait on that.

1. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,250 @@
   +    // @todo Support deleting translations of a composite reference.
   +    // See https://www.drupal.org/node/2834314.
   +    //$this->assertFalse($composite->hasTranslation('de'));
   +
   

   See ... should be indented by two spaces. And one space before the commented out line.

2. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,250 @@
   +    // Test that the composite entity is deleted when its parent is deleted.
   +    $node->delete();
   +    $composite = EntityTestCompositeRelationship::load($composite->id());
   +    // @todo Support deletions for translatable fields https://www.drupal.org/node/2834374.
   +    // $this->assertNull($composite);
   +  }
   

   > 80 characters, should be written the same as the one above, with See .. on separate line

3. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,250 @@
   +    // @todo Enforce this when saving a new revision.
   

   This @todo can be linked to #2801321: New host revisions do not always create new composite entity revisions, again, like the others.

4. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,250 @@
   +    // @todo Support host revision delete https://www.drupal.org/node/2771523.
   +    //$this->assertNull($composite_revision);
   

   and.. same here re formatting.

Those are all only comment changes, so can also be fixed by miro before commit. I think we have all the follow-ups now and we can then move @hchonov's fixes to the respective issues.

Committed with comment fixes as requested. Plus added/improved some more comment.