Give "use layout builder" less precedence than CE display config

We have discussed this problem and figured following the following logic should be all that's needed:

* When checking layout-builder for being enabled:
    * When current view-mode (e.g. teaser) has no CE-display, fallback to default core display-mode for layout-builder setting-check
    * When current view-mode (e.g. teaser) has a CE-display, NEVER fallback to core display-mode for layout-builder setting-check. 
        * If there is no core-display-view-config for the view-mode (e.g. teaser), take layout-builder to be disabled.

That way, the fallback for the layout-builder-check will only be applied when there is a fallback at CE-display config-level active. This is the naive assumption a site-builder would have on how things should work, so this is what we want to do.

Working on it - but need a clearer head (and some succeeded internal tests with a patch) before I dare comment here.

Most important tests are added, I may add a few more tomorrow.

@fago Thank you for the message. I agree that this is clearer, but had to see this in practice to really get a feel for it.

We have implemented something a while ago in our internal projects, because a fix was urgently needed for the above "Problem/motivation". It works well in practice. Now, tests are added to prove out that it works 100% as I thought, and I can verify things more easily if we need to discuss/change more.

The MR also includes changes to the README. I need to do this now, otherwise I'll forget to include details later. Since this change also has effect on the update path (makes it more straightforward), I have documented implications for the update path too.

However, there is a complication with this proposal vs. the fix we are running internally.


* When current view-mode (e.g. teaser) has no CE-display, fallback to default core display-mode for layout-builder setting-check
* When current view-mode (e.g. teaser) has a CE-display, NEVER fallback to core display-mode for layout-builder setting-check.
* If there is no core-display-view-config for the view-mode (e.g. teaser), take layout-builder to be disabled.


Point 2 is fine.

But the indented 'point 3' (as I read it now) seems to refer to 'point 1, not 2'. Which I didn't understand until much later. So I have to doublecheck:

Do you mean,

• If e.g. teaser has no CE display
• and teaser has no core display-mode for layout-builder setting-check
• then do not check "default" core display-mode for layout-builder setting-check, but instead immediately assume that the default CE should be used?

There are some complications with that.

First:

It will be impossible to use layout builder with the "default" view mode... unless a CE display for "default" view mode is explicitly created and disabled. IMHO we should heavily discourage this, because it is not intuitive. The "default" core display-mode also cannot be disabled, in practice. There is no UI for this (and a few months ago, you explicitly reverted a change where I made the default CE display disable-able - which I now agree with). We haven't yet tested how our UI behaves with a disabled default display.

So, in summary: it will be practically impossible to use a layout configured for the "default" view mode, to build CE displays. Which may be an issue in itself(?)

But also - secondly:

The update path will be more complicated for our internal project. And therefore, I suspect, also for other existing sites. It will not be "if you want no hassle at upgrade time, just execute this code" anymore. (See README)

We have several entities/bundles that are rendered in "full" view mode, and expect to be built using a Layout, which is set up at the "default" view mode.

This will stop working. Therefore,

• During the update to v3.0-stable, we will need to take that layout from "default" view mode and copy it to "full" view mode.
• (Note if we were using this layout to also render non-custom-elements, we would need to keep the "default" layout too. Or change more code.)
• Or alternatively, we will need to explicitly disable the "default" CE display. But we don't want that in 'normal' sites (see above), and it can influence other non-default view modes too -- leading to more auditing of our sites.

Because of this reason, I'm going to push back a little here, propose to also check the default core-display with a changed first-bullet-point:

• When current view-mode (e.g. teaser) has no CE-display, first check the relevant core display-mode (i.e. teaser if it exists/is enabled; otherwise the default core display-mode) for layout-builder setting-check
• When current view-mode (e.g. teaser) has a CE-display, NEVER fallback to core display-mode for layout-builder setting-check.

This is the naive assumption a site-builder would have on how things should work.

I'm not totally sure about that.

When a site builder sees the "Manage Custom Element" screen, and the CE display for "teaser" is not enabled, then they see no information at all about "Teaser". There is no "Teaser" tab. So they also don't see that Layout Builder is active for "teaser".

So they will assume that the "Default" CE display is used, instead. They will not think about core display-modes.

I believe we have to tell them that this is not the case (but layout builder is instead enabled for view mode "teaser"), in an explicit extra message.

I believe we can/should do that, regardless whether we also check the layout builder setting on the "default" core display-mode. So there's not a huge difference for the naive site builder, whether or not we also check the "default" core display-mode.

---

I created a MR and set it to "needs review". What is done, is the code (according to what our internal projects are running now == my proposal), the tests, and the README changes (describing the "my proposal" situation).

What is not done yet, are the messages in the form which point to Layout Builder; there's still a @todo in the code.

* This change is massive and contains tons of docs and tests to review. I am not going through that now, I think it would be not a good use of time. But instead wrap my head around it on how it should it be first. Let's better discuss this and then make the MR ready.
* Let's better leave out the update path here? We don't need to ship with an update path for 3.x. I know we need one internally, but I don't want this to complicate the change or the module code and README. Better stay simple.
* The MR re-adds worrysome notes about UI being buggy. Is this some glitch?

