Track Layout override revisions on entities which support revisioning

This wasn't marked for review but I tested it anyway in order to write out some instructions for testing. I did the following:

• Enabled layout modules
• Used "Basic page" content type since it's already configured for revisions
• Created a node with title and body text
• For the layout, only added a block with the body
• Edited the node and update the node body text so a new revision was created
• Updated the layout so the content type block was shown below body block
• Reverted back to the original version of the node
• Confirmed that the layout went back to just showing the body
• Reverted back to the 2nd version of the node
• Confirmed that the layout went back to showing the body and content type

@EclipseGc It worked exactly like I thought it would so it makes sense to me from a UX perspective. The patch code is readable and I understand you want to make it "more correct" but I don't know what would make it "more correct" ;)

https://www.drupal.org/node/2936357 recently changed the code for revisions

Here's a rerolled patch from #2 taking the new API into consideration and changes done since. Still needs tests, and everything else required to complete this issue.

Looking at this again, especially when working with inline blocks (https://www.drupal.org/project/drupal/issues/2957425 or serialized one). This one makes much sense.
If enforcing a new revision is bad, can we make some UX changes to allow for saving with a new revision?

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,6 +232,14 @@ public function label() {
+    $bundle = $this->entityTypeManager->getStorage($bundle_entity_type)->load($this->getEntity()->bundle());

bundle could be not loadable entity
This code will fail for user entity

1. Fixed #12 include test for user

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,6 +232,14 @@ public function label() {
+      /** @var ContentEntityStorageInterface $storage */

@var should use full space

2. Added tests

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,7 +231,18 @@ public function label() {
-    return $this->getEntity()->save();
+    $entity = $this->getEntity();
+    if ($entity->getEntityType()->isRevisionable() && $bundle_entity_type = $this->getEntity()->getEntityType()->getBundleEntityType()) {
+      $bundle = $this->entityTypeManager->getStorage($bundle_entity_type)->load($entity->bundle());
+      if ($bundle instanceof RevisionableEntityBundleInterface && $bundle->shouldCreateNewRevision()) {
+        /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
+        $storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
+        $revision = $storage->createRevision($entity);
+        return $revision->save();
+      }
+    }
+
+    return $entity->save();

There is no way this is reasonable for modules to need to do themselves. I think we need an issue like #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing and an @todo here pointing to it.

Added @todo pointing to https://www.drupal.org/project/drupal/issues/2942907

Setting this back to NR because the fails in patch in earlier comment was due to #2973992: Permission issue in Nightwatch step marks all full testruns as unstable

Also retest the patch.

Patch in #16 looks good to me. Test coverage makes sense and I think is comprehensive enough. All coding standards check out. Manual testing is working as expected - changes to the layout are triggering revisions, and reverting back to a previous revision is restoring the previous layout. Looks like everything is well commented and readable.

I am guessing this was random unrelated test failure: "Drupal\Tests\quickedit\FunctionalJavascript\FieldTest"

retesting

Yep unrelated random test failure. Patch is passing again.

Not sure why this is Needs work because 2937199-16.patch looks like it did pass.

Just got a chance to review this, had a couple suggestions and questions.

1. --- a/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   
   @@ -230,7 +231,20 @@ public function label() {
      public function save() {
   
   @@ -0,0 +1,107 @@
   +class RevisionsTest extends BrowserTestBase {
   

   This code is in an API method, why is it being testing with a BrowserTestBase?

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -230,7 +231,20 @@ public function label() {
   +    $entity = $this->getEntity();
   ...
   +    if ($entity->getEntityType()->isRevisionable() && $bundle_entity_type = $this->getEntity()->getEntityType()->getBundleEntityType()) {
   

   $this->getEntity() is in here twice. Not a big deal, but it keeps jumping out at me :)

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -230,7 +231,20 @@ public function label() {
   +    // @todo Request editable revision when API provided by entity API.
   +    // @see https://www.drupal.org/node/2942907
   

   @see is invalid inline, and I think the comment could be rewritten:

       @todo Replace this specific implementation with calls to the generic API
         from https://www.drupal.org/node/2942907.
   

4. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,107 @@
   +    // Create two nodes.
   

   These are bundles (node types), not nodes

5. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,107 @@
   +  public function testRevisions() {
   +
   +    $this->drupalLogin($this->drupalCreateUser([
   

   Extra blank line

6. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,107 @@
   +    /** @var \Drupal\node\NodeStorageInterface $node_storage */
   +    $node_storage = $this->container->get('entity_type.manager')->getStorage('node');
   ...
   +    $revision_id = $node_storage->getLatestRevisionId($revision_node->id());
   

   Nitpick, move this variable declaration down to where it's used

7. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,107 @@
   +    $revision_id = $node_storage->getLatestRevisionId($revision_node->id());
   +    $this->saveLayoutOverride($revision_node);
   +    $this->assertGreaterThan($revision_id, $node_storage->getLatestRevisionId($revision_node->id()));
   +
   +    $revision_id = $node_storage->getLatestRevisionId($no_revisions_node->id());
   +    $this->saveLayoutOverride($revision_node);
   +    $this->assertEquals($revision_id, $node_storage->getLatestRevisionId($no_revisions_node->id()));
   

   The reuse of $revision_id for both $revision_node and $no_revisions_node is confusing. Also, this could use some comments to explain why these two are different

8. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,107 @@
   +    // Save an override on a non-revisionable entity.
   +    $this->saveLayoutOverride(User::load(1));
   

   What are we actually asserting here? The saveLayoutOverride helper doesn't do that much on it's own.

1. added \Drupal\Tests\layout_builder\Unit\OverridesSectionStorageTest::testSave
2. fixed
3. fixed
4. fixed
5. fixed
6. fixed.
7. used more descriptive variable names and added comments.
8. I wanted to make sure that the logic didn't assume the entity is revisionable.
i.e. call

$storage = $this->entityTypeManager->getStorage($entity_type->id());
$revision = $storage->createRevision($entity);

saveLayoutOverride() checks the confirmation message which would not be the case if there was error. But maybe we don't need this now \Drupal\Tests\layout_builder\Unit\OverridesSectionStorageTest::testSave is added.

Thanks for all the changes! One more thought

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,7 +231,21 @@ public function label() {
+    $entity_type = $entity->getEntityType();
+    // @todo Replace this specific implementation with calls to the generic API
+    // from https://www.drupal.org/node/2942907.
+    if ($entity_type->isRevisionable() && $bundle_entity_type = $entity_type->getBundleEntityType()) {
+      $bundle = $this->entityTypeManager->getStorage($bundle_entity_type)->load($entity->bundle());
+      if ($bundle instanceof RevisionableEntityBundleInterface && $bundle->shouldCreateNewRevision()) {
+        /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
+        $storage = $this->entityTypeManager->getStorage($entity_type->id());
+        $revision = $storage->createRevision($entity);
+        return $revision->save();
+      }
+    }

This is really precise and complex bundle/revision logic that LB should have no knowledge of (hence the @todo).

Can this patch provide methods getNewRevisionDefault() and getBundleEntity(), and @see the implementations on \Drupal\Core\Entity\ContentEntityForm? I think it would be much clearer that way, and easier to clean up in the long run.

Also, nit: every line after the first in an @todo should be indented an additionally 2 spaces

@tim.plunkett good suggestion. Done

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -228,7 +229,56 @@ public function label() {
   +    $entity = $this->getEntity();
   ...
   +    return $entity->save();
   

   This could be a one-liner now.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -228,7 +229,56 @@ public function label() {
   +   *   generic API is available. Currently this method copied from
   ...
   +   *   generic API is available. Currently this method copied from
   

   Do we really need to mention these are copied from there?

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -228,7 +229,56 @@ public function label() {
   +    $entity = $this->getEntity();
   +    if ($this->getNewRevisionDefault()) {
   +      /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
   +      $storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
   +      $revision = $storage->createRevision($entity);
   

   We have to use the entity 2 other times in the method so creating a local variable makes sense I think

2. I am not sure I was responding to @tim.plunkett's suggestion in #29. @tim.plunkett is this normally done?

#32.1 Yeah you're absolutely right, I totally over-looked that. Sorry :P

I agree with @tedbow on both counts.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,21 +230,57 @@ public function label() {
   public function save() {
     $entity = $this->getEntity();
...
-    // @todo Replace this specific implementation with calls to the generic API
-    // from https://www.drupal.org/node/2942907.
...
+    if ($this->getNewRevisionDefault()) {
+      /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */

Can we keep an @todo here too?

Test failure is in InlineBlockPrivateFilesTest::testPrivateFiles, uh oh.

re #35 added back the todo.

Test failure is in InlineBlockPrivateFilesTest::testPrivateFiles, uh oh.

uh oh indeed

for anyone interested see security problem documented here #2957425-239: Allow the inline creation of non-reusable Custom Blocks in the layout builder

Now that the above issue was recommitted with the fix, this looks to be ready!

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -228,7 +229,58 @@ public function label() {
   +   * Should new revisions created on default.
   

   nit: language is off here, think the word 'be' might be missing

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -228,7 +229,58 @@ public function label() {
   +      $new_revision_default = $bundle_entity->shouldCreateNewRevision();
   

   we could return here and avoid the local $new_revision_default variable, with a return FALSE later

3. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,115 @@
   +    // Ensure that saving a layout for a node of the bundle that does save a new
   +    // revision by default does create a new revision.
   

   the two uses of `does` here make this confusing to read, can we improve it?

   Suggest:

   Ensure that saving a layout for a node where the bundle defaults to saving new revisions causes a new revision to be saved.

   or something.

4. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,115 @@
   +    // Ensure that saving an entity type that does not revisionable does not
   +    // have an error.
   

   this comment needs work

5. +++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
   @@ -0,0 +1,115 @@
   +   * @throws \Behat\Mink\Exception\ResponseTextException
   +   * @throws \Drupal\Core\Entity\EntityMalformedException
   

   do we need these?

6. +++ b/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
   @@ -387,4 +391,88 @@ public function testBuildRoutes() {
   +    $entity_type = $this->prophesize(EntityTypeInterface::class);
   

   am I the only one that things this test is highly contrived because of the amount of mocking required?

I've also asked some folks with better knowledge of revision api than I have to give it a review

@larowlan thanks for the review

1. fixed
2. Fixed
3. Suggestion looks good. Fixed
4. fixed
5. removed.
6. I don't it shows that ::save() respects the entity type for whether there is bundle available and the bundle for whether a new revision is the default.

+++ b/core/modules/layout_builder/tests/src/Functional/RevisionsTest.php
@@ -0,0 +1,112 @@
+    $this->assertGreaterThan($revision_id_before_new_revision, $node_storage->getLatestRevisionId($revision_node->id()));

This is where #43 failed. The difference that makes it fail is not actually anything in this issue but rather #2983887: Add static cache to \Drupal\Core\Entity\ContentEntityStorageBase::getLatest*RevisionId()

Basically before we could call $node_storage->getLatestRevisionId($revision_node->id()) before and we would always get the latest revision.

But now have to call $node_storage->resetCache([$revision_node->id()]); to make sure we get that latest revision.

I am actually wondering if #2983887: Add static cache to \Drupal\Core\Entity\ContentEntityStorageBase::getLatest*RevisionId() is actually a BC break because if a class gets the storage handler for an entity type \Drupal\Core\Entity\EntityTypeManager::getHandler() is going to give me an existing instance if another class has asked for the storage handler.

UPDATE: Actually @berdir pointed out that \Drupal\Core\Entity\EntityStorageBase::doPostSave() resets the cache and this is only a problem in tests.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -267,7 +268,57 @@ public function label() {
+  protected function getNewRevisionDefault() {

It is not a getter so the method should be named better.

@jibran

This function was added because of @tim.plunkett's comment in #29 and are copies of \Drupal\Core\Entity\ContentEntityForm. They will be removed in #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

For what it's worth, the proposal in #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features subverts the need for this and also provides the same options content admins already have elsewhere: the option to create a new revision and the ability to write a revision log message.

Let's postpone for now on that entityform approach.

Now that #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features has been committed. We should be able to mark this issue as Closed.

Manual test:

1. Installed Drupal 8.7.x.
2. Installed Layout Builder and Enabled it and overrides on Basic page.
3. Created a new node.
4. Updated the Layout override and Saved. A new revision was made.
5. It's possible to revert back.

However, unticking "Create a new revision" in the Layout Builder UI still creates a new revision, which perhaps deserves its own issue? The timestamp on the new revision is also incorrect.

I created: #3033516: Revision log message field missing from layout overrides entity form.

I tried the steps in #50 with the very tip of 8.7.x HEAD. Unchecking the box and saving the layout did not create additional revisions. So I'm not sure this issue, or #3033516: Revision log message field missing from layout overrides entity form, is needed anymore...

I checked out 8.7.x to #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API and gave this another quick manual test. I still couldn't reproduce a problem; if unchecked the "Create new revision" checkbox, no new revision was created. If I left the checkbox checked, then a new revision was created. That seems like expected behavior...

Me and phenapromixa manually tested the concerns in #50 which seems to work as expected, which means we can now close this issue.

This issue has an @todo in code pointing to it, with what looks like a temporary workaround that was added in #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder.
Reopening for more discussion, or to remove the @todo pointing here.

Correct Tim!

We always make a new revision on the inline blocks, regardless if the parent entity is saved with a new revision or not. Let's see what happens now when we update the code to do what was proposed in the todo.

Patch looks great to me. I assume this is NW for tests? I imagine we'll need to test both the scenarios where new inline block revisions are created as a result of the parent creating a new revision and the opposite where a new revision is not created.

Hm, I've figured out a bunch of reasons why #56 would lead to rewriting the revision history of an item of content in #3053881: Reverting entity revisions that contain custom blocks erroneously triggers EntityChangedConstraint.

So I think this actually needs to go the opposite way, new revisions need to be enforced on all custom blocks that are modified as part of an entity save, regardless of the parent entity and tests should be introduced to enforce that.

Anything short of that will compromise the integrity of content revisions and essentially cause data loss.

Since this issue has been repurposes a couple of times already, I've spun off #3053887: Add test coverage and document why inline blocks require a new revision to be created when modified, regardless of whether a new revision of the parent has been created to add appropriate test coverage which would have marked #56 as red.

I think based on the consensus in #3053887: Add test coverage and document why inline blocks require a new revision to be created when modified, regardless of whether a new revision of the parent has been created, this is a won't-fix?

There's a link to this issue in the codebase, we need to remove that

Re-opening per #61

Nevermind, the patch in #3053887: Add test coverage and document why inline blocks require a new revision to be created when modified, regardless of whether a new revision of the parent has been created will remove it.