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

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

The last submitted patch, 53: 3035992-53.patch, failed testing. View results →

+++ 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

The last submitted patch, 56: 3035992-56.patch, failed testing. View results →

+++ 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 🐛 Allow field blocks to display the configuration label when set in Layout Builder Fixed

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 🐛 Allow field blocks to display the configuration label when set in Layout Builder Fixed , 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

Left some suggestions for nits, but also raised a few points that need to be addressed.

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 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Postponed . 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?

I will try to look between now and the end of DrupalCon. For now, I'd consider this NW for finding a better way

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 :)