OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.42k stars 2.39k forks source link

CultureInfo should not depend on local computer settings #11228

Closed sarahelsaig closed 2 years ago

sarahelsaig commented 2 years ago

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: image

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

That would make more sense.

hishamco commented 2 years ago

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 commented 2 years ago

Enabling Location feature should solve this

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

related issues

https://github.com/OrchardCMS/OrchardCore/issues/10655#issuecomment-962632483

sarahelsaig commented 2 years ago

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

Piedone commented 2 years ago

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 commented 2 years ago

@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.

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

sarahelsaig commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ns8482e commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

sebastienros commented 2 years ago

👀

Piedone commented 2 years ago

@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 commented 2 years ago

I may refer to this issue: https://github.com/OrchardCMS/OrchardCore/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

hishamco commented 2 years ago

Let me reproduce the issue and see if can I have a suggestion or a quick fix

hishamco commented 2 years ago

@DAud-IcI could you please let me know your languages listed in your browser with thier order?

ns8482e commented 2 years ago

ASP.NET you can override date formats of a given culture using RequestLocalizationOptions via Configure/PostConfigure. Same can be achieved in Orchard Core as well with custom module

hishamco commented 2 years ago

I'm suggesting to add an option an appsettings.json and / or could be from admin settings to enable and disable the customization

Again the default should be UseUserOverride=true as it's the default nature of .NET API, then the user could change it if he/she likes

If all agree on this I will start a PR

Piedone commented 2 years ago

Did you look into what Sebastien said under https://github.com/OrchardCMS/OrchardCore/issues/11228#issuecomment-1050027843?

hishamco commented 2 years ago

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.

I didn't understand the first part about the identifier, but I'm agree to change the settings through the configuration

Skrypt commented 2 years ago

We don't use LCID's, because they differ from one OS to another; we use the culture code if I remember correctly. So, he says that we should be able to create a standardized CultureInfo from a culture code that will not be altered by the system settings "Normalized". But optionally allow changing these settings for any culture through the configuration.

hishamco commented 2 years ago

What if we control the user override settings by a configuration in localization module, so when the localization middleware is configured it will know if we should accept the custom settings or not. Perhaps this the easiest way to accomplish this without much effort

hishamco commented 2 years ago

Related question, how we can ensure that there's no one will set the current culture with the default values of the user settings, I just saw few place such as Content Localization module, unless we use CultureScope to control the culture usage

If anyone else have another thought please share it

sebastienros commented 2 years ago

Is there a way to build a CultureInfo that is not altered by the system settings? This way we could have a boolean to not use the system info's cultureinfo. The extreme solution is to configure all the properties of a CultureInfo, numbers, date time formats, ... which we don't want to do.

hishamco commented 2 years ago

Is there a way to build a CultureInfo that is not altered by the system settings?

This could be done by setting useUserOverride to false

The extreme solution is to configure all the properties of a CultureInfo, numbers, date time formats, ... which we don't want to do.

Much simpler to introduce a new service that uses the default behavior of CultureInfo - which affected by system settings - and the boolean you are talked about could be automatically comes from site settings

This way we align with the current behavior of CultureInfo, if the user need to change he/she can toggle a simple settings like what we already have

sebastienros commented 2 years ago

There is an issue with CultureScope, but I believe it's not the only place where cultures are used, or set, like by the request localization middleware. How would we change that?

Otherwise no need for a custom service, just check the settings when we need to create a cultureinfo.

hishamco commented 2 years ago

like by the request localization middleware. How would we change that?

I will try to investigate on this part

Otherwise no need for a custom service, just check the settings when we need to create a cultureinfo.

Which settings that you refer to? Second thing, this mean when need for check everywhere we change the culture

hishamco commented 2 years ago

Seems you mean

This way we could have a boolean to not use the system info's cultureinfo.

jtkech commented 2 years ago

Here the aspnet localization middleware setting the culture provided by the winning provider

https://github.com/dotnet/aspnetcore/blob/b2679bf0bfe96cd3f8e1663868f5c87a1b0dbadd/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L129-L133

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.

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

Skrypt commented 2 years ago

That's pretty risky but that could work. Though, as we see the useUserOverride param is not taken into consideration at all in the middleware.

jtkech commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

hishamco commented 2 years ago

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 commented 2 years ago

Where exactly is ASP.NET using overrides consciously?

sebastienros commented 2 years ago

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