Reverting entity revisions that contain custom blocks erroneously triggers EntityChangedConstraint

Adding related issues

I ran into a similar issue in #2946333: Allow synced Layout override Translations: translating labels and inline blocks because editing a translation of layout and editing the inline block would cause a similar problem. In that case the inline blocks are also translations and when you create a translation you create a new revision of the block.

In that case I am trying to solve it in the issue with this #3052042: If an inline block has been edited outside of layout builder it can't be edited in layout builder

If we take the approach in this issue and we edit the previous revision of the block would we lose the translation in the new revision?

I closed #3087882: Impossible to update inline blocks after reverting to a previous revision as a newer duplicate of this issue.

In summary, here's the direction that discussion on that issue was headed:

When a revisionable a host entity is reverted, new revisions of its inline blocks are also created with those block revisions being referenced by the entity's layout builder field.

I think this is better classified as a major, no? This bug prevents editors from editing a block's contents in LB after reverting the parent entity.

Re #21, agreed. The repercussions are significant, and there's no viable workaround.

Per #9, this is postponed.

This is extremely easy to reproduce as per the test

1. Create a node and add a basic block to it via LB
2. Create a new revision, and edit the block
3. Revert to the first revision via the Revisions screen
4. Try to edit the block again, on save you will trigger the constraint and get the
   

   The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.
   

   error

I ran into the same issue as acbramely after my latest round of drupal and 3rd party module updates, but it was happening with inline paragraph blocks as well. Patch #11 seems to work to solve the problem for me.

I have the same issue as described in #24 (thanks @acbramley).
And the patch from #11 fixed my issue (thanks @sam152)!

Adding a patch for 8.7.14 that works to opt the inline custom blocks out of the entity constraint validation.

#11 applied and resolved the issue for me. Thanks!

Rerolled against 9.1.x, BlockContentUpdateTest was removed as part of #3087644: Remove Drupal 8 updates up to and including 88** so this adds it back again.

(#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 was the blocker per #9, and is now fixed)

I readed #5, but don't know why no set a changed time when InlineBlock::blockValidate(), it will not affect any behavior of the current system.

#34 has resolved issue with "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved." message when saving inline block in Layout Builder. Thanks.

#34 worked for me, thank-you!

#34 is fixing the problem for me, too.

Now that I look at it more closely I do agree with Sam152...however I'm going to keep using #34 until #30 lands in order to avoid introducing an update hook via a patch.

#34 is working for me.

#34 resolved the issue while using Drupal 8.9.x

#34 fails to apply after updating to Drupal 9.1.x

Reroll of #34 for 9.1.x (while #30 is developed as per Sam152 #39 & azinck #40).

I added this patch to be working with
https://www.drupal.org/project/drupal/issues/3049332

As Odai Atieh mentioned in #43, Drupal Issue #3053881 conflicts with #3049332 as both attempt to modify InlineBlock's constructor signature.

Unfortunately, #43 fails to apply using Drupal 9.1.4

I've combined #3053881's #34 with #3049332's #56 here in #44. Alternatively, #3049332's older #33 patch works cohesively with #3053881's #34 on its own, as it does not add LoggerInterface to InlineBlock's constructor's signature.

> Unable to provide an interdiff between #43 and #44 due to "interdiff: Whitespace damage detected in input" error, however attached is an interdiff between my previous #42 patch and #44.

>> I'll mention the conflict between these two issues' solutions in #3049332 as well.

Oops, fixed "Parameter comment indentation" error from failed tests in #44

Note, as mentioned in #44, this patch combines #3053881's #42 (aka #34 for D9) with #3049332's #56

Thank you for all the patches, but for my scenario (content blocks nested in content block), none of the patches worked, unfortunately.

But the approach in #30 works nicely for 9.1.9, with these changes:
1) removed the whole "block_content_update_8700" function
2) removed the related "BlockContentUpdateTest" class
3) reversed the "BlockContentEntityChangedConstraintValidator" condition logic, so instead of:
if ($entity instanceof BlockContentInterface && !$entity->isReusable()) {
I used:
if ($entity instanceof BlockContentInterface && $entity->isReusable()) {

I'm not sure if this is the best way, but at least it solves the urgent situation.

#47 did not work for me. Reverting the condition logic reversal in core/modules/block_content/src/Plugin/Validation/Constraint/BlockContentEntityChangedConstraintValidator.php does seem to work, however. I think this brings that logic back to what it was in #30.

I would guess the update hook in #30 is necessary, but leaving it out doesn't seem to affect things on my installation, and my status page is clear of any entity definition mismatch warnings. Unfortunately I don't have time to research this area any further at the moment - just sharing my experience.

New patch attached. It's essentially the patch from #47 with that one small logic change (see the interdiff).

#49 Not worked for me.

So from my experience #42 works if anyone is wanting to patch a 9.2 site, I think that all patches after this would require applying multiple patches as they were modified to fit with other in-progress 9.3 changes.

Sorry if I'm repeating #51 but which patch is addressing the issue here? Is it #42? The other patches seem to be including other tickets are they needed?

Have the issue where a user is unable to save their inline blocks and #42 seems to initially work. #49 does not but I don't know if it's because of the additional changes it includes.

So some clarity would be greatly appreciated!

Using #45 and it seems to be working so far.

As mentioned in #53, the patch provided at #45 works as expected, backporting the same to 8.9.x version as well.

Trying to get this back on track with Sam's original approach. Updated the issue summary as such.

Here's the patch from #30, unchanged except for comments added which I think were needed. This is Sam's original approach. Also uploading a test-only patch to prove that the test is valid.

Updated issue summary to describe the alternate approach that is represented in most of the more recent patches. I agree with #39 and #40 that it's not the best approach.

Removed unused use statement.

Changes since #60:

1. Switched the upgrade path test fixture from 8.8.0 to 9.0.0.
2. Resolved drupalPostForm deprecations in \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest

This should fix the upgrade path test + deprecations in #60...but can someone explain why we need an update hook implementation for this? On the surface it seems that an entity type alter should do the trick? Is it simply to trigger a cache rebuild?

I'm not entirely convinced that the update test is really testing what it's intended to.

Thanks! :-)

but can someone explain why we need an update hook implementation for this

You know what, I think you're right. Alter hooks don't require an update hook to solidify what they're doing, they're altering existing definitions. We should instead just have an empty post-update hook to clear caches as you say. That means we can remove the update path test entirely!

Would be good for someone else to confirm.

I think update hooks are only needed for entity-type changes that impact the database.

So I think what @acbramley is saying is correct.

You can alter an entity-type to add a new constraint without needing an update hook.

Thanks for the guidance! Hopefully a less risky fix can accelerate the resolution of this nasty little bug.

Add an empty post_update() hook to trigger a cache rebuild.

/**
 * Updates the entity changed constraint implementation.
 */
function block_content_post_update_entity_changed_constraint() {

}

I think that the test fails in #68 may be red herrings. I see a similar "random" fail in the latest patch in https://www.drupal.org/project/drupal/issues/3192234 (an unrelated issue).

Tested the 3053881-68.patch with Drupal 9.3.x
Thank you Luke

Update I.S. to remove data model change.

This is data-loss-like so promoting to critical.

Looks good to me, haven't manually tested yet though.

Super-nit:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
@@ -662,4 +662,46 @@ class InlineBlockTest extends InlineBlockTestBase {
+    $this->drupalLogin($this->drupalCreateUser([
+      'access contextual links',
+      'configure any layout',
+      'administer node display',
+      'administer node fields',
+      'administer nodes',
+      'bypass node access',
+      'create and edit custom blocks',
+    ]));
+    $this->drupalGet(static::FIELD_UI_PREFIX . '/display/default');
+    $this->submitForm(
+      ['layout[enabled]' => TRUE, 'layout[allow_custom]' => TRUE],
+      'Save'
+    );

Can we do this via the API instead?

Saves an extra 2 HTTP requests in the test.

$display = \Drupal::service('entity_display.repository')->getViewDisplay('node', 'bundle_with_section_field');
assert($display instanceof \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay);
$display->enableLayoutBuilder()->setOverridable()->save();

- Manually tested the issue with the test steps from issue summary.
- Issue has been resolved with the latest patch from #68
- Before Patch:
- After Patch: https://nimb.ws/vawIBA

Can be marked as RTBC after question from #79 are resolved.

Made the requested change in #79.

Remove unneeded assertion.

Cleaned up IS.

Sam's question from #6 was related to attempting to copy revisions on revert. This is not the path that the patch took.

Setting back to NR. I blame random fails still.

:(

#82 fixed the error for me.

Thanks

Since the test code changed, it would be nice to have a test-only patch to make sure that fails before the fix.

Attach test-only fail patch as requested in #90.

Thanks for your work on this, I tried #68 on my site and the issue persists.
Steps to reproduce are:

1. Create a new LB page and save as draft
2. Edit the layout, add a section and any block and save as a draft
3. Edit the layout (again), edit the block added in step 2, and again save as draft
4. Select “Show revisions” from tasks menu and revert to the revision created in step 2 (the second oldest revision)
5. Edit the layout yet again, and try to edit the block. When you try to save changes to the block (by selecting “Update”) you’ll get the error message "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."

Thanks for testing the patch out, @omahm.

Can you double check that you cleared caches after applying #82 please? It was my understanding that the old constraint validation should be completely bypassed for inline blocks, meaning that what you're seeing _should be_ impossible with the patch applied and caches flushed.

I won't have time this week to write a fail test, so if you want to take a swing at it, please don't hesitate to. :-)

Cheers

Verified #82 fixes the issue following the steps in the description.

1. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -13,3 +13,10 @@ function block_content_removed_post_updates() {
    }
   +
   +/**
   + * Updates the entity changed constraint implementation.
   + */
   +function block_content_post_update_entity_changed_constraint() {
   +
   +}
   

   The comment makes it look like the empty post update is a mistake. It should mention that it's added purely to flush caches.

2. +++ b/core/modules/block_content/src/Plugin/Validation/Constraint/BlockContentEntityChangedConstraintValidator.php
   @@ -0,0 +1,30 @@
   +   * {@inheritdoc}
   +   */
   +  public function validate($entity, Constraint $constraint) {
   +    // Don't validate "inline" (i.e. non-reusable) block content entities
   +    // as this prevents saving an update to the block form on a reverted host
   +    // entity's layout in layout builder. This is safe because inline blocks
   +    // are not actually saved until the whole layout is saved, in which case
   +    // Layout Builder forces a new revision for the block.
   ...
   +    if ($entity instanceof BlockContentInterface && !$entity->isReusable()) {
   +      return;
   +    }
   

   I'm not sure the comment shows enough why we have to change this.

   This is a problem when the host entity is changed outside of the entity form, not only a revision revert I think?

   Layout builder is the only implementation of inline blocks we have in core, but there could be others later or in contrib. Given this code is in block module, I think it should mention layout builder as being an example of this problem.

   Something like:

   'This prevents saving an update to the block via a host entity's form, if the host entity has had other changes made via the API instead of the entity form, such as a revision revert. For example... [layout builder-specific stuff]".

Updating the comment is a Novice task

Updating IS for last step - let's get that patch rerolled with changes.

Updated comments as prescribed by @catch in #95

reviewing this

Verified #98 patch.

Testing steps:
# Created new instance of drupal 9.5.x
# Create a node and add a basic block to it via LB
# Create a new revision, and edit the block
# Revert to the first revision via the Revisions screen
# Try to edit the block again.

Problem:
on save you will trigger the constraint and get the error message "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."

Expected output:
on save content should be save without any error

Test Result:
we can save the content with any issue.
this new patch included all the changes mentioned by the #95.

Attaching SS for reference.

Before:

After:

Can be moved to RTBC

+++ b/core/modules/block_content/block_content.post_update.php
@@ -13,3 +13,10 @@ function block_content_removed_post_updates() {
+/**
+ * Added purely to flush caches.
+ */

This text appears in the UI. It has different rules than regular function text. It should be about why we actually have the update function. So
Clear the entity type cache.

We also tend to comment empty functions. Here's an example from 9.4.x...

/**
 * Clear the source count cache.
 */
function migrate_post_update_clear_migrate_source_count_cache() {
  // Empty post_update hook.
}

Given this is a critical issue I've made the changes on commit.

diff --git a/core/modules/block_content/block_content.post_update.php b/core/modules/block_content/block_content.post_update.php
index 8e88fd0ffd..8e707e9099 100644
--- a/core/modules/block_content/block_content.post_update.php
+++ b/core/modules/block_content/block_content.post_update.php
@@ -15,8 +15,8 @@ function block_content_removed_post_updates() {
 }
 
 /**
- * Added purely to flush caches.
+ * Clear the entity type cache.
  */
 function block_content_post_update_entity_changed_constraint() {
-
+  // Empty post_update hook.
 }

Committed and pushed 7362bc50b2 to 10.1.x and 8672c43544 to 10.0.x and 6a261489b6 to 9.5.x. Thanks!