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.45k stars 2.41k forks source link

Default and Supported cultures discussion. #3559

Closed jtkech closed 5 years ago

jtkech commented 5 years ago

Just saw the last meeting video about localization.

And re-read our last comments in #3189 saying that by default the options.DefaultRequestCulture is equals to the system culture and that the system culture is also added by default to the supported culture. And that AddSupportedCultures always starts from a new collection.

So, without complicating things, my suggestion is that, when we 1st go to the supported cultures page, we show the current internal supported culture (the system culture), as we already have set the default site culture with the system one. Then the user can remove it explicitly if he want (or not), because i don't say that the default site culture should be always a supported culture.

This because adding the first time a supported culture (and save) is not really just adding a supported culture, it is also removing the system one from the internal collection that we will override. And having a default culture which is supported or not is not exactly the same thing. Maybe not so important because we can still add this removed culture explicitly (or not), but maybe not so obvious.

Finally, if there are separate things, maybe we would need to be able to change the default site culture to a new one, even this new culture is not in our supported cultures list. But maybe this last one would be more confusing than useful.

@Skrypt ? Not a concern for me, just some thoughts after seeing the meeting. Feel free to agree or just close the issue ;)

Skrypt commented 5 years ago

I just looked at the PR from @jptissot and the behavior is still the same on Mac OS where the default OS culture returned is InvariantCulture. So from first installation on Mac OS the default culture returned by CultureInfo.InstalledUICulture will be CultureInfo.InvariantCulture. So, we need to make sure simply that we cover all NRE that could happen with content item localization by falling back to InvariantCulture every time that a content item has a LocalizationPart = null. Though, what I was saying is that it would be easier if that default culture would always be set from the setup so that we never use the System default culture which can return in some case an InvariantCulture. Somehow, InvariantCulture is almost the same as en-US so there is no issue at all. But we must make sure that all our implementations use the same fallback mechanism as what .NET Core does if we want to handle all the use cases that people might require which means supporting also having InvariantCulture as a selectable culture.

At some point I removed the option of selecting the InvariantCulture and proposed that we interpret the fact that a content item with a LocalizationPart = null should maybe propose to use the default site setting culture when we open to edit it. After a second thought I'm not sure this is necessary. The reason I wanted to propose a culture is because I thought that we would require to localize some Fields based on that proposed culture but I think it is the responsibility of the admin culture picker to display the proper culture when editing in the admin UI. So we could be editing a RTL content item in the admin while displaying the UI in en-US because this should be a user setting or simply a cookie value stored for the admin.

Though, confusion here is that if I want to translate a content item in a RTL culture I want to have my Wysiwyg editor set as RTL even if my admin culture picker is set to en-US. So, what I need to determine precisely is if we also have a culture priority also with Fields or should we just rely on the admin culture picker. Either if we use the passed culture or culture picker to localize the item and/or to determine the displayed culture having InvariantCulture should not be an issue... though it just feels awkward to have it as an option since I can't find any use case where I would want to keep my content items as InvariantCulture even if it's almost like en-US. Those content items should always be assigned to a specific culture which should be in most case the current default culture set on the website. There could be special cases where a person would want to switch from en-US to en-GB because he made a mistake but then in that case we should provide a tool to migrate content items from a specific culture to an other.

What I was proposing is that if we find a content item that has LocalizationPart = null then we open it and propose to use the current default culture in it's culture picker. If we have content items in a website that stays as LocalizationPart = null while other content items have LocalizationParts ; in which version (culture) of the website should we display those content items? Should we then only show those content items when we request the frontend website with an InvariantCulture "which should rarely happen¨? Which returns me to the first question I had in mind when I did the Culture settings. Should we set a culture manually from the setup and/or recipes to avoid having an InvariantCulture as an option for default culture?

An other thought I had was to fix the issue on Mac OS X. Opened a ticket about it, but that won't happen soon ; so we need to handle the issue with what we have.

Skrypt commented 5 years ago

As for your concern about the default cultures the issue is not on how the middleware works. I think it's fine. When we reassign a culture manually in the admin it is expected that the culture(s) that will be supported will be those added ; which works as expected. Unless the previous default culture would be kept as supported then there is no issue at all.

The real underlying issue is that the default culture can sometimes fallback to InvariantCulture which makes this even more ambiguous for people. We can't rely on CultureInfo.InstalledUICulture to always return a culture that is not InvariantCulture because of issues in the Mono framework which .NET Core still uses under Mac OS and also linux. So we need to also take account that a default OS culture could be InvariantCulture.

