Fix confusion in UI when no CE display exists

I just woke up to the thought of "what ) wrote in this isssue is fundamentally wrong".

It's possible that I was having a mental meltdown (somehow caused by trying to upgrade an internal project and misinterpreting details about why we precreated CE displays inside it, using a beta version several months ago).

I believe there may be something here about "printing a message when a CE Display doesn't actually exist yet", but it is likely minor.

Basically I need more information from my future self.

I un-confused myself and rewrote the summary to reflect the actual issue.

I want to fix this now; it's going to save me time and confusion while working on actual projects.

I've created an MR and will set it to Needs Review for the message / as an FYI, but actually, I will already push this to the main branch.

Because I need it, and because I've already pushed so much form-message changes anyway.

I will still handle requested changes to the message.

On a fresh Core install (e.g. lupus-decoupled), Core auto-installs entity view displays for Default, Teaser and RSS, so the warning shows up. It's a lot of screen space, but... the warning IMHO is really necessary (and it refers to the info message so that it itself can be shorter).

Tweaked message a bit - this is now what is showing on a fresh Core install for articles.

If we don't want to have this message on lupus-decoupled, then we can ship a default CE display for each Core node bundle (and perhaps a CE display for the 'full' view mode too, if we need it). But I guess that's a followup that could wait until we implement proper output for Drupal CMS content types.

I think the message is bad for UX since users opening the screen the first time will have to read it and try to parse/understand what it means for that. Given this, I think for UX we should try hard to avoid this. Also this is not a pattern core introduced anywhere and it'S ususally good to follow core patterns in contrib.

I must say I'm not fully understanding why this message is needed and why it's confusing without. Why is it important whether the configuration you see is acutally saved or not? It should not matter, because either way the same configuration should be applied.

major: if they press the "Save" button without doing anything else, the output will likely change for any view mode that has its own entity view display (because it will be based on the default CE display, from then on).

I cannot follow, could you elaborate what changes? Does core face the same problem?

I cannot follow, could you elaborate what changes?

Situaition: on a certain bundle, you have

• entity view displays enabled for 'default' and 'full' view modes;
• no CE displays

In this situation, custom elements in 'full' view mode will be bult based on the 'full' entity view display. (CustomElementGenerator::getEntityCeDisplay() will auto-create a CE display that is based on 'full'.)

This IMHO is the only logical behavior, and it's v2-compatible.

However,

1. this fact is not visible in the UI. You only see the one "default" (auto-created) CE display, and no tab/local tasks for "Full" etc.
2. as soon as you save the default CE display, this behaviour changes. Custom elements in 'full' view mode will be bult based on the default CE display. This also IMHO is the only logical behaviour.

I am just thinking about this now...

