Prevent auto-adding new fields to LB layouts

See \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::setComponent(), that entire method is providing this functionality.
There's a guard condition right at the top that could be expanded to include the check you are proposing.

Adding credit for neclimdul from #3221294: New fields get rendered by LB which I closed as a duplicate.

Overall looking really good! We have some test failures though.

Tests are green, I've responded to the feedback. Happy to hear other options if people disagree. Ready for review :)

Can the issue summary be updated to use the standard issue template.

Left some comments on the MR.

Flagging that we should add a change record for this too

IS updated, CR created.

Setting this to needs review, there is a couple of open threads that could use some more discussion moving forward.

Could this be simplified to not add the fields by default at all, and just remove the functionality that does this?

Think that’s something the sub maintainer could say

But I do vote for the tests to be fixed here.

Unless there’s history for adding a setting and adding default value to another ticket. That I don’t know

Pushed a commit that sets the new configuration to FALSE for new sites. Lets see how the tests go

Not sure if this would fly though. If a bunch of tests do break, maybe in the setup we just set this new configuration to true with a todo to a follow up to cleanup.

If we do go with the config option and not remove the code out right

Not sure if this would fly though. If a bunch of tests do break, maybe in the setup we just set this new configuration to true with a todo to a follow up to cleanup.

I'm a bit lost now. Having it FALSE by default then overriding every LB test to set it to TRUE feels significantly less clean to me then just maintaing the existing behaviour in this issue then changing the default in a follow up. Maybe I'm not understanding something.

Yea now that I read that out loud you’re right

Reverted the last commit. Since updating the default value and tests can hopefully be done in a follow up I'm moving this back into needs review

Spoke with @larowlan and we can push the test fixes to a follow up.

Opened #3423225: Remove layout_builder_add_new_fields_to_layout

Added some review comments.

I've changed the approach of this issue to use a feature flag module rather than configuration. The unresolved threads are no longer relevent, but I'm leaving them open for the moment. Adding the feature flag issue as a related issue.

Can the CR be updated with the new approach same for IS.

CR & IS updated

Thanks! That clears things up.

Should this be postponed on #3423352: Add an API for feature flags then as it is using the feature flag module approach but that approach is not yet 100% agreed.

My understanding was using a sub module flag was a temporary solution till that was worked out

@smustgrave looking at the most recent comments on the issue from @catch and @larolwan is looks like each flag is going to be module...

A feature flag that disables something feels the wrong way round to me. I think we should install it on existing installs, where users can disable it again if they wish, but not on new installs.

The code that is behind the flag should also be moved to a hook in the feature flag module, if possible?

+1 for longwave's suggestion.

A feature flag that disables something feels the wrong way round to me.

Yeah good call, I agree with this.

I think we should install it on existing installs, where users can disable it again if they wish, but not on new installs.

For the sake of getting this one in quickly we were going to change the default behaviour in a follow up issue. #3423225: Remove layout_builder_add_new_fields_to_layout However this one is blocked now so maybe we can fit in in here.

The code that is behind the flag should also be moved to a hook in the feature flag module, if possible?

As far as I'm aware, the current goal of the feature flag modules is for them to contain no code.

Postponed on getting agreement on #3423352: Add an API for feature flags

A feature flag that disables something feels the wrong way round to me

But this is a "feature" for developers/site builders to stop this annoying behaviour of things being added automatically.

Honestly I think this behaviour is so disruptive it could almost be considered a bug and just switched off altogether, but that's probably going to delay this even longer.

I think we should install it on existing installs, where users can disable it again if they wish, but not on new installs.

Then why not just blanket remove the functionality as per above...?

When is adding a new field automatically to a bundle's view display actually a good action for the site builder?