The real question is : should we consider InvariantCulture as a proper culture for site usage or should we use it only for internal purpose? If we consider it as a proper culture then what we are doing is fine else we should be more consequent with the way we are using it in the UI and else. Also, if we support InvariantCulture then it should be an option to select as a supported culture in the culture settings.

My opinion is that this should never happen (no system culture found) :

Capture d’écran, le 2019-05-02 à 00 37 50

But then if we consider InvariantCulture as a proper culture we should be consequent. CultureInfo.GetCultures() returns InvariantCulture in it's list. I just removed the option intentionally. (see first item which returns name = "") which I think in that case is wrong. Vice versa depending on ...

Capture d’écran, le 2019-05-02 à 05 59 41
hishamco commented 5 years ago

@jtkech is the video that you refer to online, I'd like to see it and share my thoughts regarding this ..

jptissot commented 5 years ago

@hishamco https://www.youtube.com/watch?v=UeDTM1ACVeY

jptissot commented 5 years ago

To simplify all this, I think the user should be presented with a culture selection on the setup screen to choose the default culture of the site. After setup is done, we allow the user to add / remove cultures except the default culture. The user could potentially change the default culture and delete the culture that was selected on the setup screen but there would always be a default culture set and the user could never get in a state where they don't have a culture set.

Is there any downsides to forcing the user to choose a default culture like this ? What is the behavior of other CMS (Drupal, Wordpress)?

Skrypt commented 5 years ago

My point is that if we force a specific culture and never let them fallback to the system one then we might miss some edge use cases from people that require it. I mean, we need to handle this case too. We can "patch" the issue by always requiring a culture in setup but the .NET Core framework itself requires that we handle system cultures ... if we want OC to be more like a framework then I think it's best to give the option and to decide if we want to make InvariantCulture a proper culture and make the changes needed accordingly too. @jptissot I agree with what you are suggesting too.

sebastienros commented 5 years ago

Maybe I can change my mind and let it how it was, I think I was confused, and the timezone selector is already doing the job for us to return a culture all the time, which will be the system one if non has been set. So I think we are good. My concern was that the dev would have to do something when the culture is null, but it's never, the dev gets the system one. So we should not have to do any change.

jtkech commented 5 years ago

Tanks all for your comments and i understand the other localization concerns.

But i was just talking about the DefaultCulture and the SupportedCultures internal states and how the aspnet middleware behaves. So, without talking about another way to 1st select the default culture, e.g excluding the invariant one, assuming that we also update the 2 internal default states.

jtkech commented 5 years ago

Hmm, and because the default culture is just used as a fallback by the middleware, so this is not the same behavior if it also belongs to the supported cultures, maybe good, but not sure, that we could change the default culture (maybe through an editable list of available cultures) with a new one without having to add it to the supported cultures.

But i agree maybe not so important, just to be clear on what i was meaning.

hishamco commented 5 years ago

@jptissot I saw the video, the demo was great but I have few concerns and thought that I'd like to share in the content localization PR (which I will probably contribute to it with you guys after finishing Admin RTL)

Regarding the default and supported cultures, it's a crucial topic that effects on localization in general, sadly the issue was closed early!! while its a discussion

It's not the matter of localization content only, but its for localization in general. IMHO there are many issue in the current implementation of OrchardCore:

My suggestion are:

but again from site owner perspective setting default culture is very basic and should be go first, so that why the site settings there

Those are some of my humble suggestion, your feedback is very welcome

Skrypt commented 5 years ago

I just passed 2 hours talking about this with @jtkech 😄

Right now the responsibility for changing the CurrentCulture and CurrentUICulture is left to the ASP.NET Localization middleware which has been implemented as part of the Localization module.

The way that we initialize the middleware is that we change the SupportedCulture only if we assigned a culture in the site settings. Else it will fallback to the Middleware default behavior which is to use the System Culture. Though, like I exposed, in some systems like Mac OS X the returned system culture is InvariantCulture because there is an issue. .NET Core itself can't find the current system culture and it is falling back to InvariantCulture.

@hishamco I agree with some of your comments because it seems logical and I had the same thoughts but we also need to face the fact that we need to handle null or String.Empty as InvariantCulture.

Questions

There's no notion of using DefaultCulture with empty string

Default culture with empty string is equal to CultureInfo.InvariantCulture. Because the InvariantCulture.Name is equal to String.Empty. So, right now we support it and we should continue to do so since we don't enable the Localization middleware on every recipe.

We're heavily used CultureInfo.CurrentUICulture

This is because we left the responsibility to the Localization Middleware for assigning the proper cultures. And that middleware is changing the CultureInfo.CurrentUICulture and CultureInfo.CurrentCulture.

There's no use of SupportedUICultures anywhere, which is doesn't make sense

You need to enable the Localization module and add manually a culture so that it be added to the SupportedCulture and SupportedUICulture options of the Localization middleware.

Default Culture shouldn't be empty, in case of content localization

The DefaultCulture could be set to InvariantCulture so it could be String.Empty so we are obligated to consider InvariantCulture as a proper culture. Which means that we also should be able to add InvariantCulture as a SupportedCulture. And also create content items that are set to InvariantCulture. In that case the LocalizationPart = null or String.Empty which make sense since content items that have been created before enabling the Content Localization module will always have no LocalizationPart set.

CultureInfo.CurrentUICulture should be revised everywhere in the system, because there 're many places CultureInfo.CurrentCulture is preferred for example: calendars

I think we are simply just using CultureInfo.CurrentUICulture because that's the name that seems the most relevant but in most times CultureInfo.CurrentCulture will have the same culture if we don't apply any changes to it and are talking about what the Localization Middleware is doing.

Culture setup or recipes

We decided that having a culture site setting from the setup should not be necessary because the culture settings should simply replicate the behavior that .NET Core Localization has if we want to cover all scenarios possible. There is nothing preventing adding a culture from the setup later on. Though, the culture site settings should properly set the DefaultCulture and also the SupportedCultures when the Localization middleware is enabled. If someone doesn't require enabling it then it will simply fall back to the current system culture and if none is found it will be InvariantCulture.

Current work

Me and @jtkech tested most of things tonight with the Localization Middleware and there is no issue at all when adding InvariantCulture as a SupportedCulture. Though we fixed an issue that sometimes the DefaultCulture was not added to the SupportedCulture and it should always be for the purpose of when using the AcceptLanguageHeaderRequestCultureProvider.

hishamco commented 5 years ago

Right now the responsibility for changing the CurrentCulture and CurrentUICulture is left to the ASP.NET Localization middleware which has been implemented as part of the Localization module.

Yep, but it gives you the choice to specify both SupportedCultures and SupportedUICultures, but if you didn't provide one of those or both together it's their responsibility, still I'm not convinced why we didn't add SupportedUICultures during activated the middleware!!

Default culture with empty string is equal to CultureInfo.InvariantCulture

You're right, but why we didn't use the default culture without adding it to the list in the site settings or use the first supported culture

You need to enable the Localization module and add manually a culture so that it be added to the SupportedCulture.

This is supported cultures not supported UI cultures

I think we are simply just using CultureInfo.CurrentUICulture because that's the name that seems the most relevant but in most times CultureInfo.CurrentCulture will have the same culture if we don't apply any changes to it and are talking about what the Localization Middleware is doing.

Please refer to the @sebastienros scenario in #3455, which is expected in case of using calendars

Last but not least regarding the current work, can I contribute to the current PR, because the UI needs improvement from UX/UI perspective

Thanks too much for your detailed information

Skrypt commented 5 years ago

As for SupportedUICulture it is if you look at this line :

https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.Localization/Startup.cs#L41

Though it will only add cultures to that list if there is cultures found in the site settings which makes sense. The Localization middleware will always add the default culture to the supported cultures if you never change the supported cultures like we do in that code. And if you look in the Localization middleware code you will see that the .AddSupportedCulture() will not add an other culture but clear the list and replace it with what you provide in that method.

You are right I need to fix the list of cultures and add back the CultureInvariant option.

Everyone is welcome to contribute! Just make sure to sync with @jptissot for the UI changes first or simply provide an alternate PR or branch so that we can review it. 😉

The new code I'm working on in that startup.cs

        public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider)
        {
            var siteSettings = serviceProvider.GetService<ISiteService>().GetSiteSettingsAsync().GetAwaiter().GetResult();

            var options = serviceProvider.GetService<IOptions<RequestLocalizationOptions>>().Value;

            // If no specific default culture is defined, use the system language by not calling SetDefaultCulture

            // testing InvariantCulture as default culture
            siteSettings.Culture = String.Empty;

            if (!String.IsNullOrEmpty(siteSettings.Culture))
            {
                options.SetDefaultCulture(siteSettings.Culture);
            }

            if (siteSettings.SupportedCultures.Length > 0)
            {
                //We add here the defaultCulture as a supported culture including InvariantCulture if it is
                var supportedCulture = siteSettings.SupportedCultures.ToList();
                supportedCulture.Add(siteSettings.Culture);
                supportedCulture = supportedCulture.Distinct().ToList();

                options
                    .AddSupportedCultures(supportedCulture.ToArray())
                    .AddSupportedUICultures(supportedCulture.ToArray());
            }

            app.UseRequestLocalization(options);
        }