1. can be fixed by showing CE displays in the UI (auto-created if they don't exist), for _all_ view modes that have an entity view display... when the default CE display is nonexistent / disabled. This likely only needs a change to the RouteSubscriber, to also show routes to $path/ce-display/{view_mode_name} if no 'default' CE display exists and the "$view_mode" entity view display exists.
2. can be fixed (only in the UI form) by, if a default CE display is saved for the first time, at the same time also saving those auto-created displays. Then the behaviour doesn't change for those view modes.

That's a whole lot of 'invisible' behavior, so I wouldn't implement that without your agreement... but it feels doable, without introducing extra problems. (Just added the code complication, but that's contained to the UI form.)

I added to the bottom of my previous comment, saying "However, 2. is not perfect for many use cases, either. "

For completeness: "Needs review" status is for my last comment, not for the code.

I am considering just replacing the code by a variant of my "Another alternative" (last paragraph+bullets)... but this hasn't risen to the top of my to-do list yet.

However,

1 this fact is not visible in the UI. You only see the one "default" (auto-created) CE display, and no tab/local tasks for "Full" etc.
2 as soon as you save the default CE display, this behaviour changes. Custom elements in 'full' view mode will be bult based on the default CE display. This also IMHO is the only logical behaviour.

imo this is totally fine.

However, 2. is not perfect for many use cases, either. In a standard Drupal install / lupus_decoupled gitpod, it would mean that as soon as you save the "default" CE display for an article, the "RSS" and "Teaser" CE displays are immediately saved with it (so that custom elements output does not change for those view modes). You very likely do not want "RSS" to be saved, because you just don't care about that view mode.

yes, that'S a bad idea. We shall not auto-save more view-modes.

The current behaviour + message seems totally fine, and I don't see a need to mess with it. It's reliable and also makes sense: In the beginning you only see "default" and it's obvious that is what will be applied to all of them. Does the "default" also have the checkboxes for which view-modes to customize in the bottom? So you need to save that anyway when you want to customize any of those, so seems good?

I took a look at this. Adding message is weird, it's just a workaround to the underlying problem: The auto-creation of the entity default display during rendering is not the same as the entity you are going to save in the UI.

The difference is, in the UI you are always have to start with the "default" view mode. This is what we need to do during auto-creation also.
By fixing this, I get consistent behaviour and there is no need for a message.

-> https://git.drupalcode.org/project/custom_elements/-/merge_requests/118/...

@fago tests are failing now.

Right, the test was covering the changed logic, so it had to be adapted. So I've dig into the test-logic and adapted it.

Re #15:

In the beginning you only see "default" and it's obvious that is what will be applied to all of them.

You have alreaddy discovered that this is untrue. In the beginning you only see "default" and this is not what will always be applied, so it is not obvious. See of the example that I gave at the top of #11. Which is this way, as stated, to be v2-compatible.

I took a look at this. Adding message is weird, it's just a workaround to the underlying problem:

Yes, it is weird (and can be solved better than my initial PR). No, it is not a "workaround to a problem", it is an implementation of your own stated requirement that the behavior of custom_elements v3 should be equal to v2, if no CE display is present/saved in config.

I will repeat the example I gave in #11: if you have

• no CE displays (or v2 of the module installed)
• entity view displays enabled for 'default' and 'full' view modes;

Then the output will be based on the fields from the full display mode, not the default one. Per your own stated requirement of v2-compatible behavior, when no CE display exists.

So: MR!118 does not reflect a bugfix. It reflects a change in the specification.

Now...
If you retroactively think your previous specification is "weird", because it makes for worse UX / you do not want to complicate UX to match the spec... I have no firm opinion on changing the spec. But it needs to be done consciously.

I would really prefer that this was done in a dedicated ticket whose description mentions this change in behavior, and which discusses e.g. why this does ( / not) warrant a new major release, and the (new) need to explicitly create CE displays while upgrading from v2, if you want to keep the same behavior.

I'm changing to Needs Work to decide on a way forward, and if you really want to do this, to at least mention/acknowledge these things. If you prefer, I can have a look at changing the README section discussing upgrades.

From https://git.drupalcode.org/project/custom_elements/-/commit/c27b1ff37bbe..., I see that you do not want to give your users help upgrading from v2.

Fine. Your decision.

But this change at the very least needs a change record.

> If you retroactively think your previous specification is "weird", because it makes for worse UX / you do not want to complicate UX to match the spec... I have no firm opinion on changing the spec. But it needs to be done consciously.

indeed. I was aware of this UX problem my previous opinion created. I'm now aware and I do think keeping the v3 simple+sane and is much more important than keeping the 2.x behavior intact here.

> From https://git.drupalcode.org/project/custom_elements/-/commit/c27b1ff37bbe..., I see that you do not want to give your users help upgrading from v2.

I want to keep the README brief and focused. Additional help can be available in tickets or other follow-up material, where people can collaborate and exchange code-snippets.

I was not thinking this needs a change-record, since it will only influence behavior when no config is created, what I'd not expect anyone to do, however yeah, some people will still do, so I agree a change record makes sense. will add one.

https://www.drupal.org/node/3527054 → - created! Moving on with this then.

fago → closed merge request !106

fago → closed merge request !106

I was not thinking this needs a change-record, since it will only influence behavior when no config is created, what I'd not expect anyone to do,

People are doing this by simply installing/enabling the module, unless that is done through a recipe that includes default config. So, an unknown amount of sites is going to be affected.

(OK. I'm not against merging, and this may well be an overall improvement. I'm just really conservative / hesitant changing behavior when the reason wasn't spelled out / when there is seemingly-unaddressed confusion.)

I see, thanks for clarifying! Yeah, I do think this helps to simplify things in the end, sry for leading into the other direction behavior before.

Recipes also started to include some config, but yeah, I agree some people might be using it like this, so good to have the change-record. Thanks for pointing that out!