Introduce DefaultCultureAliasProvider

[hishamco]

Fixes part of #18455

/cc @MikeKry

[gvkries]

I feel this approach might be a bit overengineered. I’d suggest simply moving the functionality into the actual interface implementation and removing the static method from the interface. This would eliminate the need for the new CultureInfoWrapper class, which introduces additional complexity, especially with its use of async local ShellScope, which raises performance concerns.
Additionally, I’m not convinced that introducing an ICultureAliasProvider is necessary at this stage. It seems like an abstraction that doesn’t yet provide enough value to justify its inclusion.

[hishamco]

I feel this approach might be a bit overengineered. I’d suggest simply moving the functionality into the actual interface implementation and removing the static method from the interface. This would eliminate the need for the new CultureInfoWrapper class, which introduces additional complexity, especially with its use of async local ShellScope, which raises performance concerns.

I might agree, but don't forget that we have two implementations for ILocalizationService if I recall, @sebastienros , correct me if I'm wrong, so rewriting the same implementation is not suited IMHO

Additionally, I’m not convinced that introducing an ICultureAliasProvider is necessary at this stage. It seems like an abstraction that doesn’t yet provide enough value to justify its inclusion.

If you look at the current localization APIs, there's already a rules provider, which might not be necessary, but it's good for flexibility and extensibility

@Skrypt, I forgot to ping you while you were doing many things in localization

[MikeKry]

@hishamco

as for me - this satisfies my need to add custom localization. Code looks good, and definitely cleaner than before :)

[gvkries]

I might agree, but don't forget that we have two implementations for ILocalizationService if I recall, @sebastienros , correct me if I'm wrong, so rewriting the same implementation is not suited IMHO

I don’t think this is a major issue for such a small feature, but if needed, it could be implemented once as a static helper.
However, relying on the current ShellScope for this kind of logic doesn’t seem like a good design choice.

[hishamco]

it could be implemented once as a static helper.
However, relying on the current ShellScope for this kind of logic doesn’t seem like a good design choice.

I agree, but as you know, there's no service locator pattern in OC AFAIK, that's why I'm using ShellScope

[sebastienros]

I agree with @gvkries that the only change to do should be to convert this static method to one in the interface that can be implemented and customized wholly. The change should be very simple, and whatever was invoking the static method can now resolve the service directly. No need for a CultureWrapper either.

[sebastienros]

To be clear, no need for ICultureAliasProvider either, the GetAllCultureAndAliases will return the union. If people want to change the aliases, they change GetAllCultureAndAliases.

[hishamco]

I never mind, but don't forget that we have two implementations, so no need to duplicate the code twice; moreover, customizing the aliases might be a headache. FYI, we already have similar approaches in SystemRoleProvider and a few others, but I'm open to any suggestion that could be flexible and extensible

[gvkries]

We do not expect that this has to be customized regularly and therefore it does not need to be another abstraction inside OC.
The DefaultLocalizationService may not require the implementation at all and could potentially just return either the .NET cultures, or just the same as the supported cultures.

[hishamco]

We do not expect that this has to be customized regularly and therefore it does not need to be another abstraction inside OC.

I didn't expect further customization in IPluralRuleProvider too

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.Localization.Core/DefaultPluralRuleProvider.cs

while OC is extensible from the ground up, let us do it the way that it should

[sebastienros]

@hishamco it is abstracted. You can replace the whole implementation of the method. You even prove it by saying it will be duplicated in two implementations. We just don't need to split this method in two more concepts.

Someone who doesn't want to see these aliases can replace the method. Someone who doesn't want to list the .NET cultures can replace it.

[hishamco]

I agree with you, may issue that I will find myself duplicating the code on both current implementations, if it's fine, I will update the PR ASAP