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

[Discussion] ResourceManagerWithCultureStringLocalizer class and WithCulture interface member marked Obsolete and will be removed #7756

Closed
ryanbrandenburg opened this issue Feb 20, 2019 · 35 comments
Milestone

Comments

@ryanbrandenburg
Copy link
Contributor

ryanbrandenburg commented Feb 20, 2019

The ResourceManagerWithCultureStringLocalizer class and WithCulture interface member are often sources of confusion for users of Localization, especially if they want to create their own IStringLocalizer implementation. These items give the user the impression that we expect an IStringLocalizer instance to be "per-language, per-resource", when actually they should only be "per-resource", with the language searched for determined by the CultureInfo.CurrentUICulture at execution time. To remove this source of confusion and to avoid APIs which we don't want users to use we will be obsoleting these in 3.0.0-preview3, and they will be removed in a future release (4.0 or above).

For context, see here.


This is the discussion issue for aspnet/Announcements#346.

@hishamco
Copy link
Member

and they will be removed in a future release (4.0 or above).

Why we don't remove them in 3.0 while it's a major release?!!!

@ryanbrandenburg
Copy link
Contributor Author

@hishamco our process is generally to Obsolete in a major release then remove in the next major after that.

@hishamco
Copy link
Member

Got it .. thanks

@joakimriedel
Copy link

We're using WithCulture to specify the culture used for the localizer used in an e-mail template, when sending e-mails triggered by certain events. If I understand you correctly @ryanbrandenburg, this usage will be Obsolete.

        protected IStringLocalizer GetLocalizerForCulture<T>(string culture)
        {
            var type = typeof(T);
            var localizer = _localizerFactory.Create(type);

            var cultureInfo = String.IsNullOrEmpty(culture) ? CultureInfo.CurrentCulture : new CultureInfo(culture);
            return localizer.WithCulture(cultureInfo);
        }

        public EmailMessage GenerateDailyDigestMessage(DailyDigest digest)
        {
            var localizer = GetLocalizerForCulture<DailyDigestTemplate>(digest.User.Culture);
            var template = new DailyDigestTemplate(localizer, digest.User.FirstName);

            return new EmailMessage(template, digest.User);
        }

In this example, GenerateDailyDigestMessage will be called multiple times for different users, each having (potentially) a different culture.

Please advise what would be a suggested implementation in 3.0.

Changing the thread's locale multiple times in a loop seems a bit wrong to me.

@ryanbrandenburg
Copy link
Contributor Author

It sounds like you already have an idea, but our proximal suggestion for that scenario is something like:

        private CultureInfo _originalCulture;

        protected void SetCurrentCulture(string culture)
        {
            _originalCulture = CultureInfo.CurrentCulture;
            var cultureInfo = String.IsNullOrEmpty(culture) ? CultureInfo.CurrentCulture : new CultureInfo(culture);
            CultureInfo.CurrentCulture = cultureInfo;
            CultureInfo.CurrentUICulture = cultureInfo;
        }

        protected void ResetCurrentCulture()
        {
            CultureInfo.CurrentCulture = _originalCulture;
            CultureInfo.CurrentUICulture = _originalCulture;
        }

        public EmailMessage GenerateDailyDigestMessage(DailyDigest digest)
        {
            try
            {
                SetCurrentCulture(digest.User.Culture);
                var localizer = _localizerFactory.Create(type);
                var template = new DailyDigestTemplate(localizer, digest.User.FirstName);
                return new EmailMessage(template, digest.User);
            }
            finally
            {
                ResetCurrentCulture();
            }
        }

That code hasn't been tested or anything, but it should be relatively close.

