Skip to content

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 20, 2021

Fixes #57310

Backport of #57754 to release/6.0

/cc @tarekgh

Customer Impact

This change is fixing a regression from .NET 5.0. The problem happens when trying to create a CultureInfo object using culture names that Windows don't allow. If any app take dependency on such behavior will be broken. The issue is already reported by external community member. Here are more details about the change:

This regression occurred during the code refactoring work done to optimize the globalization data size and WASM support.
Although this fix is addressing the regression, it fixes another issue I have discovered during the investigation. Here is what the change is doing:

  • In the file CultureData.Windows.cs, the condition ShouldUseUserOverrideNlsData is wrong to use. The reason is this part of the code is not concerned about the user overrides. Even the user overrides flag is not initialized yet at this stage.
  • In the same file, I added the call to Windows API GetLocaleInfoEx that help validate the culture name. That is what we have done in .NET 5.0 too.
  • In the file IcuLocaleData.cs, there was a bug that caused not finding the proper culture name mapping to our data because the culture name containing characters like underscore character _.
  • Fixing and adding tests.

Testing

I have run manual testing and passing all our regression tests. Also, I have added more tests to cover the gap of the scenario of this issue.

Risk

Is not high as the fix is scoped and shouldn't affect other culture scenarios.

@ghost
Copy link

ghost commented Aug 20, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #57754 to release/6.0

/cc @tarekgh

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Aug 20, 2021

CC @ericeil

@tarekgh
Copy link
Member

tarekgh commented Aug 20, 2021

@ericstj @jeffhandley this is a backport for a fix we need to have before the release. Could you please approve it?

@tarekgh tarekgh added this to the 6.0.0 milestone Aug 20, 2021
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for RC2. Regression from previous release.

(I'll leave the code/PR approval to someone else though)

@jeffhandley jeffhandley added the Servicing-approved Approved for servicing release label Aug 20, 2021
@jeffhandley
Copy link
Member

@stephentoub or @jeffschwMSFT -- could one of you merge this in @danmoseley's absence please?

@ericstj ericstj merged commit eea9ec5 into release/6.0 Aug 20, 2021
@akoeplinger akoeplinger deleted the backport/pr-57754-to-release/6.0 branch August 28, 2021 22:04
@ghost ghost locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants