CultureInfo should not depend on local computer settings

[sarahelsaig]

Describe the bug

When the library sets the culture using new CultureInfo(culture) you can get unexpected results if this is also the system's culture and data formats were customized. This could be a problem if the server wants one data format for internal administrative use that's different from the culture's canonical format.

For example you have an en-US server with date format set to ISO 8601 (this is actually what we have on our CI - this problem is most painful while doing UI tests). Then if you want to display a date in OC with the site set to American locale, it will display it using ISO 8601 which is not what you'd expect. The site's internationalization should not depend on the local format settings.

To Reproduce

This repro assumes you have Windows 10 with en-US region settings:

Steps to reproduce the behavior on Windows:

1. Go to /Admin/Settings/localization and make sure your site is en-US.
2. Create a new view that contains @T["Date: {0:d}", new DateTime(2022, 02, 21, 12, 0)]
3. Navigate to it, you will see Date: 2/21/2022.
4. Go to Start Menu > Settings > Time & Language > Region > Change data format
5. Change the Short date to "2017-04-05".
6. Restart the server.
7. Visit the same page again, you will see Date: 2022-02-21.

Expected behavior

You should still see Date: 2/21/2022.

Speculation

I believe the problem can be solved simply by altering OrchardCore.Localization.CultureScope.SetCultures to generate fresh stock cultures. So from this:

        private static void SetCultures(CultureInfo culture, CultureInfo uiCulture)
        {
            CultureInfo.CurrentCulture = culture;
            CultureInfo.CurrentUICulture = uiCulture;
        }

Into this:

        private static void SetCultures(CultureInfo culture, CultureInfo uiCulture)
        {
            CultureInfo.CurrentCulture = new CultureInfo(culture.Name, useUserOverride: false);
            CultureInfo.CurrentUICulture = new CultureInfo(uiCulture.Name, useUserOverride: false);
        }

[Skrypt]

I'm not sure, to be honest. If the DateTime format is changed intentionally then it is because you want to use that specific DateTime format. Then it would defeat the purpose of having it changed.

Generally, if you have an application that requires this DateTime format to be changed then an option is to isolate it inside a Docker container and to not use Orchard Core in the same container. If these 2 environments have incompatibility then make them use a different one ...

Personally, I prefer leaving the option to change the DateTime format intentionally, than to restrict it.

[sarahelsaig]

There can be many reasons to change the system date format that have nothing to do with the website. I agree that using a container is a valid workaround, but it feels like an overkill if it's otherwise not needed.
How about making useUserOverride a configuration option with default true value? Then it could remain as is (though I still doubt many people would want that) but can be disabled sitewide if not wanted.

[Skrypt]

That would make more sense.

[hishamco]

Go to Start Menu > Settings > Time & Language > Region > Change data format

I don't think it's good idea to change the date time settings for a server because may it affects the clients

Second if you this change need to be applied it could be in cultures confugration, no need to new up an instance of CultureInfo here in each call

[ns8482e]

Enabling Location feature should solve this

if you doesn’t want to enable localization feature then set UI culture in program.cs

related issues

#10655 (comment)

[sarahelsaig]

It doesn't help. The Localization feature was already enabled:

[Piedone]

The server perhaps shouldn't customize the date format but the app should never rely on the server's configuration (apart from the basic runtime requirements) either. So just as we don't rely on the server's locale, we shouldn't on the date format either. Also, this is not just an issues in production but in local development too. The point is, that if you want to have consistent date displays, you need to do what Dávid has demonstrated.

[ns8482e]

@Piedone @DAud-IcI OC the functionality works as desired and depends on how ASP.NET reads culture data from OS.
However OC also detects browser's culture and renders output as desired - if Localization is enabled and configured.

• If you don't enable Localization it will use server's locale. You can change this behavior by changing CurrentCulture and CurrentUICulture in your Program.cs to meet your custom needs
• If you enable Localization add supported cultures in Settings -> Localization -> Cultures, Orchard Core will pick the culture based browser's culture priority and date/time will be rendered based on browser's culture format.

In above scenario - IMO you are really changing Short date format for the browser :)

[sarahelsaig]

@ns8482e As I've said in my previous comment Localization is already enabled. This issue is about how Localization behaves when it automatically sets CurrentCulture and CurrentUICulture for you.