My more structural suggestion would be that the CurrentCulture and CurrentUICulture be set off of User.Culture when the users context is loaded (and reset when it's disposed). That way you don't have to repeat this pattern everywhere you localize in your system. If you're working inside of an MVC context that's basically what happens behind the scenes anyway, but given the implied context here I assume this is more of a "CRON job" kind of task.

@joakimriedel
Copy link

Thanks @ryanbrandenburg.

As you've guessed, this is part of a background task (IHostedService) where there are 20+ templates for e-mail and text messaging sent to thousands of users all on different locales.

I was mostly concerned about performance, concurrency or other issues that could arise from rapidly changing the current culture on the thread. Since that does not seem to be an issue, I will go with your suggestion.

Took the liberty to redesign it for the using pattern, which would pair nicely with the new C# 8.0 using directive. https://github.com/jcemoller/culture-switcher/blob/master/src/CultureSwitcher.cs

Thanks for the early notification, gives us time to adapt our systems in time for 3.0.

@ryanbrandenburg
Copy link
Contributor Author

Looks great! Yes, this is a great candidate for a using/IDisposable structure. Glad we could help!

@martincostello
Copy link
Member

Similar to the above, we were using WithCulture() to create a localizer for specific cultures for usage in tests. It was neat and clean.

It could also be used to pull a single string for a specific language out for inclusion in a page rendered in another language, for example in a language picker where a string in the language of what you want to switch the UI to is needed.

Now we need to wrap all such tests in try-catch blocks to change the culture of the current thread and then swap it back again, which is much less tidy.

@martincostello
Copy link
Member

We also have a unit test that we use to validate that translations have been applied to our resource files. This is now much less trivial to achieve.

foreach (string language in SupportedOtherLanguages)
{
    var culture = new CultureInfo(language);
    var specificLocalizer = localizer.WithCulture(language);

    foreach (string resource in resourceNames)
    {
        string otherText = specificLocalizer[resource].Value;
        string englishText = baseLocalizer[resource].Value;

        otherText.ShouldNotBe(
            englishText,
            $"The value of resource '{resource}' is the same in English and {culture.EnglishName}. Does it exist in the appropriate .resx file?");
    }
}

@ryanbrandenburg
Copy link
Contributor Author

RE try-catch: Something similar to the IDisposable pattern @jcemoller used cleans that up a lot. With regards to loading multiple languages depending on how many you needed you could either cache the result before changing languages or write a ResourceManagerStringLocalizer with behavior similar to https://github.com/aspnet/Extensions/blob/master/src/Localization/Localization/src/ResourceManagerWithCultureStringLocalizer.cs.

@dradovic
Copy link

dradovic commented Mar 22, 2019

From an API design point of view, I would expect that an IStringLocalizerFactory is able to create an IStringLocalizer which is bound to the correct CultureInfo.

Why not add a parameter there? Something like:

    IStringLocalizer Create(Type resourceSource, CultureInfo culture);

The default for culture then of course should be CurrentUICulture. (Needless to say that such a defaulting can easily be achieved by having a IStringLocalizerFactory extension method that only has a resourceSource parameter).

@hishamco
Copy link
Member

hishamco commented Jun 9, 2019

@ryanbrandenburg I think we need to close this while it's already done

@ryanbrandenburg
Copy link
Contributor Author

There is a bot that closes Discussion issues after a certain period, there's no need to close it outside of that framework.

@anna-git
Copy link

To generate documents, emails and for so many other usecases, we need to be able to specify the culture in the localizers. The idea of reading the culture statically within the thread is non testable, not a solution for these so common use cases, and feels somewhat dirty. Why not keep the nice mechanism that the method GetStringSafely() provided within ResourceManagerWithCultureStringLocalizer, eventually calling ResourceManager.GetString(name, culture) which seems totally made for the purpose. I understand the false "per-language, per-resource" impression with WithCulture function returning a new instance, then why not make it a void method, which would just change the _culture field value?
If I understood correctly, the solution for those common usecases is to copy the obsolete class and to keep it locally in the project. It seems like a regression, something that is gonna be missing again from the framework.

@skuami
Copy link

skuami commented Dec 19, 2019

Like many others, I also need a StringLocalizer for email generation in a specific language.

I totaly agree with @anna-git - the suggested solutions with switching the current culture is kind of dirty. It also needs way more code to write. But on the other hand I do understand the reason to remove this method from the IStringLocalizer interface.

My suggestion for a cleaner solution would be to make an overload of the Create method with a CultureInfo parameter to the IStringLocalizerFactory.

This is my old implementation:

public StepResult(IStringLocalizer<SharedResources> localizer)
{
    _localizer = localizer;
    _localizerEnglish = localizer.WithCulture(new CultureInfo("en"));
}

And this would be the new implementation:

public StepResult(IStringLocalizer<SharedResources> localizer, IStringLocalizerFactory factory)
{
    _localizer = localizer;
    _localizerEnglish = factory.Create(typeof(SharedResources), new CultureInfo("en"));
}

@dradovic
Copy link

Exactly what I wrote too.

@FilipVanBouwel
Copy link

I'm also totally not a fan of having this method removed. Changing CurrentUICulture and reverting it every time is very annoying and makes it pretty much not unit-testable.

@skuami
Copy link

skuami commented Jan 6, 2020

@dradovic

Exactly what I wrote too.

Sorry for the double post.
You're right. I've just read your code and there it is not obvious you meant the IStringLocalizerFactory but you describe it in the text very well.

@joshfriend
Copy link

joshfriend commented Mar 11, 2020

The process of obsoleting this API somehow broke the whole localization system for SharedResoruce dotnet/sdk#4033

Is there an alternative I can use?

Also, I really strongly dislike the suggestion of modifying Culture.CurrentUICulture. It's a very messy way to work with localizations and it is extremely prone to someone forgetting to set the culture back to the old value. It also does not work because of the above issue. IStringLocalizer.WithCulture made total sense to me and was exactly what I wanted. I need to get strings for several languages at once and getting a "scoped" localizer was perfect.

Basically I am doing exactly what was described earlier in this comment: #7756 (comment)

@Cybrosys
Copy link

Cybrosys commented Mar 24, 2020

Hello,

I just migrated one of our projects over to .NET Core 3.1 and was working through the warnings. I then noticed that IStringLocalizer.WithCulture had been marked as Obsolete; with vague references to CurrentCulture and CurrentUICulture.

Are we expected to change the current thread's culture at runtime when requesting a specific localization?

We are in several scenarios reading the default localization as well as the user's requested localization. One of those scenarios are when we're returning an error. The error structure contains the neutral error message (In English) and the localized error message (In the user's requested language).

I see above that others also retrieve multiple localizations at the same time.

So how would you replace code similar to this:

return BadRequest(new ErrorDto
{
    Error = _localizer.WithCulture("en")["The error message"],
    LocalizedError = _localizer["The error message"],
});

What I think I will do, for now, is create a decorator for IStringLocalizer which is returned from an extension method on IStringLocalizer. That decorator will switch the thread's current culture back and forth.

@maxbeaudoin
Copy link

Temporarily forcing the thread's culture is a gross hack at best. Always been.

@iozcelik
Copy link

iozcelik commented Jun 2, 2020

I think ".WithCulture" is usefull. Changing culture everytime is not the good solution and it is untestable.

@the-avid-engineer
Copy link

the-avid-engineer commented Jul 16, 2020

WithCulture should remain as a way to override the thread's culture. Maybe a better name, such as WithCultureOverride? I like hishamco's original idea of making it an extension method.

@dradovic
Copy link

I still fail to see why it's not the responsibility of the factory to create a "immutable" localizer with the correct culture instead of working with a side-effect.

Can please one of the proponents of WithCulture explain?

@the-avid-engineer
Copy link

I don’t have a strong opinion there.. I think the factory being responsible is a good solution

@BrightSoul
Copy link

BrightSoul commented Aug 20, 2020

Can please one of the proponents of WithCulture explain?

Sometimes I do not want a localizer for the current culture. Instead, I want to be able to get multiple localizers for lots of different cultures because I'm in a loop and I'm sending localized e-mails.
I'm using this in production.

The hack proposed by @ryanbrandenburg would indeed work but it would be just that - a hack.

are often sources of confusion for users

If something's confusing, then the solution is to explain it better, not to remove it altogether. There are people who are relying on this functionality. I hope Microsoft re-thinks this decision.

Moreover, the Obsolete description is not helpful at all.

This method is obsolete. Use `CurrentCulture` and `CurrentUICulture` instead.

Suddenly, I'm asked to understand how string localizers work internally and deal with their black magic. This is confusing.
If the solution is to use the overload of IStringLocalizerFactory.Create accepting a CultureInfo, it should mention that.

@dradovic
Copy link

dradovic commented Aug 20, 2020

@BrightSoul of course we sometimes want a localizer in a different culture. That's not a question. By

Can please one of the proponents of WithCulture explain?

I meant, WithCulture vs. the factory pattern in order to get a localizer in a different culture.

@BrightSoul
Copy link

BrightSoul commented Aug 20, 2020

I meant, WithCulture vs. the factory pattern in order to get a localizer in a different culture.

In my opinion, either way is fine but I tend to prefer WithCulture because:

  • People won't have to change their source code;
  • The Obsolete message is poorly written and it currently points to the wrong direction. If it mentioned the factory pattern approach, it would ease the transition but that's not the case;
  • I don't find it confusing at all. With* methods are widely known and used across all languages.

@rubo
Copy link

rubo commented Aug 26, 2020

You're killing a good API. Judging by this thread, there's even no replacement for the functionality cut by this removal.
The suggested workaround of temporarily changing the current culture is awful for many reasons and is not a real solution.

This move doesn't really seem justified and the community opinion is kind of neglected. What a pity.

@joshfriend
Copy link

there's even no replacement for the functionality cut by this removal

I tried the "recommended" path hinted at by the docs and several other things for DAYS with no luck, eventually resorting to hard-coding translations of my text in code. I was using an online translation management service for my resx files, but am no longer able to do that because all my text is jumbled inside a few .cs files. It's an awful mess.

Also a reminder that "deprecation" means planning to remove something at a later date, but instead .NET 3.1 broke the API entirely.

@ivann14
Copy link

ivann14 commented Sep 11, 2020

I tried the "recommended" path hinted at by the docs and several other things for DAYS with no luck, eventually resorting to hard-coding translations of my text in code. I was using an online translation management service for my resx files, but am no longer able to do that because all my text is jumbled inside a few .cs files. It's an awful mess.

Exactly my case... I used the approach mentioned above with any luck whatsoever... Can you at least fix it so that your workaround works?!

@odinnou
Copy link

odinnou commented Dec 1, 2020

Hello, I would liked to migrate an app with ~35k lines of codes from .NET Core 3.1 to .NET 5.0 and currently the only one reason preventing me from doing this is : ResourceManagerWithCultureStringLocalizer class and WithCulture interface member removed

It's very frustrating :(. I prefer stick in .NET Core 3.1 and use an obsolete method instead of use untestable hack

@hishamco
Copy link
Member

hishamco commented Dec 1, 2020

No worry @odinnou you can move to .NET 5.0, if you did something with WithCulture you can easily use something similar to https://github.com/aspnet/Localization/blob/master/src/Microsoft.Extensions.Localization/ResourceManagerWithCultureStringLocalizer.cs, because WithCulture is nothing but localizer with specified culture (other than current culture)

@valdisiljuconoks
Copy link

@rubo exactly, this API change reminds me of usage of static ambient context dependency (thread's current culture) which leads to other nasty side-effects..

@ghost
Copy link

ghost commented Feb 26, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost ghost closed this as completed Feb 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests