Prevent auto-adding new fields to LB layouts

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 📌 Update layout builder tests and set new config Active

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 ✨ Add an API for feature flags Active 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.

+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. 📌 Update layout builder tests and set new config Active 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.

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 ✨ Add an API for feature flags Active . 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.