In above scenario - IMO you are really changing Short date format for the browser :)

I doubt it. The browser only sends the culture code (Accept-Language: en-US,en;q=0.5 header) and doesn't communicate a desired shot date format to the server. Yet the DateTime is already rendered into HTML string on the server side so the client's date format has no effect at any step of the process.

[hishamco]

To make it clear, ASP.NET Localization middleware already sets the cultures the same way that we did without setting useUserOverride to false, so I suggest to file an issue in ASP.NET Core repo

[ns8482e]

The browser only sends the culture code (Accept-Language: en-US,en;q=0.5 header)

Correct I meant the same as per my comment below

Orchard Core will pick the culture based browser's culture priority

The culture locale sent by your browser - you customized the short date format for that locale so ASP.NET renders what you asked to do.

[Piedone]

Which part of Accept-Language: en-US,en;q=0.5 specifies a short date format?

[ns8482e]

If you add support for another culture (e.g. fr ) in OC - and change your browser's locale priority (to fr) to new culture it will change the date format for that culture.

[Piedone]

The problem is not the culture. The problem is a date format within the specified culture. It's all en-US but depending on the server's settings, the same en-US culture can use different date formats with the same format string.

[ns8482e]

Which part of Accept-Language: en-US,en;q=0.5 specifies a short date format?

He changed the short date format for the en-US on the server - and browser is requesting to render on en-US
@hishamco said it's how ASP.NET renders

[Piedone]

That we know. The point of this issue is being able to configure the date time format used by Orchard for a given culture to be reliably exactly the same everywhere, regardless of server settings. The culture settings of the server shouldn't matter.

[Skrypt]

@Piedone What happens if you want to change the DateTime format intentionally then? The reason why we kept it like this is that this is how it is handled by the ASP.NET Localization middleware. Here, what is suggested is to hack the behavior of the middleware. While I see that this could lead to an issue changing the DateTime of the En-Us culture, for example, this is not supposed to affect database transactions, this should only affect DateTime display, which is the goal here. I can't agree on the proposed change and I believe that I already had this discussion with @sebastienros and @jtkech a while ago.

As far as environment variables go, I think that if they are not the same from one environment to another then that's a configuration of the environment issue to me.

[hishamco]

@DAud-IcI try to reproduce the issue in a simple ASP.NET Core application, if you get the same result, you can file an issue in ASP.NET Core repo, otherwise it might related to DefaultCalendarManager

[Piedone]

I don't see why we'd hack the ASP.NET Localization middleware. This is about changing the behavior of CultureScope, part of Orchard. It's already an Orchard-specific concept. But even if we do the change elsewhere, the point here is to make the display of dates consistent, not about storage or any other backend behavior.

Culture settings are already an environment variable. To make the app not depend on those, we have culture settings in Orchard (and similarly we have time zone settings). We don't say that these should also be provided by the environment but rather we override them with Orchard settings. Here, we're talking about a subset of culture settings. Why would that be any different?

[ns8482e]

IMHO changing regional settings and that gets reflected on ASP.NET applications is a feature and expected behavior. If you don't want that change then why do you even change default settings on server at first place?

If you have specific requirement to have all other asp.net app (any other apps) read regional settings but not specific orchard core app then it's custom use-case

You can achieve it by modifying Program.cs or have your own middleware run before ASP.NET's Localization middleware to run the code suggested in issue desc or run in container.

[Skrypt]

@Piedone And this CultureScope uses the same mechanic as the Localization Middleware to be consistent.

[sebastienros]

👀

[Piedone]

@ns8482e I don't want to change this on the server. In fact, this is something out of my control (while Dávid mentioned our CI server, the problem is not its configuration but rather that it, the developers' machines, and the production server may all be different, and they are), and the problem is that it's not consistent in all environments. Hence, Dávid and I am presenting the point of making the app not depend on this particular configuration of the server, just as it does not depend on any other configuration of the server either.

Putting aside for a moment what the Localization Middleware does and why, and whether we should do the same: Which one of these do you agree with and why?

1. I want the app to display dates according to server settings, i.e. potentially in different formats than other environments.
2. I want the app to display dates according to the app's settings, i.e. in the same format everywhere even if environment settings would potentially configure something else.

For now, forget about implementation details, or whether it should be part of Orchard, or anything else. I just want to understand whether we have some common ground and thus whether talking about specifics makes sense.

👀

[Skrypt]

I may refer to this issue: #5595

Related to that, there is also the fact that each OS has its own culture code listing.
.NET Core compiles the OS culture dictionary in its framework so that it is not resolved each time on runtime.

There have always been discrepancies between different OS and that's one of the reasons why the DateTime format has always been modifiable under Windows. Unless all OS would be using the same standardized cultures it's always going to be a challenge to be able to make a UI to be consistent from one OS to another. This is mainly why I'm saying that the issue lies deeper and that this should be handled by making sure that you compare apples with apples when you are trying to verify if the UI is displayed correctly. Of course, changing a DateTime format on a system will cause the UI to be different because you are not comparing it with the same OS environment variables. And, for that matter, running CI tests for example need to be compared with a host system that is equivalent to the CI itself. So, for that matter, there is always Docker.

Also, I believe that on Windows, having the ability to change the different string formats has been made to be able to fix issues caused by these discrepancies. So, removing the ability to change these formats will also cause issues for others.

[Piedone]

What issues do you mean? Just to clarify, I'm not against changing this format. I'm for being able to change it in Orchard.

While having consistency across different OS families is indeed an issue, and something I agree that ultimately .NET needs to solve, that's out of scope here. This particular issue with the short date format is a Windows-only one, and the parameter Dávid mentioned only has an effect on Windows. We're only talking about differences of date settings between different Windows installations.

[sebastienros]

I have thought about it too. And to drive a parallel with Timezones management, installing OC on a system should not behave differently based on the system's settings, as much as it can.

To fix this problem I believe we should be able to at least create a standardized culture based on the identifier, that is not changed by local settings. And optionally provide a way to change these settings for any culture, probably from configuration only.

Has anyone found a related issue in ASP.NET? This ought to be the same there with request culture providers.

[hishamco]

If I'm not I saw an issue related a date format long time ago, hope to find it

[jtkech]

That's pretty risky but that could work.

Didn't say to use it if this is what you meant ;) Just showed the aspnet code that doesn't use useUserOverride: false, this to create cultures when setting the RequestLocalizationOptions.

[Skrypt]

I just meant that it is risky to change the behavior of the ASP.NET Middleware. This could work but that seems hacky unless it is supported on every level which it doesn't seem to be at first sight. And we both know that doing this leads to a lot of needing hacks everywhere which I don't really like to encourage. I'm not going to approve the pull request for that matter. But, else, let's find a compromise if people need this. To me, it's simpler to not override the Culture settings on a server. And if it is because of a particular developer PC culture setting then it is a really specific use case that we can see that the ASP.NET team did not take care of.

So, should we?

The proper resolution would be to open up an issue on the ASP.NET localization middleware repository but that can take a while to resolve in that case.

[hishamco]

Here for example how SupportedCultures are added to the options when we use AddSupportedCultures, we can see the usage of new CultureInfo(culture) without using useUserOverride: false.

@jtkech that's what i meant here

The proper resolution would be to open up an issue on the ASP.NET localization middleware repository but that can take a while to resolve in that case.

If we open I'm sure it may take long time probably years ;) coz all of the team that working on localizations are not active in the repo, probably because the feature is stable or they moved into another ASP.NET Core components

[sebastienros]

Based on the fact that aspnet uses overrides we need to keep the current behavior.

[hishamco]

So, let use keep the current behavior, but someone can tamper the localization middleware to make this happen. @Piedone can we implement this in Lombiq or OCC if there's no plan to implement it here?

[Piedone]

Where exactly is ASP.NET using overrides consciously?

[sebastienros]

@Piedone https://github.com/dotnet/aspnetcore/blob/b2679bf0bfe96cd3f8e1663868f5c87a1b0dbadd/src/Middleware/Localization/src/RequestLocalizationOptions.cs#L121-L132

[Piedone]

That looks more like a default behavior when you're specifying cultures only by their names. Otherwise, this is not enforced and you can add CultureInfos to SupportedCultures directly.

[jtkech]

Yes, see #11228 (comment)

Here for example how SupportedCultures are added to the options when we use AddSupportedCultures, we can see the usage of new CultureInfo(culture) without using useUserOverride: false.

