Breaking shape name resolution regarding menu shape

[sarahelsaig]

Describe the bug

I found this undocumented breaking change regarding the "menu" shape (potentially all non PascalCase shapes).

Orchard Core version

Latest main.

To Reproduce

1. Set up an OC 2.0 site with any recipe that creates a main-menu content item (e.g. Blog).
2. Create a content type with a Liquid part.
3. Create a content item with the Liquid content {% shape "menu", alias: "alias:main-menu" %}
4. Go to Admin > Design > Templates.
5. Create a template called "Menu" with any content.
6. Display the content item, observe that the shape is not overridden.
7. Delete the "Menu" template and create a "menu" template instead.
8. Display the content item again, observe that the shape is successfully overridden.
9. Now do the same again in OC 1.8, observe that in 1.8 the menu shape is overridden by the "menu" template but not the "Menu" template, in other words it's the opposite.

override works	menu	Menu
OC 1.8	❌	✅
OC 2.0	✅	❌

Additional note: btw you can't create menu and Menu templates at the same time in Admin > Design > Templates. You have to delete one first to create the other. Is this intentional? Seems like the template creation is checked case-insensitively, but the template evaluation is case-sensitive.

Expected behavior

Either "Menu" template override should keep working. It would be even better if both "Menu" and "menu" worked. Maybe it should first try the shape type case sensitively and if it's not found then case insensitively? (that's what I do with media library themes in Lombiq.Hosting.MediaTheme here)

If that's not possible, then please document the breaking change in the 2.0.0.md release notes.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[sebastienros]

I personally would suggest it should work with all cases. That would also fix the breaking change.
If the solution is to be case sensitive then the solution would be to use the same case as the content type, which would also fix the breaking change.

[sebastienros]

@sarahelsaig Looked at the linked PR and it seems that the issue is only for admin templates? That would reduce considerably the scope of the problem and help find out when it changed. That could also mean that it's store dependent: Linux vs Windows vs blob storage vs database.

[sarahelsaig]

I agree, the problem is only for admin templates. I did some testing, and it looks like file templates in the Views folder (both cshtml and liquid) are resolved case-insensitively. Even on Linux, so that side is not filesystem dependent.

So I think it would be best if the admin templates get resolved case-insensitively as well. For consistency.

[sebastienros]

I am surprised that admin templates are based on the filesystem, I thought we were storing them in database by default ... Or are they in a single "document" in the database and then it's not an issue with the filesystem but the way we search for them.

[sarahelsaig]

I've investigated further and TemplatesDocument.Templates is already StringComparer.OrdinalIgnoreCase. But somehow this is not respected and the default generic comparer is used instead:

This is a problem when a model contains a dictionary with a setter, because STJ just creates a new dictionary with the default comparer instead of updating the existing setter with the provided values.