NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Translator] Make sure a null locale is handled properly#38127

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:4.4fromjschaedl:translation-issue-38124

Sep 18, 2020

Merged

[Translator] Make sure a null locale is handled properly#38127

fabpot merged 1 commit intosymfony:4.4fromjschaedl:translation-issue-38124

Sep 18, 2020

Conversation

Copy link

Contributor

jschaedl commentedSep 9, 2020•
edited by xabbuh
Loading

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#38124
License	MIT
Doc PR	-

carsonbot added Status: Needs Review Translation Bug Deprecation labels

Sep 9, 2020

jschaedl force-pushed thetranslation-issue-38124 branch 2 times, most recently from9563ed4 tobe749d6Compare

September 9, 2020 12:00

ro0NL approved these changes

Sep 9, 2020

View reviewed changes

src/Symfony/Component/Translation/Translator.php OutdatedShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 9, 2020

nicolas-grekas added this to the4.4 milestone

Sep 10, 2020

jschaedl force-pushed thetranslation-issue-38124 branch from33a861b tod14f539Compare

September 10, 2020 09:52

ro0NL approved these changes

Sep 10, 2020

View reviewed changes

Copy link

Member

fabpot commentedSep 10, 2020

Tests seems broken by this change.

Copy link

Contributor

ro0NL commentedSep 10, 2020

@jschaedl we need to ensure a fixed default locale still, see e.g.7fce86f#diff-59203b25094d9f89c0a4abd9864d8a9c

jschaedl force-pushed thetranslation-issue-38124 branch fromd14f539 toa934e56Compare

September 16, 2020 15:42

Copy link

ContributorAuthor

jschaedl commentedSep 16, 2020

@ro0NL Thanks for the hint :-)

jschaedl force-pushed thetranslation-issue-38124 branch 2 times, most recently from53e2d66 to1fb1f46Compare

September 16, 2020 16:10

jschaedl commented

Sep 16, 2020

View reviewed changes

src/Symfony/Component/Translation/Translator.php Outdated

		publicfunctiongetLocale()
		{
		return$this->locale;
		return$this->locale ?: \Locale::getDefault();

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Borrowed fromhttps://github.com/symfony/contracts/blob/master/Translation/TranslatorTrait.php#L38 which implies that an empty locale is not a valid locale.

Copy link

Contributor

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

jschaedlSep 16, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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 inassertValidLocale on master.

WDYT?

Copy link

Contributor

ro0NLSep 16, 2020•
edited
Loading

Choose a reason for hiding this comment

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

we cannot add new deprecations in a patch release either :)

Copy link

ContributorAuthor

jschaedlSep 16, 2020

Choose a reason for hiding this comment

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

ah true 🙈

ro0NL reviewed

Sep 16, 2020

View reviewed changes

src/Symfony/Component/Translation/Tests/TranslatorTest.php

		publicfunctiongetValidLocalesTests()
		{
		return [
		[''],

Copy link

Contributor

ro0NLSep 16, 2020

Choose a reason for hiding this comment

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

not sure we should change this behavior in a patch release :/
the previous?? \Locale::getDefault() approach was fine no?

Copy link

ContributorAuthor

jschaedlSep 16, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Actually I am not sure too, but good point.

I thought it would be good to have bothTranslator andTranslatorTrait work the same way.

Copy link

Contributor

ro0NLSep 16, 2020

Choose a reason for hiding this comment

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

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 thetranslation-issue-38124 branch from1fb1f46 toac9bd5eCompare

September 18, 2020 07:35

Copy link

ContributorAuthor

jschaedl commentedSep 18, 2020

@ro0NL I reverted things as discussed.

ro0NL approved these changes

Sep 18, 2020

View reviewed changes

xabbuh approved these changes

Sep 18, 2020

View reviewed changes

xabbuh removed the Deprecation label

Sep 18, 2020

fabpot approved these changes

Sep 18, 2020

View reviewed changes

[Translator] Make sure a null locale is handled properly

080ea5a

fabpot force-pushed thetranslation-issue-38124 branch fromac9bd5e to080ea5aCompare

September 18, 2020 08:46

Copy link

Member

fabpot commentedSep 18, 2020

Thank you@jschaedl.

fabpot merged commit785a066 intosymfony:4.4

Sep 18, 2020

jschaedl mentioned this pull request

Sep 18, 2020

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

Closed

Copy link

Contributor

Aliance commentedSep 23, 2020

@fabpot are you planning to release a new version with this fix?

Copy link

ContributorAuthor

jschaedl commentedSep 23, 2020

@Aliance A new patch version comes out at the end of every month.
See:https://symfony.com/doc/current/contributing/community/releases.html

A new Symfony patch version (e.g. 4.4.9, 5.0.9, 5.1.1) comes out roughly every month. It only contains bug fixes, so you can safely upgrade your applications;

You can subscribe for release notifications on this site:https://symfony.com/account/notifications