Skrypt commented 5 years ago

Time to go sleep cya tomorrow 😉

hishamco commented 5 years ago

Again I dislike this line

.AddSupportedUICultures(supportedCulture.ToArray());

List should be added at the site settings

Time to go sleep cya tomorrow 😉

Sorry, in Yemen it morning, seem our timezone are different (localize related 😄). Have a good night .. see ya

cnoelker commented 5 years ago

Hi there, I watched the video linked above and got a glimpse of the planned functionality concerning Content Localisation. In the roadmap , it's written that Content Localisation is in the Backlog for the RC. I think that Content Localisation is essential for a CMS. Will this be included in the RC? Or when can we expect this?

hishamco commented 5 years ago

Will this be included in the RC?

Of course according my understanding

Skrypt commented 5 years ago

@hishamco What we are doing is looking at the defaultCulture and if it's not included in the site settings supported cultures then we add it and doing a distinct to prevent having the same culture added twice. There are scenarios where it can happen that the default culture will not be added to the supported cultures so we add it now since we can't think of a scenario where it would not be required.

The default culture is the fallback culture if no supported culture are resolved by the Localization middleware. SupportedCultures are cultures accepted by the Localization Providers. It makes sense to me that the default culture always be in the supported cultures.

Though nothing prevents you from adding the default culture manually to the supported cultures too.

Let me know if I missed something or try to explain why you don't like

.AddSupportedUICultures(supportedCulture.ToArray());

sebastienros commented 5 years ago

@hishamco I read what you said, but you never give an example of the actual issues. I think there isn't any issue with what we have done, so please change my mind ;)

hishamco commented 5 years ago

then we add it and doing a distinct to prevent having the same culture added twice

It isn't the case .. adding it makes some issues for me:

Breaking the localization rules, because the Default Culture is using for fallback, but how can fallback to culture that doesn't support, except if the user doesn't provide it, so it will fallback to en-GB. If you told me "as I said we add it .." this is another issue. Don't enforce anyone to add unsupported culture that doesn't specified from his/her side and leave the default behavior

The question is why you wanna add it?!!

It makes sense to me that the default culture always be in the supported cultures

This is confusing a lot of people, of course you will think it's supported, but behind the scene it's the fallback culture. Isn't it? if all we think this way .. i'm assuming ASP.NET Core team will add this too to the supported cultures, but it doesn't make sense at all

What I suggest that we use the normal and default behavior, the user should be able to specify the default and supported cultures, if he/she didn't the ASP.NET will fallback to en-GB. Also there's no to add system culture to the default culture list, may be a warning message is suitable

For content localization, we should add the content with the default culture, if someone said what if the user forgot to set the default culture. My answer is simple this is his fault!! but we can provide a mechanism to change the translation if the default culture changed

hishamco commented 5 years ago

I forgot one thing we need to add the Supported UI Cultures too

.AddSupportedUICultures(supportedCulture.ToArray());

This is correct if the user doesn't specify the supported UI cultures, so we don't wanna put the users into a corner without give the ability to specify them

Skrypt commented 5 years ago

I think the last changes reflects how you think maybe I did not explain appropriately. The default behavior of the Localization middleware is that it uses the system OS culture and adds that same culture to the supported cultures. So by default the behavior is to always have the default culture as a supported culture. The confusion we had is that when added supported cultures and we were still using the system culture as default ; that system culture could have been different than the supported cultures a user has added. So we fixed the issue by always adding the system culture to the supported cultures. The issue here is that we added the system culture option afterward and we never thought about the fact that it could be different than the actual supported cultures.

Most important part is that now we also support Invariant Culture as a proper culture that can be added as a supported culture if needed. You can try pulling latest changes on dev and test it. Tell me if you find an issue. You can test InvariantCulture by using ?culture= as url param.

hishamco commented 5 years ago

Checking the content localization branch?

Skrypt commented 5 years ago

dev branch. It's not merged yet to the content localization branch. We just merged the changes tonight.

hishamco commented 5 years ago

Ok got it .. thanks

Right now i'm checking something else in localization which is probably a bug that no one knows, let me finish what I have and test the dev

thanks again

hishamco commented 5 years ago

