Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
On a project recently, I started themeing an existing bundle. It had view_display config already with various fields. I enabled layout builder and configured the default display. Then my tests started failing.
They were checking for specific ids rendered on the page, generated by Html::getUniqueId, and these ids started getting the numeric counters added to the end signalling that they were being rendered more than once on a page.
It turns out that the regular view_display fields (i.e stuff under the content key) are still rendered even with layout builder enabled!
This could be quite a large performance overhead for sites with large bundles as this rendering is effectively discarded.
What makes this worse is there's no way to clear this config out once Layout builder is switched on other than hand editing the view_display config. This is not intuitive for most people.
Even worse is that when hand editing this config, it takes multiple rounds of import/export to really get everything hidden.
E.g if I set content: { } and do a config import, then a config export, some fields may be added back to content or even layout builder itself. This seems to mostly happen with extra fields such as links, content_moderation_control, and member_for (for user entities).
Steps to reproduce
Configure a bundle view display with a formatter that uses Html::getUniqueId
Enable layout builder and add a field block with the above formatter
Notice numeric suffixes output in your HTML.
Proposed resolution
Probably the best solution is to clear out this content key and automatically put all fields into hidden when LB is enabled.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3385746-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3385746
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
tim.plunkettAdding some related issues
Comment #3
catchThis 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.
Comment #4
longwaveAs 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
contentkey 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 thecontentkey 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 fromcontenttohidden.Comment #7
AnybodyWe 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.
Comment #8
AnybodyCurrently Proposed resolution says:
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:
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?
Comment #9
DanielVezaTests 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.
Comment #10
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #11
DanielVezaRebased the MR
Comment #12
smustgrave CreditAttribution: smustgrave at Mobomo commentedHave 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.
Comment #13
DanielVezaAgree with the feedback. Removed the obsolete line
Comment #14
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks, that was the only thread I saw so appears good to me!
Comment #15
alexpottWhat 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
is not handled yet. Do we need a follow-up or should we attempt to fix that here too.
Comment #16
DanielVezaIMO 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
Comment #17
tim.plunkettSay 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.
Comment #18
AnybodyPerhaps we should instead just move the values to a different (disabled) key and undo that in the uninstall hook?
Comment #19
longwaveAlternatively 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.
Comment #20
AnybodyI 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 :)
Comment #21
AnybodyPS: 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?
Comment #22
DanielVezaThe 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
Comment #23
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedI 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.
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.