[Translator] Make sure a null locale is handled properly #38127
Conversation
jschaedl
commented
Sep 9, 2020
•
jschaedl
commented
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #38124 |
| License | MIT |
| Doc PR | - |
jschaedl
force-pushed
the
translation-issue-38124
branch
2 times, most recently
from
9563ed4
to
be749d6
Compare
jschaedl
force-pushed
the
translation-issue-38124
branch
from
33a861b
to
d14f539
Compare
|
Tests seems broken by this change. |
|
@jschaedl we need to ensure a fixed default locale still, see e.g. 7fce86f#diff-59203b25094d9f89c0a4abd9864d8a9c |
jschaedl
force-pushed
the
translation-issue-38124
branch
from
d14f539
to
a934e56
Compare
|
@ro0NL Thanks for the hint :-) |
jschaedl
force-pushed
the
translation-issue-38124
branch
2 times, most recently
from
53e2d66
to
1fb1f46
Compare
| @@ -171,7 +171,7 @@ public function setLocale($locale) | |||
| */ | |||
| public function getLocale() | |||
| { | |||
| return $this->locale; | |||
| return $this->locale ?: \Locale::getDefault(); | |||
There was a problem hiding this comment.
Borrowed from https://github.com/symfony/contracts/blob/master/Translation/TranslatorTrait.php#L38 which implies that an empty locale is not a valid locale.
There was a problem hiding this comment.
if we want to take this route, i would preserve the empty string (as given by the user)
and validate as such in assertValidLocale, rather than silently ignoring it.
I think it's worth a discussion on master.
There was a problem hiding this comment.
I think we could do this:
For now we deprecate the possibility to set en empty locale in 4.4 and stick with $this->locale ?? \Locale::getDefault(); for now and afterwards we add a proper validation in assertValidLocale on master.
WDYT?
There was a problem hiding this comment.
we cannot add new deprecations in a patch release either :)
| @@ -622,7 +650,6 @@ public function getInvalidLocalesTests() | |||
| public function getValidLocalesTests() | |||
| { | |||
| return [ | |||
| [''], | |||
There was a problem hiding this comment.
not sure we should change this behavior in a patch release :/
the previous ?? \Locale::getDefault() approach was fine no?
There was a problem hiding this comment.
Actually I am not sure too, but good point.
I thought it would be good to have both Translator and TranslatorTrait work the same way.
There was a problem hiding this comment.
I thought it would be good have both Translator and TranslatorTrait work the same way.
i think that's worth pursuing yes ... on master, then we wait for @nicolas-grekas's review also :)
jschaedl
force-pushed
the
translation-issue-38124
branch
from
1fb1f46
to
ac9bd5e
Compare
|
@ro0NL I reverted things as discussed. |
fabpot
force-pushed
the
translation-issue-38124
branch
from
ac9bd5e
to
080ea5a
Compare
|
Thank you @jschaedl. |
|
@fabpot are you planning to release a new version with this fix? |
|
@Aliance A new patch version comes out at the end of every month.
You can subscribe for release notifications on this site: https://symfony.com/account/notifications |