Just two points I have

  1. Still there's a need to specify the Supported UI culture from settings, the scenario I have I add ar culture, it works fine as CurrentUICulture but not as CurrentCulture, I got a Hijri dates, I'd like to be able to add ar-YE as supported culture and ar or ar-SA as supported UI cultures

  2. Invariant culture works fine, but what's the need of someone use ?culture= while it will fallback by default to en-GB?!!

Skrypt commented 5 years ago
  1. I can't say, maybe a special use case scenario. I need to understand more.

  2. ?culture= doesn't fallback to default culture if you add InvariantCulture as a supported culture. It's usefull if you have created content items that are invariant and want to still see those content items on your website.

hishamco commented 5 years ago

I can't say, maybe a special use case scenario. I need to understand more.

Let me try real show case that probably useful for calendars too and come back to you

?culture= doesn't fallback to default culture

If you look closely to this https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L155-L167 you see that InvariantCulture is supported and fallback to the parent if FallbackToParentCultures is true

It's usefull if you have created content items that are invariant and want to still see those content items on your website

The aim of the default culture is allow the user to set the default culture for his/her website, so I didn't see any value to make it empty string, so when he/she want to localize content a message should show up if there's default culture set and no supported cultures are provided too, otherwise it should fallback to the default en-GB

hishamco commented 5 years ago

I can't say, maybe a special use case scenario. I need to understand more.

Let me try real show case that probably useful for calendars too and come back to you

?culture= doesn't fallback to default culture

If you look closely to this https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L155-L167 you see that InvariantCulture is supported and fallback to the parent if FallbackToParentCultures is true

It's usefull if you have created content items that are invariant and want to still see those content items on your website

The aim of the default culture is allow the user to set the default culture for his/her website, so I didn't see any value to make it empty string, so when he/she want to localize content a message should show up if there's default culture set and no supported cultures are provided too, otherwise it should fallback to the default en-GB

Skrypt commented 5 years ago

I got a Hijri dates

For first point I assume that you just want to have the Hijri calendar while using ar-YE and you want to have ar-SA as a supported culture to be able to display your dates with the Hijri calendar. I think that if that's what you mean then the PR for calendars should add the ability to pick any calendar system like in 01 to allow this. The SupportedCultures and SupportedUICultures set from the site settings here we should assume that they are Global to the tenant but there's nothing preventing us to change the current Thread culture for achieving these kinds of changes but it will be per need. Right now I'm not sure 100% on how we we're doing this in 01 ; I need to check and make sure this is the proper way to do it. But it should not be related with the SupportedCultures that we previously set. These cultures are set for the Localization Middleware which has the only responsibility to allow to use specified cultures for the UI translation per LocalizationProvider. It should have no incidence in allowing to use a Hijri calendar in a specific place in the UI.

so I didn't see any value to make it empty string

The InvariantCulture has been added to the list of cultures on the left side of that form. Of course you can't add it with the textbox since it doesn't allow a String.Empty.

The aim of the default culture is allow the user to set the default culture for his/her website, so I didn't see any value to make it empty string, so when he/she want to localize content a message should show up if there's default culture set and no supported cultures are provided too, otherwise it should fallback to the default en-GB

First, with the latest fix it is not possible that a default culture not be a supported culture.

Secondly, for content items that have been created before enabling the content localization module. Those content items will have no LocalizationPart set. So they will all be set to InvariantCulture naturally. If you want to be able to see those content items you will either need to disable the Localization module to get back in the previous state, add InvariantCulture as a supported culture or set your default culture to InvariantCulture... because that's what you had previously! You we're creating content items as InvariantCulture ...

Though, you could still set your default culture to en-US and have content items set to InvariantCulture and want to still see them on the frontend website by adding InvariantCulture to your supported cultures and see them with ?culture=. Of course, InvariantCulture, generally is not a culture that you should want to keep your content items as.

This is why it requires a human interaction on those content items so that they be migrated to a default culture either by proposing the current defaut culture when editing them the next time (should be a true/false setting because not everyone will need this) or by batch updating them with an upgrade process that will be eventually added on the admin UI.

Skrypt commented 5 years ago

Elaborating on "proposing the current default culture" when editing a content item that is set as InvariantCulture. The reason why it should be a setting is that it could be wrong to propose it since some people could actually want to keep their content items as InvariantCulture. The changes we made this week was about making InvariantCulture a proper culture so it should not treated differently than any other culture for that matter. We never propose for example the default culture (ar-SA) for a content item that is set to (en-US)... so it makes sense to me to do the same with InvariantCulture (unless specified**).