CultureInfo should independent from local computer settings (Lombiq Technologies: OCORE-86)

[hishamco]

Fixes #11228

[hishamco]

Seems I need to rename LocalizationOptions to avoid naming collision

[hishamco]

@sebastienros I just use CultureSettings enum to make clear, but I never mind to replace it with a boolean value, one more thing I just moved CultureLocalizationOptions into OC.Localization.Abstractions all the options that we are using are placed in their modules, but while this kind of option could be use in any localization implementation I think it's fine to place it in OC.Localization.Abstractions rather than OC.Abstractions. Another thing I suggest to use the system culture instead of user-selected culture by default, similar to what CultureInfo is using

If sound looks good I will remove everything I added related to ILocalizationService

[jtkech]

@hishamco

Take care, about CultureInfo the user is not an OC user but an user at the machine level, that's why we were talking about System settings.

There are 2 ctors CultureInfo(string name) and CultureInfo(string name, bool useUserOverride), the first one is the same as using CultureInfo(name, useUserOverride: true), so if we assume that the default is when using the first ctor, the default is then useUserOverride = true.

So the 2 defined values of the current enum Default, User is confusing because of the above. Also, I have nothing against using enum but not when it is to retrieve a value to be used as a bool.

So to not be confused with the "user" we could name this option field UseSystemSettings whose default would be true, as I remember @sebastienros suggested IgnoreSystemSettings but whose default value would be false and we would need to pass useUserOverride = !option.IgnoreSystemSettings.

Not sure we need to bind this IOptions to the configuration (e.g. from appsettins.json), if so and if we bind this field individually, if we use UseSystemSettings we would need to pass to the GetValue() a default value of true, if we bind the whole IOptions class (as we do many time) we just need to specify in the IOptions class a default value of true for this field.

Or if we use IgnoreSystemSettings, as said above, we would just need to pass useUserOverride = !option.IgnoreSystemSettings

Maybe better to name this IOptions CultureOptions and define it in OC.Abstractions, where we define CultureScope, for example this IOptions would be used each time we use CultureScope.

[hishamco]

Take care, about CultureInfo the user is not an OC user but an user at the machine level, that's why we were talking about System settings.

Sure

There are 2 ctors CultureInfo(string name) and CultureInfo(string name, bool useUserOverride), the first one is the same as using CultureInfo(name, useUserOverride: true), so if we assume that the default is when using the first ctor, the default is then useUserOverride = true.

Oops, seem I made a mistake here, I presumed it's a default culture by default

Or if we use IgnoreSystemSettings, as said above, we would just need to pass useUserOverride = !option.IgnoreSystemSettings

I never mind with this option as I mentioned above

Maybe better to name this IOptions CultureOptions and define it in OC.Abstractions, where we define CultureScope, for example this IOptions would be used each time we use CultureScope.

It was there before, but while all the modules adding the options in module project and CultureScope is using CultureOptions that's why I move it into OC.Localization.Abstractions

[jtkech]

@hishamco

If we assume that IgnoreSystemSettings is a good name for the configurable option, because it doesn't contain the term User which may be confusing as here it is not an OC user, and because it has a default value of false, here I was just suggesting to use this good name as much as possible, also for the names of method params, for example AnyMethod(... , goodName: options.GoodName).

But no problem, was only a simple naming suggestion, otherwise LGTM.

The only thing is that if we provide a concrete value e.g. in appsettings.json, do we want it to be taken into account even if OC.Localization is not enabled? If no that's okay, if yes the config binding should not be kept in the OC.Localization startup.

[hishamco]

Agree with you @jtkech as I said before, but using the name UseSystemCultureSettings or UseSystemSettings with default value true will be much better if all agree

do we want it to be taken into account even if OC.Localization is not enabled? If no that's okay, if yes the config binding should not be kept in the OC.Localization startup.

Agree, but let me do some tests and update the PR

[jtkech]

@hishamco Did you tried it? If so I will approve ;)

