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

Add option to use system culture in RequestLocalizationOptions #44282

Merged
merged 6 commits into from
Oct 24, 2022
Merged

Add option to use system culture in RequestLocalizationOptions #44282

merged 6 commits into from
Oct 24, 2022

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Sep 30, 2022

Fixes #44286

We faced an issue in OrchardCore, the current culture based on the user-selected culture all the time because CultureInfo called the default constructor with `useUserOverride

It would be nice if we can fix this behavior by adding a simple parameter to RequestLocalizationOptions that allow us to use whether to select user-selected culture or default system culture

/cc @DamianEdwards

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Sep 30, 2022
@ghost
Copy link

ghost commented Sep 30, 2022

Thanks for your PR, @hishamco. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@hishamco
Copy link
Member Author

I just set the default value to true to avoid behavior breaking changes

@DamianEdwards
Copy link
Member

Thanks @hishamco. Can you log an issue for this please?

@hishamco hishamco requested a review from a team as a code owner September 30, 2022 17:26
@hishamco
Copy link
Member Author

Can I refer to the Orchard Core issue because it contains much details, otherwise I will file an issue here

@hishamco
Copy link
Member Author

Seems it has been years since I contributed to localization after the repo becomes HUGE ;) Does the PublicAPI.Shipped auto generated or shall I need to modify it?

@DamianEdwards
Copy link
Member

There's a code fixer for it in Visual Studio so you should just be able to Ctrl+. to fix it.

@hishamco
Copy link
Member Author

hishamco commented Oct 1, 2022

Is there another way to modify that file, I'm unable to do it from VS code the project is HUGE, my laptop is hanging ;)

@DamianEdwards
Copy link
Member

Docs don't seem to indicate another way: https://github.com/dotnet/aspnetcore/blob/main/docs/APIBaselines.md

You should only need to open a filtered solution (or even just a project) that contains the affected APIs and run it in the context of that though.

@hishamco
Copy link
Member Author

hishamco commented Oct 3, 2022

I already opened the middleware solution, I will have a try

@hishamco
Copy link
Member Author

Seems it doesn't work with I'm expecting that I need to install preview bits of .NET Core 8 version

@DamianEdwards
Copy link
Member

@hishamco the repo relies on bits installed locally to the repo. You should only need to run restore.cmd in repo root to get required bits.

@hishamco
Copy link
Member Author

I got the following exception during the restore process

D:\Git\Repositories\aspnetcore\src\Servers\IIS\build\Build.Common.Settings(12,3): error MSB4019: The imported project "
D:\Git\Repositories\aspnetcore.tools\msbuild\17.1.0\tools\MSBuild\Microsoft\VC\v170\Microsoft.Cpp.Default.props" was n
ot found. Confirm that the expression in the Import declaration "D:\Git\Repositories\aspnetcore.tools\msbuild\17.1.0\t
ools\MSBuild\Microsoft\VC\v170\Microsoft.Cpp.Default.props" is correct, and that the file exists on disk. [D:\Git\Repo
sitories\aspnetcore\src\Servers\IIS\AspNetCoreModuleV2\AspNetCore\AspNetCore.vcxproj]

Do I need a C++ packages? If this will take long, I can modify the APIs, after @halter73 reply on my comment in the original issue, then if some can modify the PublishedAPI file it will be helpful

Hope if we can merge this before shipping v7.0.0

@halter73
Copy link
Member

Hope if we can merge this before shipping v7.0.0

Unfortunately, it's far too late to merge any non-critical changes in .NET 7. This will have to wait until .NET 8.

I suspect you can build Localization.slnf without a C++ SDK, but the easiest thing to do is probably to follow our BuildFromSource doc and run ./eng/scripts/InstallVisualStudio.ps1. I think this should install all the necessary SDKs for .\restore.ps1 to complete successfully.

@BrennanConroy
Copy link
Member

./build.cmd -nobuildnative should also work

@hishamco hishamco requested review from halter73 and DamianEdwards and removed request for halter73 and DamianEdwards October 20, 2022 21:13
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Thanks for updating the API!

Co-authored-by: Stephen Halter <[email protected]>
@hishamco
Copy link
Member Author

BTW @halter73 could you please doble check the APIs file coz I faced issue with VS, but I struggled to modify it

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Thanks again!

@halter73 halter73 changed the title Ability to use user-selected or system culture in RequestLocalizationOptions Add option to use system culture in RequestLocalizationOptions Oct 24, 2022
@halter73 halter73 merged commit 59cb76f into dotnet:main Oct 24, 2022
@ghost ghost added this to the 8.0-preview1 milestone Oct 24, 2022
@hishamco hishamco deleted the request-localization-options branch October 24, 2022 17:14
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing the date / time format reflect to the CurrentCulture all the time
6 participants