Simplify Localization Options and reuse zh-** cultures on setup

[jtkech]

As commented at the end of #11672 (comment) 3 weeks ago ;)

[hishamco]

I presume the fallback will map zh-Hans-CN -> zh-CN and zh-Hant-TW -> zh-TW right?

[jtkech]

No, as already said so many times, the aspnet request localization middleware fallback logic just lookups to the previous hyphen -, so for example we have the following fallbacks zh-Hans-CN => zh-Hans => zh.

[hishamco]

the aspnet request localization middleware fallback logic just lookups to the previous hyphen -

I need to recheck, because this is wrong, we already fallback to the parent elsewhere on the ASP.NET code base

[jtkech]

Hmm, answered too quickly, most of the time our Localization feature is not enabled on setup, so no aspnet middleware unless it is enlisted at the host level.

So yes, here this is the real Parent culture that may be taken into account, but then, as already commented but I don't remember the details, the Parent culture may depend on the OS.

So I'm closing this PR and let you check this ;)

[hishamco]

The RequestLocalizationMiddleware is the only place that used hyphen to get the parent culture, when I look deeply seems someone did that for PERF purpose, but I will propose a PR because Chinses cultures for instance breaks the rule as usual ;)

[jtkech]

Be aware that on setup most of the time the RequestLocalizationMiddleware is not there.

[hishamco]

Be aware that on setup most of the time the RequestLocalizationMiddleware is not there.

Ya, right

[jtkech]

When I said

Be aware that on setup most of the time the RequestLocalizationMiddleware is not there.

Was from memory, didn't take the time to check.

In fact the OC.Setup startup registers the RequestLocalizationMiddleware.

        ...
        if (_supportedCultures?.Length > 0)
        {
            localizationOptions
                .AddSupportedCultures(_supportedCultures)
                .AddSupportedUICultures(_supportedCultures);
        }

        app.UseRequestLocalization(localizationOptions);
        ...

So I will re-open this PR because it just does what we need.

[hishamco]

Was from memory, didn't take the time to check.

That what I want to say, I might check this if I have a time before merge

[jtkech]

@hishamco

Okay, I also updated the OC.Setup startup to use our OrchardCoreRequestLocalizationOptionsas in the OC.Localization module, so that for example the configured IgnoreSystemSettings is used.

But then no culture in the UI because in the Index.cshtml view we inject IOptions<RequestLocalizationOptions> which was no longer holding the cultures.

So it revealed another little breaking change regardless OC.Setup, currently if someone injects IOptions<RequestLocalizationOptions> for example in a view, he doesn't retrieve the cultures used by the middleware. Yes normally it should use ILocalizationService but was possible before.

Anyway I also updated OrchardCoreRequestLocalizationOptions to also update the cultures of the original options. Normally not good to mutate options in a startup.Configure(), normally should be done through the regular options configuration pattern e.g. with a services.Configure<>() or an IConfigureOptions<>, but this is what we were doing before, and any module startup.Configure() is called by only one request when building the tenant pipeline once (we use an async lock), so that's okay.

Edited; So OrchardCoreRequestLocalizationOptions is more like an helper, I may use the updater pattern tomorrow.

[hishamco]

Seems the PR ignored the PR #13201 but no problem I will close it. My concer here is that LocalizationOptionsUpdater is options or it can treated as options builder to retrieve RequestLocalizationOptions with OC specific properties

Anyhow the only thing I dislike that we breaking an APIs without user notice,

[jtkech]

Anyhow the only thing I dislike that we breaking an APIs without user notice,

Don't worry, it was not usable, anyway not recommended and prone of errors, it's more like a fix than a breaking change.