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 #3115759: Prevent auto-adding new fields to LB layouts 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 layout buildered view mode.

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 always 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!)

I'm really unsure if the proposed resolution is the right way at all! I think not using "hidden" and "content" at all for rendering, when layout builder is active, would by far be more bulletproof, because otherweise:

• hidden has to correctly list all fields
• content needs to be empty

If one of them is not the case (in this dynamic environment), things will go wrong!

Simply skipping this logic / config with layout builder enabled and using the layout builder third party settings is just straightforward.

Would you agree?

Tests are green again. Between #3115759: Prevent auto-adding new fields to LB layouts 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 #3115759: Prevent auto-adding new fields to LB layouts

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

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