Make menu trail behaviour in SystemMenuBlock optional

MR 12375 is up.
I added tests for the active trail functionality, but I didn't do one for the #states handling in the UI. Is that needed?

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1? Effectively it creates an empty menu if the start level is greater than 1, the menu is set to expand all items, and ignoring the active trail is enabled.

    // For menu blocks with start level greater than 1, only show menu items
    // from the current active trail. Adjust the root according to the current
    // position in the menu in order to determine if we can show the subtree.

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1

We might need both #states and config validation for that?

Added more #states handling and the config schema validation constraint.
Also added functionaljavascript test for the form.

Only thing I'm wondering here is whether instead of the negative we could use 'Use active trail' or similar.

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Yeah, I went with the negative to make the upgrade path easier (NULL being the same as FALSE), despite that it makes the wording maybe a bit awkward.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided. We'd probably also have to disable those fields when "track active trail" is unchecked, because #states can't change field values. Then in the form submit handler, we'd have to interpret those NULL values as level = 1 and expand_all_items as TRUE.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

As in NULL and TRUE would be treated the same? I don't remember doing this but it seems possible. However:

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided.

Yeah it starts getting complicated, good to think about how much work it would be before doing all the work.

Part of the problem might be the 'active trail' wording which is really an arcane internal menu system concept.

What about this for the checkbox label:

Show the same menu markup on every page

With description:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

Made the text changes and pushed. Thinking about this now, though:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

The class added in the core theme templates is menu-item--active-trail. There's also an is-active class added to a menu link if it is the current page, but that is added through JS. Still I wonder if "active" is not informative enough? Also, I'm not sure what the antecedent to the final "it" in the description text is.

My attempt/suggestion for an edit:

When this is checked, the menu will have the same markup on every page. Otherwise, the current page's position in the menu structure will be calculated and an "active-trail" class added to menu links in the current page's menu hierarchy.

That looks good. I couldn't think of a good wording for 'the current page may or may not be in the active trail' but ""active-trail" class added to menu links in the current page's menu hierarchy." covers it implicitly really well.

MR updated per #12.

This looks great to me now.

The last remaining question for me is the nullable config. It avoids having to have an upgrade path, but it means saving menu blocks in the UI will gradually introduce the new config key over time. The other option would be to make it not nullable, and have a config entity update to add the default everywhere, as well as a hook_entity_presave() for exported config.

After discovering 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active I think this is probably the way we should start doing things though - e.g. allow a gradual update over time with runtime code compatibility.

Yes, the issue with LB overrides mentioned in 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active , among others, is why I wanted to avoid doing an upgrade path. I think it's okay to have the config key introduced over time on block saves?

For block entities specifically, I wonder if we can change get()<code> so that calling <code>get('settings') will merge in configuration from the plugin collection. That way additions in defaultConfiguration() from the plugin will be picked up in the loaded block entity. That doesn't really change anything here, but it could maybe apply to other situations?

The issue with this is how often do these blocks get resaved?

The issue with this is how often do these blocks get resaved?

Here, it doesn't much matter because NULL/undefined is the same as FALSE for the new property, and FALSE is always a valid value.

It'd be an issue for any new block property that doesn't work like that. What I suggested in #15 could help address that, but if there are constraints where the default value is not valid with existing configuration, that would be another problem.

I was more concerned about how long we'd need to handle the missing config.

After looking at the actual code it's not really a huge concern.

@catch asked my opinion on the nullable approach. I think it is a good one because the new key is not added during a config import so you're not getting unexpected changes. The one thing that gives me pause is how are we going to tell any modules or recipes that supply this config that they need to update. I.e. will we leave tis key as optional...

@alexpott I think we'd need to leave the config key as optional permanently if nothing happens after this issue. We could open a follow-up to make it non-optional and try to implement that using the to-be-decided APIs in 🌱 [meta] Just in time updates Active and 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active though.

Updated MR per feedback and adjusted tests.

Updated the IS proposed resolution to match the work done so far.