support host revision delete

We'll need to do a query to ensure that a revision is unused. We'll also need to make sure it is not the default revision and other things.

I just told @ModernMantra to extract the relevant parts from the other issue and post failing patches so we have a starting point.

Tried to understand/get/extract the right "relevant parts from the other issue". Not sure if I'm doing it correctly.

Same with deleting with a translatable field.

We must be very careful to not delete revisions that are still used. We now enforce that, but there will still be a lot of existing content where this is not true yet. Do an entity query with allRevision() for this specific revision, only delete if you can't find any content.

We might need to do some tricks to reproduce such a case in a test, e.g. manually update a field table to use the same revision ID multiple times.

Ok, tried to reproduce the problem in the test, now it should fail (correct). Still can not figure out how to get the content revision (it is not always node ?) to compare with the field revision (not always composite_reference_target_revision_id) that is about to be deleted to check if other content revisions are referring to it. Probably need some hints.

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -323,6 +323,30 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+    $host = $this->getEntity();
+    $nodes_query = db_select('node_revision__composite_reference', 'n')
+      ->fields('n')
+      ->execute()
+      ->fetchAll();
+    var_dump($nodes_query, 'node_revision__composite_reference');
+    $entity_query = \Drupal::entityQuery('node');

As discussed, we should be doing an entity query instead, not db_select. You will need the same information, but it is available, you have the entity type and the field name and the paragraph id and revision id.

Something like \Drupal::entityQuery($entity_type_id)->condition($field_name . '.revision_id')->allRevisions()->count()->execute()

Improved entity query in deleteRevision(), but If I leave the throw exception, then the test fails. How can I catch it properly to assert that the exception is correct ?
Will continue tomorrow.

Looks like I lost my review.

Here's a shortened version for now:

* Do not throw exceptions, just don't delete.
* The default revision test doesn't seem to make sense yet, you're not really doing anything yet. What you should do load one of old revisions, make it the default revision and save it again. Then delete the node revision pointing to it. Make sure it still exists and no exception is thrown.
* Mixing all those cases together is complicated. Make two separate tests methods for the duplicate revision and default revision logic, keep the existing one and just uncomment the assertion.

Ok, separated and improved the tests and removed the exceptions.

Re-run tests since #2849136: EntityReferenceRevisionsServiceProvider should check for rest module that fixed our problem is committed.

1. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -326,6 +326,30 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
   +    if ($child->isDefaultRevision()) {
   +      // Do not delete if it is the default revision.
   +      return;
   +    }
   

   you can already do this before executing the query.

2. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -284,4 +292,111 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +  /**
   +   * Tests composite relationship with revisions.
   +   */
   

   docblock is always the same, expand it a bit:

   Tests that composite revisions are not deleted when it is the default revision.

   Tests that composite revisions if they are still in use.

3. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -284,4 +292,111 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +    $node->composite_reference->entity->setNeedsSave(TRUE);
   +    $node->composite_reference->entity->setNewRevision(TRUE);
   

   those two lines should now actually happen automatically, try removing them.

4. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -284,4 +292,111 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +    $composite_default = \Drupal::entityTypeManager()->getStorage('entity_test_composite')->loadRevision($composite_original->getRevisionId());
   +    $composite_default_revision_id = $composite_default->getrevisionId();
   +    // Check the default composite revision is the original composite revision.
   +    $this->assertEquals($composite_original_revision_id, $composite_default_revision_id);
   

   this is too complicated. you load by a revision_id to then get its revisionid? that is guaranteed to be the same, so just check for $composite_original->getRevisionId() being the same.

5. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTest.php
   @@ -284,4 +292,111 @@ class EntityReferenceRevisionsCompositeTest extends EntityKernelTestBase {
   +    $composite_default_revision = \Drupal::entityTypeManager()->getStorage('entity_test_composite')->loadRevision($composite_default_revision_id);
   +    $this->assertNotNull($composite_default_revision);
   

   lets also check that the second revision also still exists, just to be sure.

Done as suggested above. Also noticed that in the duplicated revision test I actually didn't check for the original composite revision, fixed it.

Looks ok. I'm actually not a fan of storing local properties for services in tests, injection in tests is not necessary as we will never test a test. And there's a certain risk that it will cause some side effects if you do something like install a module that rebuilds the container.