ContentEntityBase::createDuplicate() should reset default revision flag

Oh, even weirder. The test already does save multiple times, but only tests with a base field. The bug that we see with paragraphs happens only with configurable fields, because dedicated table *reads* are different.

Drupal reads base fields also from the revision table too, and since that is updated, the test is happy. But if actually check the database, you can see that the base table has _not_ been updated in that test.

I made the test more explicit by checking the new/default revision flags explicitly and also doing an entity query to check the base table values.

I wonder if we can somehow detect if you attempt to save the default revision as not-the-default revision and throw an exception then or something because that is not a valid thing to do.

The last submitted patch, 2: 3220784-default-revision-duplicate.patch, failed testing. View results →

Adding access check.

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

The last submitted patch, 5: 3220784-default-revision-duplicate-5-test-only.patch, failed testing. View results →

triggering D10 tests

Tests pass D10

The tests do an excellent job showing the issue.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1138,7 +1138,11 @@ public function createDuplicate() {
+    // Explicitly mark the entity as new and default revision. A new entity is
+    // always the default revision, but that persists only until the entity
+    // is saved.

s/and/and the/

I am not following the second sentence. That a new entity, once saved is no longer the default revision? Then what is the default revision for a newly saved entity?

Let's get this testing on a supported version of Drupal.

> I am not following the second sentence. That a new entity, once saved is no longer the default revision? Then what is the default revision for a newly saved entity?

That's the point, there is no default revision anymore the in the loaded entity object.

The override that it is the default revision is transient and goes away as soon as it's saved:

  public function isDefaultRevision($new_value = NULL) {
    $return = $this->isDefaultRevision;
    if (isset($new_value)) {
      $this->isDefaultRevision = (bool) $new_value;
    }
    // New entities should always ensure at least one default revision exists,
    // creating an entity without a default revision is an invalid state.
    return $this->isNew() || $return;
  }

So if it's unsaved, it's always default, but as soon as isNew() returns FALSE, it suddenly might no longer be, and if you save it again, you end up with an invalid state.

I tried to rephrase the comment.

oy sorry about the patch name, the .patch is not an interdiff

@Berdir asked me in #contribute to review the comment. I still found it hard to follow, and in some ways the changes made it harder. I chatted with Berdir and he kindly explained what was happening. I now 'get it' and think the original comment make sense.

I am re-uploading the patch in #5 and testing with 11.x.

Sorry for delaying this!

The patch I read and commented on in #12 was RTBC. That is the patch I re-uploaded. Since it is passing tests and there has been no changes to the patch, I am restoring the RTBC.

Thanks for opening this, and fixing it!

However, #17's duplicate of #5 didn't fix the first (valid) point from #12. Leaving RTBC since it's a trivial comment nit. Interdiff also seems a bit confused, and is showing a bigger change than I actually made, which is just:

-    // Explicitly mark the entity as new and default revision. A new entity is
-    // always the default revision, but that persists only until the entity
+    // Explicitly mark the entity as new and the default revision. A new entity
+    // is always the default revision, but that persists only until the entity

The last submitted patch, 19: 3220784-19.default-revision.patch, failed testing. View results →

> I wonder if we can somehow detect if you attempt to save the default revision as not-the-default revision and throw an exception then or something because that is not a valid thing to do.

good thinking, past-me! current-me discovered this again and opened an issue for it: 🐛 Disallow saving the current default revision as a non-default revision Active . This is blocking that now.

This was RTBC and somehow fell off the bandwagon, lets try to finalize it. Still applies, not sure why it was needs work, random fail maybe?

Merge request is green and identical to the patch that was RTBC.

Not sure if we want to keep the explicit test coverage with the direct query, as with the issue that this is blocking, saving would result in an exception instead, we're kind of testing that issue, not this.

Let's add the test coverage here and then consider modifying it in the other issue if we need to.

Committed/pushed to 11.x and cherry-picked to 11.1.x, 10.5.x, 10.4.x, thanks!

catch → closed merge request !10907