Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Make Translator::getLocale() behave the same as TranslatorTrait::getLocale()#38230
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6de7ed8 tofdb5163Comparefdb5163 to59b3535Compare| publicfunction__construct(string$locale,MessageFormatterInterface$formatter =null,string$cacheDir =null,bool$debug =false,array$cacheVary = []) | ||
| { | ||
| if ('' ===$locale) { | ||
| trigger_deprecation('symfony/translator','5.2',sprintf('Passing "" as the $locale argument to %s::%s() is deprecated, it will throw an InvalidArgumentException in version 6.0.',static::class,__METHOD__)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this case makes sense. It conflicts, from a logical pov, with the change in getLocale(). It's one or the other to me, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it depends.
If we say injecting an empty locale in the constructor is ok, because we make sure that the default locale will be returned ingetLocale() anyway, then yes a deprecation is not needed. Only Tests need some adjustments then.
But if we say an empty locale is not valid, we could validate it inassertValidLocale() and also simplify getLocale() by removing the\?: Locale::getDefault(). But this would be a BC break if we do it right now, so I only introduced the deprecation for now and would do the adjustments ofassertValidLocale() andgetLocale() in version 6.
I personally would prefer the second approach, but I also might miss something :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should patchassertValidLocale IMHO. deprecating an empty string there, throwing in 6.0
given the locale is not a nullable in this case, there's no need for fallback to\Locale::getDefault() as well
thus the deprecated case (an empty string) keeps working in 5.x, as currently that's a BC break.
we should proboably call$this->setLocale($locale) at construct (to ensure validation), meaning the deprecation will automatically happen as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would personnaly take the other way, for consistency with TranslatorTrait, and because that's what provides the most power to users (they can decide to rely on\Locale::getDefault())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it can also take a nullable again let's not :)
but yes,?: getDefault() works, at most it hides"0" sneaking into the system. But it's consistent behavior with the trait, i agree.
IMHO the service differs from the trait, in a way doingnew Translator(Locale::getDefault() makes sense, otherwise we have to document the empty string behavior as well i figured.
The trait needs a hardcoded fallback mechanism, due the lack of a constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we still need a deprecation here (and in setLocale) then
if (!$locale) {trigger_deprecation('will be Locale::getDefault() (currently "...") in 6.0');// $locale = Locale::getDefault()}
and fallback as of 6.0
... so yes, the current behavior conflicts 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see the point of the deprecation. Let's move on.
| publicfunctiongetLocale() | ||
| { | ||
| return$this->locale; | ||
| return$this->locale ?: \Locale::getDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see also#38279, we'll introduce the same issue now :)
This PR was merged into the 4.4 branch.Discussion----------[Contracts][Translation] Optional Intl dependency| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->same as#38283, and should help streamlining#38230Commits-------d65d39d [Contracts][Translation] Optional Intl dependency
fabpot commentedSep 24, 2020
@jschaedl Can you remove the deprecation and fix the getLocale method? |
nicolas-grekas commentedSep 24, 2020
On second thought, I agree with@ro0NL: this is a behavioral BC break. The only path forward would be deprecating the empty locale but still returning the empty string when passed. Then in v6 we would return the global value. BUT, this looks like a quite costly path when the argument in favor is "consistency". I'm therefor closing, we need to focus on actual use cases. |
jschaedl commentedSep 24, 2020
I actually already did:master...jschaedl:translation-issue-38124-follow-up but the PR is closed now. |
…s-grekas)This PR was merged into the 4.4 branch.Discussion----------[Translation] fix fallback to Locale::getDefault()| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replaces#42128 and#38230Provides consistent behavior with `TranslatorTrait::getLocale()`.Commits-------20120a3 [Translator] fix fallback to Locale::getDefault()
nicolas-grekas commentedJul 20, 2021
Fixed in#42130 |
Uh oh!
There was an error while loading.Please reload this page.
This is a follow-up PR on#38127 and a proposal for discussion.
While working on#38127 I realized that
Translator::getLocale()andTranslatorTrait::getLocale()behave differently. For theTranslatoran empty locale is a valid locale and for theTranslatorTraitan empty locale will result in getting the default locale when callinggetLocale().To make things more consistent, I thought it would be a good idea to deprecate the injection of en empty locale, so we can add a proper validation in
assertValidLocale(string $locale)and throw anInvalidArgumentExceptionin version 6./cc@ro0NL