Skip to content
This repository has been archived by the owner on Nov 29, 2018. It is now read-only.

RequestCultureProvider sets an unsupported culture when the request is not supported and the default is not defined or not supported. #70

Closed
Bartmax opened this issue Sep 10, 2015 · 7 comments

Comments

@Bartmax
Copy link

Bartmax commented Sep 10, 2015

Helping someone with an issue on Jabbr about localization, I found this strange (at least for me) behavior.

RequestLocalizationOptions localizationOptions = new RequestLocalizationOptions()
{
  SupportedCultures = new List<CultureInfo>
  {
    new CultureInfo("nl-NL")
  },
  SupportedUICultures = new List<CultureInfo>
  {
    new CultureInfo("nl-NL")
  }
  //, DefaultRequestCulture = new RequestCulture(new CultureInfo("nl-NL"), new CultureInfo("nl-NL"))
};
app.UseRequestLocalization(localizationOptions);

------------------------
services.AddLocalization();
services.AddMvc().AddViewLocalization();

note the commented line

With this cshtml (in layout or index):

<h1>UICulture: @System.Globalization.CultureInfo.CurrentUICulture.Name</h1> // outputs: en-US 
<h1>Culture: @System.Globalization.CultureInfo.CurrentCulture.Name</h1> // outputs: es-UY

According to the documentation here: https://github.com/aspnet/Localization/blob/dev/src/Microsoft.AspNet.Localization/RequestLocalizationOptions.cs#L38-L54

This is not expected behavior. Those cultures are not supposed to be supported by the application.

Uncommenting the commented line (DefaultRequestCulture) I get the expected output:

DefaultRequestCulture = new RequestCulture(new CultureInfo("nl-NL"), new CultureInfo("nl-NL"))
UICulture: nl-NL
Culture: nl-NL

Looking at source code, I found that a 'premature' fallback to default culture.
https://github.com/aspnet/Localization/blob/dev/src/Microsoft.AspNet.Localization/RequestCultureProvider.cs#L42-L50 without checking if the Default is supported.

I think the workflow should be more like (not real code)

var culture = Options.SupportedCultures == null || !Options.SupportedCultures.Any() || Options.SupportedCultures.Contains(result.Culture)
    ? result.Culture
    : Options.SupportedCultures.Contains(Options.DefaultRequestCulture.Culture)
    ? Options.DefaultRequestCulture.Culture
    : Options.SupportedCultures.First();

var uiCulture =  Options.SupportedUICultures == null || !Options.SupportedUICultures.Any() || Options.SupportedUICultures.Contains(result.UICulture)
    ? result.UICulture
    : Options.SupportedUICultures.Contains(Options.DefaultRequestCulture.UICulture)
    ? Options.DefaultRequestCulture.UICulture
    : Options.SupportedUICultures.First();

 result = new RequestCulture(culture, uiCulture);

I'm assuming Empty List = null here.

@Bartmax Bartmax changed the title I fo I found a weird (bug?) issue with the RequestLocalization Middleware. Sep 10, 2015
@Bartmax Bartmax changed the title I found a weird (bug?) issue with the RequestLocalization Middleware. RequestLocalization may set an unsupported culture when the default is not defined or not supported. Sep 10, 2015
@Bartmax Bartmax changed the title RequestLocalization may set an unsupported culture when the default is not defined or not supported. RequestLocalization sets an unsupported culture when the request is not supported and the default is not defined or not supported. Sep 10, 2015
@Bartmax Bartmax changed the title RequestLocalization sets an unsupported culture when the request is not supported and the default is not defined or not supported. RequestCultureProvider sets an unsupported culture when the request is not supported and the default is not defined or not supported. Sep 10, 2015
@Bartmax
Copy link
Author

Bartmax commented Sep 10, 2015

This is related: #38

@hishamco
Copy link
Contributor

It's a bug :(

@hishamco
Copy link
Contributor

@Bartmax can you try this

if (requestCulture == null || Options == null)
{
    return null;
}

var result = requestCulture;

if (Options.SupportedCultures?.Contains(result.Culture) == false)
{
    if (Options.SupportedCultures.Contains(Options.DefaultRequestCulture.Culture))
    {
         return new RequestCulture(Options.DefaultRequestCulture.Culture);
    }
    else
    {
        return new RequestCulture(Options.SupportedCultures?.Count > 0 ?
            Options.SupportedCultures[0] : Options.DefaultRequestCulture.Culture);
    }
}

if (Options.SupportedUICultures?.Contains(result.UICulture) == false)
{
    if (Options.SupportedUICultures.Contains(Options.DefaultRequestCulture.UICulture))
    {
         return new RequestCulture(Options.DefaultRequestCulture.UICulture);
    }
    else
    {
         return new RequestCulture(Options.SupportedUICultures?.Count > 0 ?
             Options.SupportedUICultures[0] : Options.DefaultRequestCulture.UICulture);
    }
}
return result;

@Bartmax
Copy link
Author

Bartmax commented Sep 14, 2015

Looks good, not sure what will works as expected if we have:

  SupportedCultures = new List<CultureInfo>
  {
    new CultureInfo("es-UY")
  },
  SupportedUICultures = new List<CultureInfo>
  {
    new CultureInfo("nl-NL")
  }

I don't think it will be a common scenario but...

because we return earlier on Culture without even checking at UICulture. I guess best way to tell is to have some unit test.

@hishamco
Copy link
Contributor

You are right @Bartmax, let me revise the code snippet and come back to you

@hishamco
Copy link
Contributor

@kirthik can you look into this if you don't mind, seems it's a critical issue

@kirthik
Copy link
Contributor

kirthik commented Oct 18, 2015

With this design change - #111, passing in default culture will be required when adding localization. We will scan request's culture through supported culture list and fall back to default if exact match is not found. So scenario where default culture is not defined or supported does not arise. Hence closing this issue.

@kirthik kirthik closed this as completed Oct 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants