Adding a display mode to a content type using layout, and disabling layout on that new display mode removes the layout_builder__layout field and breaks layout in already configured display modes.

I'm facepalming VERY hard over here. Thanks for filing this!

This is partially due to how creating new view modes works: it copies 100% of the values over from default.
Even values that currently don't make sense, like enabling overrides for other view modes.
Until #2907413: Consider supporting Layout Builder Overrides for other view modes happens, that shouldn't be possible...

But let's futureproof this anyway.
The field should not be removed until no other view modes of that bundle have overrides enabled.
Not sure if there's a better way to do that than this entity query, but it should work.

Love getting it red/green on the first try.

Wanted to just RTBC since its nice and clean but....

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -139,6 +139,28 @@ public function preSave(EntityStorageInterface $storage) {
+  protected function removeSectionField($entity_type_id, $bundle, $field_name) {
+    $query = $this->entityTypeManager()->getStorage($this->entityTypeId)->getQuery()
+      ->condition('targetEntityType', $this->getTargetEntityTypeId())
+      ->condition('bundle', $this->getTargetBundle())
+      ->condition('mode', $this->getMode(), '<>')
+      ->condition('third_party_settings.layout_builder.allow_custom', TRUE);
+    $enabled = (bool) $query->count()->execute();
+    if (!$enabled && $field = FieldConfig::loadByName($entity_type_id, $bundle, $field_name)) {

Hard computer problem, naming things. Since this conditionally removes instead of always removes, should it maybe be "cleanSectionField()" or "pruneSectionField()"?

Tagging to work on this next week.

reviewing at MWDS

This sounds critical. Is there a mitigation I'm missing? I don't quite follow #3.

#3 wasn't mitigation or anything, just explaining how we ended up here.

Rerolled after #2998215: PHP notices/warnings when managing display for non-default view mode when LB is enabled w/ overrides

Looks pretty good to me...

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -139,6 +139,28 @@ public function preSave(EntityStorageInterface $storage) {
   +  protected function removeSectionField($entity_type_id, $bundle, $field_name) {
   

   Minor point, but does this really need to take any parameters? The $entity_type_id, $bundle, and $field_name are all known to the display entity already.

   Eh, I guess this is a nitpick, really, seeing as how addSectionField() follows the same pattern. Just pointing it out.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -139,6 +139,28 @@ public function preSave(EntityStorageInterface $storage) {
   +    $query = $this->entityTypeManager()->getStorage($this->entityTypeId)->getQuery()
   

   Can we use $this->getEntityTypeId() instead of $this->entityTypeId?

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -139,6 +139,28 @@ public function preSave(EntityStorageInterface $storage) {
+  /**
+   * Removes a layout section field from a given bundle.

I echo @neclimdul's sentiment about either renaming this method, or changing the doc comment to explain that the field *may* not be removed.

#12.1
Yeah, this is done for parallelism with addSectionField()

#12.2
Fixed

#13.2
Adjusted/expanded the docblock

Thanks, @tim.plunkett. This looks excellent. RTBC on green.

Wow, I think this is the most critical non-security bug I've seen since... maybe since release. Didn't even realize the full impact when I skimmed it before. Change a display mode and break all your site's content!

I encountered #2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing and (I think) #2941189: Confirmation form for disabling per-entity layout overrides shows a success message and should not in the process of testing this; just adding them to the related issues for posterity.

I also noticed that the confirm title and description are confusing:

The title should actually be something like "Are you sure you want to disable Layout Builder for the Teaser display?" or perhaps simply "Are you sure you want to disable this layout?" The description should be something like "This will remove all your layout data for the Teaser display of Article content". That exists in HEAD, though

Normally I'd want the missing followup filed prior to committing the patch, but that bug is there in HEAD too and this issue is bad enough that I don't want to block it on anything. Will probably commit this once I finish reviewing the test.

1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php
   @@ -0,0 +1,94 @@
   +    $this->drupalLogin($this->drupalCreateUser([
   +      'configure any layout',
   +      'administer node display',
   +      'administer display modes',
   +    ], 'foobar'));
   

   foobar isn't the best test content; I actually had to look up the second argument of drupalCreateUser() to figure it out.

   Spoiler: It's just the username. I don't think we actually need it here as it's an optional argument (random in the 99% case where it's not specified) and we don't use it in this test? Not really a big deal.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php
   @@ -0,0 +1,94 @@
   +    // Enable Layout Builder for the default view modes, and overrides.
   

   The comma here isn't grammatically feasible.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php
   @@ -0,0 +1,94 @@
   +    $assert_session->pageTextNotContains('Powered by Drupal');
   ...
   +    $assert_session->pageTextContains('Powered by Drupal');
   

   Even with the pageTextNotContains(), is this specific enough? E.g. we might change the flow and this could match the sidebar link or something. OTOH this isn't what we're actually testing in this test, so it's probably good enough.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php
   @@ -0,0 +1,94 @@
   +    $assert_session->linkExists('Layout');
   +    $this->clickLink('Layout');
   

   At first this looked redundant; one would think clickLink() would account for the assertion preceding it. But then I remembered that (IIRC) clicking a link that doesn't exist gives some incomprehensible failure message, so this likely makes sense.

These are NBD and are potentially copied from other LB tests, so I don't think they need to be blocking (for the same reasons as in #16).

Committed and pushed to 8.7.x and 8.6.x. Thanks!

Followups for #16 and #18 would be nice.

When attempting to patch 8.7.3, I got the following error from composer. Any suggestions?

patch '-p1' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
Executing command (CWD): patch '-p1' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|index 78ff430b00..2bdeb03870 100644
|--- a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.
2 out of 2 hunks ignored
patching file core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php

patch '-p0' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
Executing command (CWD): patch '-p0' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|index 78ff430b00..2bdeb03870 100644
|--- a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

2 out of 2 hunks ignored

patching file b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php

patch '-p2' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
Executing command (CWD): patch '-p2' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
patching file modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php

Hunk #1 FAILED at 112.

Hunk #2 succeeded at 184 (offset 45 lines).

1 out of 2 hunks FAILED
 -- saving rejects to file modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php.rej

The next patch would create the file modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php,
which already exists!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.

1 out of 1 hunk ignored

patch '-p4' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
Executing command (CWD): patch '-p4' --no-backup-if-mismatch -d 'web/core' < '/var/www/docker_platform/web/patches/2990517-field-14.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|index 78ff430b00..2bdeb03870 100644
|--- a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
|+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

2 out of 2 hunks ignored

patching file tests/src/Functional/LayoutDisplayTest.php

   Could not apply patch! Skipping. The error was: Cannot apply patch web/patches/2990517-field-14.patch

The same patch process created LayoutBuilderEntityViewDisplay.php.rej with this:

--- modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
+++ modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -112,8 +112,8 @@ public function preSave(EntityStorageInterface $storage) {
       if ($new_value) {
         $this->addSectionField($entity_type_id, $bundle, 'layout_builder__layout');
       }
-      elseif ($field = FieldConfig::loadByName($entity_type_id, $bundle, 'layout_builder__layout')) {
-        $field->delete();
+      else {
+        $this->removeSectionField($entity_type_id, $bundle, 'layout_builder__layout');
       }
     }

Here is the relevant section from composer.json:

"extra": {
		 "enable-patching": true,
		 "patches": {
		 	 "drupal/core": {
                   "Contextual filter patch": "web/patches/2710407-14-validator-transform-fix.patch",
                   "Layout Builder missing field patch": "web/patches/2990517-field-14.patch"
		 	 },