Problem/Motivation

This manifested itself here: #2848298: Paragraphs entity base table left without a revision ID after replication. When duplicating a non-default revision, this leaves content data tables in an inconsistent state as the row in the entity base tables has NULL as the revision reference.

Proposed resolution

As suggested by @berdir isDefaultRevision should should also be sensitive to isNew, ensuring duplicated entities are always the default revision.

Remaining tasks

Validate, test and patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Title: Duplicated entity revisions should always be set to the default revision. » Duplicating a non-default revision should produce a default revision for a newly created entity
Status: Active » Needs review
FileSize
1.57 KB
2.05 KB
Sam152’s picture

Credit to @berdir for the fix itself.

The last submitted patch, 2: 2850022-duplicated-non-default-revisions-2_TEST_ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2850022-duplicated-non-default-revisions-2.patch, failed testing.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -312,7 +312,7 @@ public function isDefaultRevision($new_value = NULL) {
         }
    -    return $return;
    +    return $this->isNew() || $return;
       }
    

    we should add a comment

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDuplicateTest.php
    @@ -0,0 +1,47 @@
    + */
    +class EntityDuplicateTest extends EntityKernelTestBase {
    +
    +  /**
    +   * @var \Drupal\Core\Entity\ContentEntityStorageInterface
    +   */
    +  protected $entityTestRevStorage;
    +
    

    I thought we already have a dedicated test for that, but apparently not.

The test fail should be easy to fix, we juts need to mock isNew() in there.

Berdir’s picture

Actually, since this is isn't replicate specific, we might want to add unit test coverage to that test there as well, to test the different cases.

Sam152’s picture

Tried doing this so we weren't mocking methods on the system under test, but not luck, it required a lot of extra entity key/field definition/language mocking code and caused conflicts where other test methods had already mocked specific behaviors for those methods (like the hasKey method in testIsNewRevision). I can post progress on that, but this approach seems okay.

We could probably drop the kernel test entirely, but maybe it can start as the foundation for any other dedicated tests.

The last submitted patch, 8: 2850022-duplicated-non-default-revisions-8_TEST_ONLY.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDuplicateTest.php
@@ -0,0 +1,47 @@
+
+    $duplicate_first_revision = $this->entityTestRevStorage->loadRevision($first_revision_id)->createDuplicate();
+    $this->assertTrue($duplicate_first_revision->isDefaultRevision(), 'Duplicating a non-default revision creates a default revision.');
+    $this->assertEquals($duplicate_first_revision->label(), 'First Revision');
+  }

We should expand this to ensure that we can save and load the new entity again. To be sure that we don't introduce a different regression related to cloning a non-default revision in the future.

Everything else looks fine to me.

Sam152’s picture

The last submitted patch, 11: 2850022-duplicated-non-default-revisions-11_TEST_ONLY.patch, failed testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now.

Disclaimer: The actual bug fix was suggested by me, but @Sam152 wrote the patch and the tests.

  • catch committed 3bd5be0 on 8.4.x
    Issue #2850022 by Sam152, Berdir: Duplicating a non-default revision...

  • catch committed 21fae20 on 8.3.x
    Issue #2850022 by Sam152, Berdir: Duplicating a non-default revision...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.