Anyway, this comment worries me:
> never replace the fields list by a 'Manage layout' button (because now, in 100% cases of when the CE display is visible, Layout Builder is not used anymore

Let me describe again how I see this:

1. Since this ticket was created, something important changed: We now always have a CE-dipslay-mode config, since the object (not config) is auto-created when missing. So the case "config is missing" is nothing we need to care about any more.

2. Thinking about use-cases again, I think it might be a valid use-case to enable layout-builder on the core display mode, but to not use it on the CE-API output. Thus ideally, we make it configurable to use it or not.

So I'd propose this behavior:

* When a CE-config for view-mode X is edited, and X has layout-builder enabled in core, show a checkbox to allow also enabling it for the CE-config.
* When a CE-config for view-mode X is edited, and X has layout-builder is NOT enabled in core, don't show the checkbox and auto-disable it.
* The above should apply for regular view modes like "teaser" but also for the "default" view mode.
* When the "X" core display is not existing, we fall back to checking the core "default" display config for layout-builder (both checkbox and setting). (as it should be working automatically when loading it)
* When X has no config and the default-fallback kicks in for CE-displays, we follow the above behavior. X must be "layout-builder" enabled in the "X" core display + fallback to default - ONLY then the checkbox is shown. When the checkbox is ticked, it also takes it.

--------------

Reading upon your comment - I think I just came to the same conclusion as you did, we should keep checking the default - but an additional checkbox would be good imo.

If you want to have the README changes moved to a separate PR, I can do that.

If you want to have the test moved to a separate PR, I can do that.

The code changes are not huge.

There is no update path in code.

The MR does not re-add a warning about the UI being buggy, it just moves it out of an 'if' construct.

1. Since this ticket was created, something important changed: We now always have a CE-dipslay-mode config, since the object (not config) is auto-created when missing. So the case "config is missing" is nothing we need to care about any more.

I do not think anything changed since this ticket was created.

The case "config is missing" == "the CE display is not enabled/visible in the UI" == "the default behavior of an auto-created CE-display-mode" is a thing. We need to define it, no matter what we call it.

Thank you for outlining your new thoughts, now I can think about it and summarize from my point of view:

In your original proposal, as implemented by me:

• If a CE display "exists / is visible in the UI", we never check layout builder.
• If a CE display "is missing / is not visible in the UI", we check if LB is enabled and act accordingly. If it is not, we fall back to the default CE display.
• Consequences:
  • It is impossible to build using LB, when specifying the "default" view mode. This is strange, but IMHO not bad: you can still build using the "default LB configuration", by specifying any view-mode (really any random string) for which no CE display exists.
  • The fact that LB is enabled for view modes which are not visible in the UI, is not apparent. (This is the comment that worried you? But it is a direct consequence of the proposal.) We can 'solve' this with an UI message saying "Layout Builder is enabled for X, Y and Z view modes" when showing the default CE display.
  • People need to go to the core-display screen, to enable Layout Builder.

In your new proposal:

• If a CE display "exists / is visible in the UI", we have some extra checkbox / config-option to
• Consequences:
  • We have an extra config option on the CE display object, which can go out of sync with the LB settings. (We need to accommodate for that config option being true, while LB does not exist.
  • A CE display object needs to be created solely to have this single config option be enabled, because it's the only way to use Layout Builder now.
• If a CE display "is missing / is not visible in the UI", we never check layout builder and always fall back to the default CE display.

I still like the original proposal because I don't like unnecessary/duplicate saved config info, but.... more importantly, I thought that YOU did not want this extra checkbox, from the beginning. Which is why I implemented the current solution, according to your spec.

I do not see the "Consequences" as a big deal.

I will add the following response, just for completeness / in case there was a misunderstanding:

2. Thinking about use-cases again, I think it might be a valid use-case to enable layout-builder on the core display mode, but to not use it on the CE-API output.

Obviously, yes. In your original proposal, this is always the case when a CE display "exists / is visible as a tab in the UI".

As discussed offline: we're going for the new proposal.

It definitely makes the UI more intuitive.

Now for review.

I'll make a CR.

This seems great! Besides the minor comments above which is mostly naming and wording, this is all great and ready to go. I'd be fine with merge + address those things in a follow-up also, or just before merge. But generally this seems ready!

Implemented requiested changes (Rename forceLayoutBuilder; implement requested description changes.)

wow, that was fast! Changes seem all great, so is the final MR! Merging, great to see this land!

I made a mistake:

The "Save this screen first" text next to the Manage Layout button is always visible, even if the Manage Layout button is not.

I don't think it's super important how this looks, so I am going to just wrap both the button and the message in a fieldset (to make the message hide-able at the same time as the button).

Automatically closed - issue fixed for 2 weeks with no activity.