search

turquoiseowl / i18n

Smart internationalization for ASP.NET
Other
556 stars 155 forks source link

Misleading comment on SetPrincipalAppLanguageForRequest -- updateThreadCulture boolean appears to have no effect #384

Open jnpwly opened 5 years ago

jnpwly commented 5 years ago

In the HttpContextExtensions.cs file, there is a comment that by setting the updateThreadCulture boolean, the user can change whether the CurrentCulture/CurrentUICulture settings can be changed:

https://github.com/turquoiseowl/i18n/blob/ce7bdc9d8a8b92022c42417edeff4fb9ce8d3170/src/i18n/Helpers/HttpContextExtensions.cs#L135

However, this boolean ultimately has no impact; it is not actually used in the method.

It's not clear to me what the intention with this boolean was. Was it meant to allow the end user to ultimately change just the Thread.CurrentThread.CurrentCulture and not the Thread.CurrentThread.CurrentUICulture? Or, was there something else in mind?

I am more than happy to submit a PR that changes the comment, to indicate that it doesn't do anything. That's the easiest thing to do. Probably can't just remove the boolean, as that might break existing usages of the i18n library. Alternatively, I could work on developing a PR that attempts to only update the Thread.CurrentThread.CurrentCulture and not the Thread.CurrentThread.CurrentUICulture. But, seeing as the library user can implement their own SetPrincipalAppLanguageForRequestHandler, then maybe that is the method that end users should use to avoid changing the UI Culture??

turquoiseowl commented 5 years ago

I suspect the SetPrincipalAppLanguageForRequest method has evolved somewhat.

It's original purpose was two-fold:

  1. To explicitly set the PAL that i18n is to use for generating the response to the current request, overriding any PAL determined from eraly request processing.
  2. To optionally invoke the following: 
    Thread.CurrentThread.CurrentCulture = Thread.CurrentThread.CurrentUICulture = langtag.GetCultureInfo();

Looks like 2. was then moved to the handler model with a default handler installed to invoke that default logic -- but updateThreadCulture wasn't adpated at the same time.

I suspect most clients will probably NOT install their own handlers and just expect the default handler here to update the cultures if updateThreadCulture == true.

Maybe the best way forward is:

  1. Remove the updateThreadCulture argument from existing methods. That will break the API but not behaviour that is according to spec.
  2. Add new method: SetPrincipalAppLanguageForRequestWithoutCallingHandlers that does what it says.