Automatically clear out the content key in view display when enabling Layout builder

Adding some related issues

This seems like a good idea. Either it should happen automatically, or there should be a way to force yourself back into the usual view mode settings to be able to configure things to off.

Also seems like even if you configured everything to hidden before enabling layout builder, and new extra fields etc. tend to automatically be added to all view displays (rather than automatically hidden), so you could still end up with cruft in there.

As reported in the linked issue I think I've experienced ✨ Prevent auto-adding new fields to LB layouts Needs review due to this. I had several fields in the content key of an entity view display, which was then switched to use Layout Builder. In the LB layout not all the fields were used. But on deploy, all the remaining fields that were not yet used in the layout but still in the content key were automatically added to the first section of my layout! This was reproducible after reverting and then reimporting the config, and went away when I manually moved the fields from content to hidden.

We hit this once again! This often seems to happen in the background (which is bad for performance) and is getting visible when causing issues like "Recursive rendering detected when rendering entity…" in our case, for a field, which isn't used at all in this viewmode.

As the issue describes correctly, that field is being rended, because it's
part of "content"
but not part of "hidden" and "third_party_settings.layout_builder.sections"

That means it's handled & rendered by Drupal (unexpected), but not visible (expected)!

Setting the priority to "Major" for the performance reason and the reason that bugs caused by this can only be resolved by experts in config.

Currently Proposed resolution says:

Probably the best solution is to clear out this content key and automatically put all fields into hidden when LB is enabled.

I'd like to add considering to also prevent rendering the fields present in "content" when layout builder is enabled to ensure they're not handled in the background, even if present due to any reason. If that's not possible, we should show a warning in layout builder and allow to clear them in UI? (Hopefully that's not needed!)

Tests are green again. Between ✨ Prevent auto-adding new fields to LB layouts Needs review and this issue all new Layout Builder displays should be clean.

In terms of #7 and #8 I feel like we should investigate this in a follow up to see if it's worth it. Happy for opinions on this, I just don't think we should block this from getting in.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Rebased the MR

Have to agree with the open thread. Read a few times but maybe missing something? If that's correct order could the comment be expanded upon.

Agree with the feedback. Removed the obsolete line

Thanks, that was the only thread I saw so appears good to me!

What about existing sites - do we need an update function to fix them? Also what is the expected behaviour if you then disable layout builder for that entity type?

Also as far as I can see

Also seems like even if you configured everything to hidden before enabling layout builder, and new extra fields etc. tend to automatically be added to all view displays (rather than automatically hidden), so you could still end up with cruft in there.

is not handled yet. Do we need a follow-up or should we attempt to fix that here too.

IMO we don't need an update hook for existing sites. Nothing is technically broken on them. This will be an improvement moving forward for all new layouts created once this is committed.

Disabling Layout Builder should just have the same behaviour as if you'd moved all the fields to hidden manually. Nothing should have changed around that.

New fields being automatically added to LB is handled by ✨ Prevent auto-adding new fields to LB layouts Needs review

Say you carefully configure your view display to have the exact order and field formatters you want. You save, everything is great. Then you wonder what might happen if you turn LB on. You try it, decide against it, and then disable it.

In HEAD, all is well. you're back to your perfectly configured display.

After this MR, everything is gone. You're back to square one.

Perhaps we should instead just move the values to a different (disabled) key and undo that in the uninstall hook?

Alternatively can we just prevent the content from being rendered in the first place when LB is enabled for the display? Then the data can stay where it is.

I see the risk, that this may lead to confusion or mistakes in contrib, as it's not self-explaining. Removing or using a different key would definitely show you that this part is unused. And if the key is used by mistake, it would show up by error or notice.

For that reason, I'd vote for the rename / move. But of course the other option is also valid, I'm just in favour of self-explaining at the root :)

PS: Perhaps we could introduce a general and optional documented "disabled" key for similar cases in the future? Where the keys can be put below. Might be a helpful pattern?

Say you carefully configure your view display to have the exact order and field formatters you want. You save, everything is great. Then you wonder what might happen if you turn LB on. You try it, decide against it, and then disable it....After this MR, everything is gone. You're back to square one.

The same is true in HEAD if you decide you want to disable LB then re-enable it. The config system makes this a non issue but I suppose we need to cater for people not using that. The other difference when disabling LB is that the confirm form makes it clear all changes will be lost.

Regardless, I spent a few hours messing around with an alternate approach when just blocks renders of the fields that exist in the content key but it wasn't quite as easy as it originally appeared. Pausing on this one for now

Perhaps we should instead just move the values to a different (disabled) key and undo that in the uninstall hook?

I think having content, hidden, and disabled keys could lead to confusion and be quite messy to handle. How would we know when to move disabled stuff back to content when the module is uninstalled? We could again unintentionally overwrite something the site builder has intentionally done.

As @DanielVeza points out, the CMI system should negate any of these issues. I don't know how to cater for people that aren't using this appropriately tbh.

Then you wonder what might happen if you turn LB on.

I think it's basically impossible to guard against people experimenting like this and make it a perfect case for every scenario. I can think of 1000 ways people can unintentionally brick their site by experimentation. The config dependency system alone makes this extremely easy to do if you're not careful with confirm forms.

So been trying to keep up, is the consensus no update hook?

I can see the situation in #17 happening to a very small fraction of users but this bug currently affects a lot of sites and doesn't have a good workaround. For me I think we should go ahead here. Without an update hook.

I agree with #25

Seems to have consensus

Running test-only job

There was 1 failure:
1) Drupal\Tests\layout_builder\Kernel\LayoutBuilderEntityViewDisplayTest::testFieldsMovedToHiddenOnEnable
Failed asserting that an array is empty.
/builds/issue/drupal-3385746/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3385746/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3385746/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderEntityViewDisplayTest.php:102
/builds/issue/drupal-3385746/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 20, Assertions: 111, Failures: 1.

So does have test coverage.

All threads appear to be resolved

Going to move this one forward

Left some comments on the MR - thanks folks.

Did anyone look into #19? That would address @tim.plunkett's concern in #17