Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0-staging] Fix creating cultures with extensions in the name #87152

Merged
merged 7 commits into from
Jun 10, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jun 5, 2023

Backport of #87114 to release/6.0-staging

/cc @tarekgh

Customer Impact

#86441

When a culture is created on non-Windows operating systems and includes an extension part in its name (e.g., xx-u-XX), ICU (International Components for Unicode) normalizes the name to something like xx@XX=Yes. However, if this normalized culture is set as the default UI culture using CultureInfo.CurrentUICulture, it can lead to infinite recursion during resource lookup. This occurs because the resource manager rejects the culture name, resulting in a fast failure and process termination. It's important to note that resource lookup takes place when throwing any exception within the process.

In contrast, on Windows operating systems, an exception is thrown early during the culture creation process.

Moe details

The .NET supports culture names that closely follow the BCP 47 format specifications, which are in the format of language-script-region-extensions-collation. However, there are slight differences between the two. For example, the culture name en-US conforms to the standards and is supported by .NET. Things become interesting when we use the extension parts in the culture name. In BCP 47, extensions are defined by starting them with either -u- or -t-. However, .NET only supports one type of extension, which is collation, and it uses a different format for it. For instance, the standard format de-DE-u-co-phonebk defines the de-DE culture with the phone book collation behavior. In .NET, the culture name for the same collation would be de-DE_phonebk. On the other hand, ICU uses the name format de_DE@collation=phonebk. As a result, BCP 47 uses the -u-co- prefix to define the collation, .NET uses _ as the collation name prefix, and ICU uses @collation= as the prefix.

Currently, when we interact with ICU, we transform the .NET culture name to a format that ICU understands, and then .NET normalizes the final culture name back to the .NET format from the name returned by ICU. However, we currently only handle the collation extension and do not handle any other extensions, resulting in the returned name not being normalized. For example, if we use a name like xx-u-XX, ICU normalizes it to xx@XX=Yes, but we do not normalize this name to a format that works with .NET. On Windows, when we interoperate with the Windows OS using such a culture name to retrieve the default user settings, the name xx@XX=Yes will be rejected and an exception will be thrown. On Linux/MacOS, we continue to create a culture with such a name, but eventually, an exception will be thrown when using this culture with the resource manager, as it is an invalid name for resources.

To fix this issue, we need to normalize the culture names, taking into account the extension parts -u- and -t-. For the collation extension, we will continue to normalize it using _ while for other extensions, we will keep the original form. For example, in the case of xx-u-XX, we will keep the name as it is.

Testing

Running all regression tests successfully and added more tests covering the cases we are fixing.

Risk

Medium risk, we are touching the culture name normalization part which is scoped and mostly the change to handle the extension parts in the culture name.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Jun 5, 2023

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #87114 to release/7.0-staging

/cc @tarekgh

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

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

area-System.Globalization

Milestone: -

@tarekgh tarekgh added Servicing-consider Issue for next servicing release review Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 6, 2023
@tarekgh
Copy link
Member

tarekgh commented Jun 9, 2023

This is approved offline by email.

@tarekgh tarekgh merged commit 4d2768a into release/7.0-staging Jun 10, 2023
@tarekgh tarekgh deleted the backport/pr-87114-to-release/7.0-staging branch June 10, 2023 18:47
@ghost ghost locked as resolved and limited conversation to collaborators Jul 11, 2023
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.

2 participants