[hishamco]

I just asked you in case if you have anything else, I will add a missing docs then do a final test after latest commit

[hishamco]

@jtkech seems we forgot to apply the same settings in SetDefaultCulture(), one more thing that I was did it right but you confused me ;) may be because the naming

UseSystemSettings != useUserOverride IgnoreSystemSettings == useUserOverride

For that I remove the negation as it was and everything is fine

[hishamco]

I'm think to do a last tweak for the added extension by adding OrchardCoreRequestLocalizationOptions for doing the heavy lifting, then we can use as the following:

var cultureOptions = serviceProvider.GetService<IOptions<CultureOptions>>().Value;

var requestLocalizationOptions = OrchardCoreRequestLocalizationOptions(ignoreSystemSettings: cultureOptions.IgnoreSystemSettings)
    .SetDefaultCulture(defaultCulture)
    .AddSupportedCultures(supportedCultures)
    .AddSupportedUICultures(supportedCultures);

app.UseRequestLocalization(requestLocalizationOptions);

[jtkech]

@hishamco

@jtkech seems we forgot to apply the same settings in SetDefaultCulture()

Yes as I mentioned here https://github.com/OrchardCMS/OrchardCore/issues/11228#issuecomment-1105795875

one more thing that I was did it right but you confused me ;) may be because the naming

Just saw that you now use new CultureInfo(culture, ignoreSystemSettings) which is wrong as by default when just using new CultureInfo(culture) it doesn't ignore system settings, so if it was not working it would mean that something else is wrong. But never sure at 100%, will look at it tonight.

[hishamco]

Yes as I mentioned here https://github.com/OrchardCMS/OrchardCore/issues/11228#issuecomment-1105795875

May be I didn't notice that, coz all the time I'm trying to focus on the PR comment, sorry for that

Just saw that you now use new CultureInfo(culture, ignoreSystemSettings) which is wrong as by default when just using

It's not wrong because the default value of ignoreSystemSettings is false which means means don't use user override, Am I right?

just using new CultureInfo(culture) it doesn't ignore system settings,

Because the useUserOverride is true, in our case if ignoreSystemSettings is true it should ignore the default settings

https://referencesource.microsoft.com/#mscorlib/system/globalization/cultureinfo.cs,325

[jtkech]

Yes I know https://github.com/dotnet/runtime/blob/5481c4bfe6ced9000c05692163a019739c912677/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs#L158-L162

Because the useUserOverride is true, in our case if ignoreSystemSettings is true it should ignore the default settings

Because the useUserOverride is true by default, meaning it will use system settings, and because ignoreSystemSettings is false by default (if not configured/overridden), we should use new CultureInfo(culture, useUserOverride: !ignoreSystemSettings), so that it works as before if the option value is not configured (e.g. in appsettings) and not overridden by code (e.g. in a s.Configure<>()).

So if IgnoreSystemSettings is configured/overridden to true (which is not the default) the result will be new CultureInfo(culture, useUserOverride: false) which is right as here we want to ignore system settings, so we don't want to use user override (meaning we don't want to use system settings).

[hishamco]

Nooo, you are right Jean, I just confused by the CultureInfo docs

so we don't want to use user override (meaning we don't want to use system settings).

That's the point, again sorry for inconvience and ignore my above comments

[hishamco]

Believe it or not the IgnoreSystemSettings is confusing especially we you read the CultureInfo(string, bool) docs ;)

[hishamco]

I need to do a final quick test again

[jtkech]

Believe it or not the IgnoreSystemSettings is confusing especially we you read the CultureInfo(string, bool) docs ;)

Yes I agree ;)

[hishamco]

Everythings looks as expected

[hishamco]

FYI I created a PR in aspnet repo while I'm working on this feature, finally it got merge. I might need to open a draft PR to change the API with the one that will be built-in in .NET 8.0

https://github.com/dotnet/aspnetcore/pull/44282

[Piedone]

Great!