Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() #38230

jschaedl · 2020-09-18T09:01:35Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
Deprecations?	yes
Tickets	Relates to #38124
License	MIT
Doc PR	-

This is a follow-up PR on #38127 and a proposal for discussion.

While working on #38127 I realized that Translator::getLocale() and TranslatorTrait::getLocale() behave differently. For the Translator an empty locale is a valid locale and for the TranslatorTrait an empty locale will result in getting the default locale when calling getLocale().

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 an InvalidArgumentException in version 6.

/cc @ro0NL

…ocale()

nicolas-grekas · 2020-09-18T09:26:09Z

src/Symfony/Component/Translation/Translator.php

@@ -91,6 +91,10 @@ class Translator implements TranslatorInterface, TranslatorBagInterface, LocaleA
     */
    public function __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__));


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?

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 in getLocale() 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 in assertValidLocale() 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 of assertValidLocale() and getLocale() in version 6.

I personally would prefer the second approach, but I also might miss something :-)

we should patch assertValidLocale 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.

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())

~~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 doing new 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.

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 😅

I don't see the point of the deprecation. Let's move on.

ro0NL · 2020-09-23T14:44:49Z

src/Symfony/Component/Translation/Translator.php

@@ -158,7 +162,7 @@ public function setLocale(string $locale)
     */
    public function getLocale()
    {
-        return $this->locale;
+        return $this->locale ?: \Locale::getDefault();


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 #...  | License | MIT | Doc PR | symfony/symfony-docs#...  same as #38283, and should help streamlining #38230 Commits ------- d65d39d [Contracts][Translation] Optional Intl dependency

fabpot · 2020-09-24T15:18:59Z

@jschaedl Can you remove the deprecation and fix the getLocale method?

nicolas-grekas · 2020-09-24T15:30:15Z

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 · 2020-09-24T15:53:53Z

@fabpot

Can you remove the deprecation and fix the getLocale method?

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 #38230 Provides consistent behavior with `TranslatorTrait::getLocale()`. Commits ------- 20120a3 [Translator] fix fallback to Locale::getDefault()

nicolas-grekas · 2021-07-20T14:00:51Z

Fixed in #42130

carsonbot added Status: Needs Review Bug Deprecation labels Sep 18, 2020

jschaedl force-pushed the translation-issue-38124-follow-up branch from 6de7ed8 to fdb5163 Compare September 18, 2020 09:03

Make Translator::getLocale() behave the same as TranslatorTrait::getL…

59b3535

…ocale()

jschaedl force-pushed the translation-issue-38124-follow-up branch from fdb5163 to 59b3535 Compare September 18, 2020 09:08

jschaedl changed the title ~~Make Translator::getLocale() behave the same like TranslatorTrait::getLocale()~~ Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() Sep 18, 2020

nicolas-grekas reviewed Sep 18, 2020

View reviewed changes

xabbuh added this to the next milestone Sep 18, 2020

xabbuh added the Translation label Sep 18, 2020

ro0NL reviewed Sep 23, 2020

View reviewed changes

ro0NL mentioned this pull request Sep 23, 2020

[Contracts][Translation] Optional Intl dependency #38285

Merged

nicolas-grekas closed this Sep 24, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

ro0NL mentioned this pull request Jul 15, 2021

[Translation] remove unused code #42128

Closed

nicolas-grekas mentioned this pull request Jul 15, 2021

[Translation] fix fallback to Locale::getDefault() #42130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() #38230

Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() #38230

jschaedl commented Sep 18, 2020 •

edited

Loading

nicolas-grekas Sep 18, 2020

jschaedl Sep 18, 2020

ro0NL Sep 18, 2020

nicolas-grekas Sep 18, 2020

ro0NL Sep 18, 2020 •

edited

Loading

ro0NL Sep 18, 2020 •

edited

Loading

nicolas-grekas Sep 24, 2020

ro0NL Sep 23, 2020 •

edited

Loading

fabpot commented Sep 24, 2020

nicolas-grekas commented Sep 24, 2020

jschaedl commented Sep 24, 2020

nicolas-grekas commented Jul 20, 2021

Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() #38230

Make Translator::getLocale() behave the same as TranslatorTrait::getLocale() #38230

Conversation

jschaedl commented Sep 18, 2020 • edited Loading

nicolas-grekas Sep 18, 2020

Choose a reason for hiding this comment

jschaedl Sep 18, 2020

Choose a reason for hiding this comment

ro0NL Sep 18, 2020

Choose a reason for hiding this comment

nicolas-grekas Sep 18, 2020

Choose a reason for hiding this comment

ro0NL Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

ro0NL Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Sep 24, 2020

Choose a reason for hiding this comment

ro0NL Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

fabpot commented Sep 24, 2020

nicolas-grekas commented Sep 24, 2020

jschaedl commented Sep 24, 2020

nicolas-grekas commented Jul 20, 2021

jschaedl commented Sep 18, 2020 •

edited

Loading

ro0NL Sep 18, 2020 •

edited

Loading

ro0NL Sep 18, 2020 •

edited

Loading

ro0NL Sep 23, 2020 •

edited

Loading