This seems to happen sporadically when view displays are saved via update hooks as well, fields will just get blanket added to the layout for what seems like no reason at all (perhaps they aren't already in the hidden key?)

We already have an existing feature flag module, already in layout builder, so IMHO we don't need to postpone this on #3423352: Add an API for feature flags. Worst case we just remove these modules, which we were going to do anyway.

I've reversed the feature flag module so it provides the old behavior, and new sites get the new behavior by default.

Setting NW for a update post_update hook to enable the module on existing sites. CR also needs updating.

Added a static copy of the latest MR patch.

+1 for this change. This auto-field-adding (bug?) just bit me in a deployment.

The problematic deployment upgrades a content type's teaser display from the basic/regular type to a LB layout. When I deploy that change without this patch I get the new LB layout in place, as configured, but with all of the old fields from the old basic/regular display unexpectedly appended into the first section (result not matching config). When I apply this patch and repeat the same deployment test I get the new LB layout, as configured, with no surprise fields appended (result matching config).

When investigating some test failures I noticed this commit https://git.drupalcode.org/project/drupal/-/merge_requests/6609/diffs?co... was resulting in You have requested a non-existent service "plugin.manager.core.layout". in LayoutEntityHelperTraitTest - see https://git.drupalcode.org/issue/drupal-3115759/-/jobs/5455575

This is because on HEAD, that test contains no sections which means we don't call $section->getLayout() in calculateDependencies. Section::getLayout calls Section::layoutPluginManager.

Obviously the easy fix here is to enable layout_discovery in that test (which is a dependency of layout_builder anyway) however it seems like this may have a side effect of adding a default section when we shouldn't be.

However, reverting that commit then makes a huge number of tests fail (30 at the time of writing this) because now when we enable layout builder for a bundle, the layout is completely empty, so tests that click things like Add block fail because there is no section to add a block to.

We have several options for fixing this:
1. Individually fix each test to add a section (a lot of work)
2. Enable the feature flag module on all of tests (easiest)
3. Figure out if we should populate a layout with the existing defaults (section + fields) when it's first enabled (might not even be possible, still investigating).
4. Go back to always adding a default section even when not adding fields.

I was hoping with option 3 we could just use $this->isNew() but since LB takes over existing view displays that won't work.

Re #48 I've gone with 4 for now.

This is green, CR and IS updated. Would be good to get a fresh review on it.

Changes look great. I'm happy with #49, it feels like the lowest impact solution for a situation that should be rare outside of tests.

I've popped one thought in about a hook_requirements implementation, so leaving in needs review for people to have a think about it. I don't feel strongly about this, if its decided we don't want it then this is ready to be RTBC

Left 1 small comment on the MR.

All feedback appears to be addressed for this and functionality working as expected. Wish I had this the other day actually!

Had one last look through the code, looks great! It would be fantastic to get this in. I constantly wish I had this when I add new fields to content types.

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.

Was a clean rebase

This looks good. I was going to ask about where we're going to remove it from all the tests it's added to, but then I saw #3423225: Remove layout_builder_add_new_fields_to_layout, we might want to split that issue into two, but can discuss on that issue.

Committed/pushed to 11.x, thanks!

This adds a deprecated module and it should be added to https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsolete. There is a similar module listed on that page and the documentation style may be of some use, https://www.drupal.org/node/3223395#s-layout-builder-expose-all-field-bl....

I don't see a review of the change record. Can someone check that?

Also, this has broken HEAD testing because

    There was 1 PHPUnit test runner deprecation:
    
    1) Metadata found in doc-comment for class Drupal\Tests\layout_builder\Functional\LayoutBuilderNewFieldsTest. Metadata in doc-comments is deprecated and will no longer be supported in PHPUnit 12. Update your test code to use attributes instead.

Bumping to critical while HEAD testing is broken.

Reverted for now - let's add the deprecated module documentation and update the test.

Added to the module page https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol...

Discussed this briefly with @lauriii, he asked why we're adding a feature flag for this at all - it only affects existing sites when they add a field in the field UI, not the actual runtime of the site - so we should be fine to just change the behaviour with no bc layer.

Thinking about it more this makes sense to me too, I tried to trace back where this was discussed and I see Damien's comment asking whether it was really necessary, but otherwise we only discussed how to do it, not whether to do it or not.

Given that would allow us to remove a fair bit of code + and upgrade path + the follow-up issue, I think we should just do that here. It will mean updating some tests but that would need to happen anyway to remove the feature module.

Should I delete the documentation entry then?

I've reverted the documentation page.

The tricky part here is that setComponent is used both for adding fields to the layout when Layout builder is initially enabled, and when adding new fields.

Both in the case of the old feature flag and removing the code outright, we get an empty layout when enabling layout builder. IMO this is a good thing, but just wanted to point that out if it wasn't obvious before.

We could retain that functionality and just move the code out of setComponent and into a new method called by the preSave which is where the initial addition of the fields when layout builder is enabled happens.

Still huge +1 for this feature.

Seems to have broken a few tests