Stale values can be displayed by the Layout Builder UI but are saved correctly

Assigning to myself to avoid duplicating work. I have a solution working on my local dev enviroment. There's a few additional things I'd like to do then I'll upload the patch and switch back to unassigned.

Was having difficulty getting the block edit ID in a Functional test so went with FunctionalJavascript but can refactor if that is addressed.

On entity save, check if a layout override exists in the tempstore for the entity. If yes, update the context of the temporary section storage entity context with a copy of the entity being saved (other than it retaining the value of the layout_builder__layout field from the entity in tempstore.

@tim.plunkett could you clarify your statement from from #7.3 ?

The other thing would be to consider other fields added to the page, see content_moderation_entity_form_display_alter()

. Does this mean that that the process added in the patch should make to retain any fields added in the manner of content_moderation_entity_form_display_alter(), or that newly-added fields to and entity should be added to overrides similar to how they are added to defaults, or something entirely different from either of those?

1. #7.1
   

   I think we should use the Drupal::classResolver pattern and have this logic live in an injectable class

   Done.

2. #7.2
   

   There's a whole dance in \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::deriveContextsFromRoute() (including an @todo) that needs to be done for the view mode.
   Follow-up idea: abstract that out to a public helper method?

   - Abstracted to two methods in LayoutEntityHelperTrait.php

3. #7.3
   

   The other thing would be to consider other fields added to the page, see content_moderation_entity_form_display_alter()

   From what I can tell, fields added this way never make it into the tempstore, so they're not subject to the conditions that lead to stale values appearing. If I'm overlooking something, a suggestion on how to cover this use case in tests should be all I need to account for it fully.

4. #11 Sort of a follow up to item 3^^ , with the current approach I dont think other fields need to be accounted for? The approach in the patch doesn't bother with transfering updated field values. Instead, it just replaces the stored entity context with the one being saved, with the exceoption of the Section Storage Field, since it contains changes that are only present in the tempstore.
5. #9
   

   I opened #3046216: SectionStorageManager::load() does not evaluate constraints to look into fixing this

   I adressed this by adding a check for FieldableEntityInterface with a @todo referencing #3046216: SectionStorageManager::load() does not evaluate constraints.

Patch from #12 got successfully applied for me..I have uploaded screenshot.Please check..So moving it to needs review status.

Hi, I am not sure if I am running in to this issue or a slightly different one.
Reproduction steps I am following are

Create a basic page with layout builder enabled
Save page
Open article/layout page
Add text block and set block body to "original"
Update block
Save layout
Open article /layout page

Edit text block and set block body to "updated"
Article body is "updated"
Edit text block.
block body is still "original"

It sounds similar since stale data is being shown in the layout builder interface, but am not sure if this is a separate issue that needs raising.
Tested in 8.7 & 8.8

Just an update on my previous comment for anyone who lands here, since this is a lot more visible in google.

The issue we were seeing had very similar symptoms to this issue, but was actually caused by https://www.drupal.org/project/layout_builder_st/issues/3067646 symmetric translation module.
The patch in that resolved the issue for us

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldValuesTest.php
@@ -27,6 +27,11 @@
+  protected $defaultTheme = 'classy';
+

Classy is not recommended to use, see https://www.drupal.org/node/3083055

As FieldValuesTest is a brand new Test added by the patch, I'd suggest changing to another theme and adjusting/rewriting the tests if necessary.

Fixed failed test cases #27.

Please ignore #31
Fixed failed test cases #27.

Patch #33 is working fine on 9.1

I reproduced this steps after applying 3035992-31.patch in #33 and it not fixed problem with update layout values.

1. Allow custom layout overrides on articles
2. Create an article with body value "original"
3. Save article
4. Open article /layout page
5. update article at /edit page. set body to "updated"
6. Open article /layout page
7. Article body is still "original"
8. Save layout
9. Article body is "updated"

Rerolled #33

Fixed Failed commands #41

Fixed Failed commands on #44
Attached interdiff

Fixed deprecation errors and added a check for the route_name

Sorry guess I got ahead of myself.

Added Patch against #49 in Drupal 10.1.x

#51 was an unnecessary reroll that will not receive credit. It's also an incomplete reroll and misses several changes. Additional work should be based on #49, as #51 breaks several things. As I've mentioned to you several times @Shubham Chandra - you can check if a patch truly needs a reroll by clicking "add test/retest" on the most recent patch and applying it to the most recent Drupal version. Drupal getting a new version doesn't necessarily mean a reroll is needed.

As far as feedback on the #49, it looks like the test added should use stark as the default theme, this will allow it to work on both 9 and 10

Updated test to use 'stark' instead of 'classy' as default theme.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldValuesTest.php
@@ -0,0 +1,119 @@
+  protected static $modules = [
+    'layout_builder',
+    'block',
+    'node',
+  ];

We should probably enable the field_ui module here as well. I believe that 4 years ago, field_ui was required by layout builder, but that has since been decoupled.

This should fix the test failure, I believe :-)

I can confirm that #53 fixes the issue on 9.4.12. Here's a dumbed down condition that was reported to us:

Say there are three states:

• Draft
• Pending
• Published

and various transitions:

• Draft -> Draft
• Draft -> Pending
• Pending -> Pending
• Pending -> Draft
• Pending -> Published
• Published -> Draft

and an "Author" only has access to Draft -> Draft, Draft -> Pending, and Published -> Draft.

1. A piece of content is in "Pending" state, thus an Author has no available transitions, and the edit / layout routes will return 403
2. A layout override is started, but not saved (only existing in the tempstore)
3. The content is moved from "Pending" to "Draft"
4. Authors will now have access to the Edit tab, but not the Layout tab until changes are discarded!

Following applying #53, authors have access to the Edit and Layout tabs appropriately.