And then the winning culture is set here https://github.com/dotnet/aspnetcore/blob/b2679bf0bfe96cd3f8e1663868f5c87a1b0dbadd/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L129-L133

Hmm, one solution would be to not use AddSupportedCultures(), AddSupportedUICultures() and SetDefaultCulture() but our own extensions that would then use new CultureInfo(name, useUserOverride: false).

updated: Maybe a configurable option for the useUserOverride value to use.

[jtkech]

@Piedone @DAud-IcI Did you see the end of my previous comment? Maybe a way to revive this issue ;)

[Skrypt]

While rewriting the extension methods might fix the issue do we really want to do that in Orchard Core? We tried to avoid doing such things when we wrote the actual Localization middleware module because we were doing too much fiddling here and there while we should rely on the ASP.NET Middleware. That would add a lot of code for a unique use case that can be fixed by not changing the actual culture settings in Windows.

[Piedone]

@jtkech, yes, this would be an optional configuration. @Skrypt as I elaborated under #11228 (comment), environment settings is not something possible to enforce if we go beyond the very trivial use-case.

[hishamco]

@jtkech I tried simialr approach when I planned to start this PR, but how you can avoid setting the cultures using CultureScope or when someone set them using CultureInfo.CurrentUICulture?!!

[jtkech]

@hishamco

how you can avoid setting the cultures using CultureScope or when someone set them using CultureInfo.CurrentUICulture?!!

He should not use CultureInfo.CurrentUICulture unless he knows what he does. Then CultureScope would need to also use useUserOverride: false as suggested by @DAud-IcI, or if it is a confugurable option use it as an additional ctor parameter.

So here we would not change the middleware, only the way we set options, and the culture scope as suggested at the beginning of this issue, but as said by @Skrypt not sure we want to override the default behavior, I don't know so I let others decide ;)

[hishamco]

He should not use CultureInfo.CurrentUICulture unless he knows what he does

Nothing, prevent him from doing that, moreover everyone TRUST localization middleware and know how everything handled

Then CultureScope would need to also use useUserOverride: false as suggested by @DAud-IcI, or if it is a confugurable option use it as an additional ctor parameter.

Right

Again, I see the solution may solve the issue partially, but change the default behavior of the middleware, we may ask @sebastienros to talk with Damian Edwards about this issue to handle it in Localization middleware if it's the right behavior, as @jtkech said let others decide ;)

[jtkech]

@hishamco Okay cool then

Only a little precision about my own comment

He should not use CultureInfo.CurrentCulture unless he knows what he does

I meant he should not update it directly (just let the middleware do it), unless he knows what he does as you do with your CultureScope.

Also, yes the behavior would change but not the behavior of the middleware itself, only the way we set the options by not using their helpers (that don't use useUserOverride).

[sebastienros]

you can get unexpected results if this is also the system's culture and data formats were customized

I can already hear the aspnet devs saying that it would be an issue that Culture settings are not reflecting the system's configuration. Similar to ICU libraries in dotnet... Hence I would agree about a lower level application wide setting that could change the current default behavior or CultureInfo. So this can be global.

[hishamco]

@jtkech I'm planning to submit a PR for this, did you see adding a middleware that sets CultureInfo.CurrentCulture with override option is the suitable solution or do you have another suggestion

Please let me if it's fine to discuss here or via Gitter

[jtkech]

Hmm, as I remember I was not suggesting to update/add any middleware but eventually use an useUserOverride configurable option that would be used when setting cultures, this by using our own extensions in place of using the dotnet AddSupportedCultures(), AddSupportedUICultures() and SetDefaultCulture(). And the option would need to be also passed/used by the CultureScope.

[hishamco]

Ya I remember ;) also we can have "useUserOverride" configurable

[hishamco]

Restart the server.

@DAud-IcI do you mean restarting the PC/Laptop or IIS?

[sarahelsaig]

I meant the Orchard Core web application, e.g. OrchardCore.Cms.Web.exe (as it acts as an ASP.Net Core server).

[hishamco]

I just started a PR, hope it's ready tonight or tomorrow

[sebastienros]

My suggestion was to have a single bool in the app settings like IgnoreSystemCulture (default to false). Not sure it should be in admin screens.