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 looks good. I think I realized we couldn't not make new revision at some point but forgot to remove this.

the only case where we could not make a new revision is when there aren't any previous revisions of the layout entity. Actually I guess if there no revisions where the layout field is not null. Not sure if make sense to check that. But if you have an entity that supports revision but you don't ever create new revisions then if you make 100 new changes to the layout(no revisions) we still make 100 revisions of the inline block. But the 99 revisions wouldn't be reference anywhere.

But if you have an entity that supports revision but you don't ever create new revisions then if you make 100 new changes to the layout(no revisions) we still make 100 revisions of the inline block. But the 99 revisions wouldn't be reference anywhere.

Correct, but to assert that we would need to look at all revisions , to ensure none of them has that revision ID, otherwise we would introduce a data loss.

Correct, but to assert that we would need to look at all revisions , to ensure none of them has that revision ID, otherwise we would introduce a data loss.

Well if we only to cover the case where there NO revisions with any layout data we just have to use and entity query to make sure there are no other revision where the layout field is not NULL. We don't actually have to look at the field value in that case.

Do you think that belongs in a follow-up, since it's adding new behavior?

Sure. Created #3054886: Do not create new revisions for inline blocks when there are no previous revisions

We could add an assertion to \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest::testInlineBlocksRevisioning() that when a block is not edited(it won't be serialized) then a new revision is not created.

Bump

Bump again. Not being able to revert is causing some major pains for clients.

Bump, the revert issue is effecting me also.

Assigning to @tedbow per #12

@Sam152 sorry it took me so long to get to this.
Just a couple of

1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -287,8 +287,10 @@ public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE
   +    // Since the custom block is only set if it was unserialized, the flag will
   +    // only effect blocks which were modified or serialized originally.
        if ($block) {
   

   I think this comment should be moved inside the if statement. Otherwise it seems to ignore the case where $block is set because of the duplicate block logic above this.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -258,6 +258,94 @@ public function testInlineBlocksRevisioning() {
   +      'administer node fields',
   +      'administer nodes',
   

   For this test the user doesn't actually edit the node only the layout. So they don't need these permission.

   Wonder if this would be more clear if we used 'access content' and 'view all revisions' instead.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
@@ -263,6 +263,93 @@ public function testInlineBlocksRevisioning() {
+      'bypass node access',

Sorry my dreditor comment was off we don't need 'bypass node access' can be replace with 'access content'

Otherwise looks good 👍

sorry wrong interdiff

This looks good. Thanks for the patch @Lal_
Thanks for all the work @Sam152!

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -163,20 +163,12 @@ public function handlePreSave(EntityInterface $entity) {
   +      // Since multiple parent entity revisions may reference common block
   +      // revisions, when a block is modified, it must always result in the
   +      // creation of a new block revision.
   +      $new_revision_if_modified = $entity instanceof RevisionableInterface;
          foreach ($this->getInlineBlockComponents($sections) as $component) {
   -        $this->saveInlineBlockComponent($entity, $component, $new_revision, $duplicate_blocks);
   

   can we just inline this while we're here?

   ie

   $this->saveInlineBlockComponent($entity, $component, $entity instanceof RevisionableInterface, $duplicate_blocks);
   

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -251,16 +243,16 @@ protected function getBlockIdsForRevisionIds(array $revision_ids) {
   -   * @param bool $new_revision
   -   *   Whether a new revision of the block should be created.
   +   * @param bool $new_revision_if_modified
   +   *   Whether a new revision of the block should be created when modified.
   ...
   -  protected function saveInlineBlockComponent(EntityInterface $entity, SectionComponent $component, $new_revision, $duplicate_blocks) {
   +  protected function saveInlineBlockComponent(EntityInterface $entity, SectionComponent $component, $new_revision_if_modified, $duplicate_blocks) {
   ...
   -    $plugin->saveBlockContent($new_revision, $duplicate_blocks);
   +    $plugin->saveBlockContent($new_revision_if_modified, $duplicate_blocks);
   

   This change of variable name looks out of scope - is there a reason we're doing so here? Can we just improve the doc-block to make it clear that it's 'if modified'?

3. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -262,12 +262,12 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -   * @param bool $new_revision
   -   *   Whether to create new revision.
   +   * @param bool $new_revision_if_modified
   +   *   Whether to create new revision, if the block was modified.
   ...
   -  public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE) {
   +  public function saveBlockContent($new_revision_if_modified = FALSE, $duplicate_block = FALSE) {
   
   @@ -283,7 +283,9 @@ public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE
   -      if ($new_revision) {
   ...
   +      if ($new_revision_if_modified) {
   

   same here

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -263,6 +263,93 @@ public function testInlineBlocksRevisioning() {
   +    $this->assertNodeRevisionContent(4, ['Block 1 updated', 'Block 2 updated']);
   ...
   +    $this->assertNodeRevisionContent(4, ['Block 1 updated', 'Block 2 updated']);
   

   We assert this twice without any changes? Is one a duplicate?

Fixes #27, agreed that renaming the vars is not necessary.

@acbramley thanks for the patch.

re #28

1. Inlining this and not have the comment before assigning $new_revision_if_modified actually makes the comment above it confusing. The specifically refers to the what we are sending to the vrar we will send to \Drupal\layout_builder\InlineBlockEntityOperations::saveInlineBlockComponent() for the $new_revision argument.

   but now the comment is before the loop and it is not clear what is referring to.

   Also won't this be less performant anyways to do the $entity instanceof RevisionableInterface test multiple times instead of just once. We don't know how many will be returned from $this->getInlineBlockComponents($sections)
   But performance is probably a small issue. I think having it separate for the comment is more important.

   If others don't think the comment is now confusing thats fine.

Other than that looks good

Fixes #29. Agreed with @tedbow that inlining that argument doesn't make sense given the comment above specifically relates to it, and it avoids reevaluating it inside the foreach.

@fenstrat thanks for the update. RTBC🎉

Committed 109fe60 and pushed to 9.1.x. Thanks!

Thanks all for seeing this one through