Sorry for the long-winded comment -- I can vouch for RTBC once the test fail is resolved.

Adding field_ui as mentioned in #55

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldValuesTest.php
@@ -0,0 +1,120 @@
+    $links_block = $assert_session->elementExists('css', '.block-extra-field-blocknodebundle-for-testing-fieldslinks');

Do'h! I think this class used to be added by the Classy theme! This test is now using Stark after #53!

A different CSS selector will have to be used -- I'm not sure off the top of my head exactly what that would be though.

Updated the CSS selector, and there is no specific CSS class on the Body field block in the Stark theme

<div data-layout-content-preview-placeholder-label="&quot;Body&quot; field" class="js-layout-builder-block layout-builder-block contextual-region" data-layout-block-uuid="3d159177-0c9a-4e57-bf47-6b5cce1e3fa3" data-layout-builder-highlight-id="3d159177-0c9a-4e57-bf47-6b5cce1e3fa3" draggable="false">

That is why I updated the code like this

    $links_block = $page->findAll('css', ".layout__region--content > .layout-builder-block");
    // The second block is the body field so that is why $links_block[1].
    $links_block_uuid = $links_block[1]->getAttribute('data-layout-block-uuid');

The other test failures from the file "core/modules/layout_builder/tests/src/FunctionalJavascript/FieldValuesTest.php" are because of the following issue
Allow field blocks to display the configuration label when set in Layout Builder

Tests should typically be passing before an issue is set to "Needs Review", especially since it looks like the failure is related to the changes being made.

There are occasional exceptions, and in those cases there should be an explanation as to why it's getting set to "Needs Review" despite failing tests.

Hi, the last patch was failing tests due to the following issue https://www.drupal.org/project/drupal/issues/3039185, now that issue has been fixed on 10.1x and 11x, So here last patch tests are passing now.

Issue summary is incomplete. If that could be updated please.

So, I've converted #59 to a MR, then fixed a few nitpicks.

My idea of modernizing it a bit is not working as expected, so I've given up on that

Please review

Issue summary looks good thanks!

Left some comments on the MR, mostly small stuff

Ran test-only feature

There was 1 error:
1) Drupal\Tests\layout_builder\FunctionalJavascript\FieldValuesTest::testLayoutBuilderUiDoesNotShowStaleEntityFieldValues
Behat\Mink\Exception\ResponseTextException: The text "The changed value" was not found anywhere in the text of the current page.
/builds/issue/drupal-3035992/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3035992/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3035992/core/tests/Drupal/Tests/WebAssert.php:956
/builds/issue/drupal-3035992/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldValuesTest.php:96
/builds/issue/drupal-3035992/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 1, Assertions: 7, Errors: 1.

So coverage appears to be there.

Looks super close!

Feedback appears to be addressed

Confirmed the issue described in the issue summary has been resolved

#67 mention the test coverage is there

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot seems to be wrong, latest MR passes everything and it contains latest changes from the core

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

So the review bot has been temporarily turned off as a bug is look at.

But appears the feedback has been addressed but will leave in review for @tim.plunkett to agree.

So, there is one unresolved threadm everything else has been addressed afaik.

I've also seen using layout_builder__layout as a check has been discouraged before, see #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant. Though the solution I came up with over there not too sure about, hoping @Tim.plunkett has some better trick.

@Tim.plunkett do you have a different preferred approach or good to continue with what's there?

Just to confirm, this is still an issue with the latest release of Drupal 11.

I rebased on Drupal 11 and moved the new entity update hook to the hook class with the rest. I'd like to throw this back out for review -

This works for me :)

The feedback from Tim Plunkett on the MR hasn't been addressed, and I agree this should do something other than check the route, which feels brittle.

@catch, in #80 @tim.plunkett has self-assigned this issue and no progress for over a year ... is anyone has a better idea on how to overcome this issue without dependency on the route system?

Crazy idea...but does it really matter if the tempstore entity is updated if the entity was saved with LB? Seems to me at face value that's a premature optimization. Can the route check just be removed...?

Implemented @luke.leber suggestion, please review. This should be ready to go in now

Tested this manually following the steps and did notice the text would update.

Would note I did have to discard my changes in layout builder AFTER I applied the MR for the change to take hold.

re: #90 - honestly I think that's alright. Sites are no worse off than they are now and given the age of this issue, IMO it should be landed as-is. It at least prevents the situation from happening to new users even if certain rare(?) tempstores aren't automatically flushed.

Don't let perfect be the enemy of good here. 🙏

#91 is correct, FWIW. While a TempStore is persistent (state) and therefore not invalidated by the normal update cache clear, it's exactly true that the user is no worse off: Their TempStore contained stale values before the update, and still will after the update. I don't think we would write an update hook to update a TempStore except under exceptional circumstances (security or data integrity issue, for example). According to the title and IS, this is a UX/user expectation problem, but not a data loss problem, so I don't think it merits an upgrade path for TempStores.

I really hate to be a pain, if its an easy swap can we please make the test Functional instead of FunctionalJavascript? Layout Builder uses FunctionalJavascript tests for so much, even when no JS is required. I'm trying to move all tests (where possible) to Functional tests so ideally we'd avoid adding too many more.

From a glance I don't see anything here that needs it to be a FunctionalJs test. If I'm incorrect I'm happy for this to be RTBC in it's current state.

Per great suggestion by @danielveza, I have converted the test to Functional

Hope this can go in finally

Left comments in MR.

addressed feedback

This has has many comprehensive reviews already, all feedback has been addressed. I've given it one last sweep, I think this is ready for RTBC

A diff from the latest MR changes. Works fine on 11.2.4

This is a tricky one but it looks good. Committed/pushed to 11.x, thanks!