Saving Layout override will revert other field values to their values when the Layout was started.

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,9 +117,26 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +      if ($edited_field_name === OverridesSectionStorage::FIELD_NAME) {
   +        $active_entity->set($edited_field_name, $previous_context_entity->get($edited_field_name)->getSections());
   +      }
   +      else {
   +        $active_entity->set($edited_field_name, $previous_context_entity->get($edited_field_name));
   +      }
   

   What happens if we replace this with the else code? Doesn't look good that every other field works except ours, do we need a change in LayoutSectionItem/LayoutSectionItemList instead?

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,9 +117,26 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
        $entity = parent::buildEntity($form, $form_state);
   -    $this->sectionStorage->setContextValue('entity', $entity);
   +    $this->sectionStorage->setContextValue('entity', $active_entity);
        return $entity;
   

   This change looks off to me. We should either not be calling parent::buildEntity() at all or should be using that result here. And returning a different entity than the one we use as a context value seems wrong for the same reasons.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -106,6 +106,52 @@ public function testOverrides() {
   +    $this->drupalGet('node/1');
   ...
   +    $page->clickLink('Layout');
   ...
   +    $this->drupalGet('node/1');
   ...
   +    $this->drupalGet('node/1/layout');
   

   Nit, here we click to get to the page and there we go to it directly. I'm not picky about which way we do it but let's be consistent within a test method.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -106,6 +106,52 @@ public function testOverrides() {
   +    $assert_session->pageTextContains('updated body');
   +
   +  }
   

   Nit, extra blank line

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +117,31 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    $previous_context_entity = $this->sectionStorage->getContextValue('entity');
   ...
   +    $non_update_fields = $this->getEditedFieldNames($form_state);
   

   I think these variable names are more confusing after the last interdiff

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +117,31 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    foreach (['id', 'revision', 'bundle', 'uuid'] as $non_update_key) {
   

   This makes me even more nervous. Are we sure those are the only 4 that matter?
   Could this instead add $entity_type->getKeys();?

Thanks, the explanation about getKeys makes sense.

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +116,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
        // \Drupal\Core\Entity\EntityForm::buildEntity() clones the entity object.
        // Keep it in sync with the one used by the section storage.
   ...
   +    $this->setEntity($section_storage_entity);
        $entity = parent::buildEntity($form, $form_state);
        $this->sectionStorage->setContextValue('entity', $entity);
   

   I think this comment should go with this code

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +116,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    // Any fields that are not editable on this form should be updated with the
   +    // value from the active entity for editing. This avoids overwriting fields
   +    // that have been updated since the entity was stored in the section
   +    // storage.
   ...
   +    $field_definitions = $section_storage_entity->getFieldDefinitions();
   +    foreach (array_keys($section_storage_entity->getFields()) as $field_name) {
   

   This comment should go with this code

3. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +116,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    // @todo Replace with new API in
   +    //   https://www.drupal.org/project/drupal/issues/2942907.
   +    /** @var \Drupal\Core\Entity\FieldableEntityInterface $active_entity */
   +    $active_entity = $this->entityTypeManager->getStorage($section_storage_entity->getEntityTypeId())->load($section_storage_entity->id());
   

   Leaving this comment to go with this code

4. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +116,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    $edited_field_names = $this->getEditedFieldNames($form_state);
   +
   

   Move the blank line from below this code to above this code

5. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -116,7 +116,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +      if (!in_array($field_name, $edited_field_names) &&
   +        !$field_definition->isReadOnly() &&
   +        !$field_definition->isComputed()) {
   

   Nit: put this on one line

6. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -106,6 +106,47 @@ public function testOverrides() {
   +    $page->clickLink('Layout');
   +
   +    $node = Node::load(1);
   +    $node->set('body', 'updated body');
   +    $node->save();
   

   Just to avoid accidental changes later, this deserves an explicit comment explaining that it is purposefully visiting the layout page so that there will be a tempstore entry _before_ editing the node values.

   Also we should be super careful to fix tests if we ever make it so that visiting the layout page without changing anything does not make a tempstore entry... Maybe better to explicitly add a block here?

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -114,9 +114,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    $edited_field_names = $this->getEditedFieldNames($form_state);
   +    // Any fields that are not editable on this form should be updated with the
   

   Nit, might as well put this below the comment too

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -114,9 +114,30 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    $field_definitions = $section_storage_entity->getFieldDefinitions();
   +    foreach (array_keys($section_storage_entity->getFields()) as $field_name) {
   

   Didn't get an answer on this part

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -106,6 +106,55 @@ public function testOverrides() {
   +    $page->pressButton('Save layout');
   +
   

   Not this part, that clears the tempstore! Also, blank line

Thanks! RTBC assuming the FAIL fails and the other passes.

Whoopsie-daisy, data loss.

Committed to 8.7.x.

I am assuming this also affects 8.6.x unless it's a recent regression. Should we backport it?

The fix in 8.7 relies on the existence of an entity form, which wasn't backported to 8.6
In the meantime, here's the test backport

Reverted at @tim.plunkett's request. Thanks!

The override of buildEntity() turns out to be completely unnecessary (at least in its current implementation). I added that in #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features #49 and #53, and it turns out that it wasn't really needed, only the addition in #68 of that issue.

Thanks, Tim! The new "pass" patch looks great to me and fixes the problem I encountered in #2950869-41: Entity queries querying the latest revision very slow with lots of revisions.

Highlighting from the "FAIL-no-access" patch:

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -102,26 +102,9 @@ protected function init(FormStateInterface $form_state) {
-    // @todo \Drupal\layout_builder\Field\LayoutSectionItemList::defaultAccess()
-    //   restricts all access to the field, explicitly allow access here until
-    //   https://www.drupal.org/node/2942975 is resolved.
-    $form[OverridesSectionStorage::FIELD_NAME]['#access'] = TRUE;
...
-  public function buildEntity(array $form, FormStateInterface $form_state) {
-    // \Drupal\Core\Entity\EntityForm::buildEntity() clones the entity object.
-    // Keep it in sync with the one used by the section storage.
-    $this->setEntity($this->sectionStorage->getContextValue('entity'));
-    $entity = parent::buildEntity($form, $form_state);
-    $this->sectionStorage->setContextValue('entity', $entity);
-    return $entity;
-  }

I added the buildEntity because our layout field wasn't being synced correctly.
Later, that issue added the #access = TRUE line. It turns out, that's all that was needed. It was preventing it from being synced over.

Thanks for the review!

So the why-this-works is a bit mind-bending, especially for how simple the final patch actually is. @tim.plunkett, @tedbow, and I walked through it.

One thing that's out of scope that we want a followup for is for:

    $form[OverridesSectionStorage::FIELD_NAME]['#access'] = TRUE;

NIH, but that's kinda scary. The followup wouldn't be #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API, but rather a separate issue to discuss hardening this so that it's a bit less sweeping than TRUE.

I manually tested the new patch and confirmed that it's working. However, there is one thing that's still weird. When I reloaded the layout tab in step 6 of the STR in the IS, I expected to see the updated field content on the page. However, it still shows the original field content, which only appears to "change" after save. So from the perspective of the user editing the layout, there's a weird mismatch between what's on their screen and the actual state of the data.

It's still way better than data loss (actually, this should be critical), but it'd be good to at least discuss the "stale content from the tempstore" behavior and whether we should fix it or open a followup or whatnot.

#3035955: Ensure that setting #access => TRUE for the Layout Builder field is acceptable is for the bit mentioned in #32.

#3035992: Stale values can be displayed by the Layout Builder UI but are saved correctly is the follow-up for this issue (which I linked from that side of things)
I think we need to fix the data loss NOW, which is what the patch does.
This issue also blocks multiple other issues.

OK, I'mm comfortable with committing this as-is to stop the data loss, so long as we have #3035992: Stale values can be displayed by the Layout Builder UI but are saved correctly.

Committed to 8.7.x. Thanks!

It seems increasingly unlikely that there will be a straightforward backport for this, so leaving it as an 8.7.x-only major bugfix for now.

This same issue now seems to be happening with Drupal 10.1.

Upon reverting the layout back to default for a node, it strips all the fields of that node.

This seems dependent on if there were new sections added/removed to the default layout after node creation.