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

Get default locale before setting a new locale #11200

Merged
merged 2 commits into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@liviascapin
Contributor

liviascapin commented Sep 15, 2017

This should fix #11199

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 15, 2017

Member

Sounds reasonable.

Member

dereuromark commented Sep 15, 2017

Sounds reasonable.

@dereuromark dereuromark added the Defect label Sep 15, 2017

@dereuromark dereuromark added this to the 3.5.3 milestone Sep 15, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 15, 2017

Codecov Report

Merging #11200 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11200      +/-   ##
============================================
+ Coverage     93.14%   93.14%   +<.01%     
  Complexity    12981    12981              
============================================
  Files           437      437              
  Lines         33623    33624       +1     
============================================
+ Hits          31318    31319       +1     
  Misses         2305     2305
Impacted Files Coverage Δ Complexity Δ
src/I18n/I18n.php 91.02% <100%> (+0.11%) 28 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639a1c7...de5020c. Read the comment docs.

codecov-io commented Sep 15, 2017

Codecov Report

Merging #11200 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11200      +/-   ##
============================================
+ Coverage     93.14%   93.14%   +<.01%     
  Complexity    12981    12981              
============================================
  Files           437      437              
  Lines         33623    33624       +1     
============================================
+ Hits          31318    31319       +1     
  Misses         2305     2305
Impacted Files Coverage Δ Complexity Δ
src/I18n/I18n.php 91.02% <100%> (+0.11%) 28 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639a1c7...de5020c. Read the comment docs.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 15, 2017

Member

Is there anyway we could test for this?

Member

markstory commented Sep 15, 2017

Is there anyway we could test for this?

@liviascapin

This comment has been minimized.

Show comment
Hide comment
@liviascapin

liviascapin Sep 18, 2017

Contributor

I'm not sure how, since we call I18n::getLocale() in the setup of the test: https://github.com/cakephp/cakephp/blob/master/tests/TestCase/I18n/I18nTest.php#L44 which sets the default class variable before we can actually test if setLocale() overwrites it. So we can not test this correctly with the current setup and I'm unsure how to proceed.

But the general idea would be to read the 'default' without setting the class variable, then setting another locale (using setLocale()) and then asserting that getLocale() returns the set locale, while getDefaultLocale returns the default we extracted earlier.

I imagine the test to go something like this:

public function testDefaultLocale() {
    $default = Locale::getDefault() ?: I18n::DEFAULT_LOCALE;
    $newLocale = 'de_DE';
    I18n::setLocale($newLocale);
    $this->assertEquals($newLocale, I18n::getLocale());
    $this->assertEquals($default, I18n::getDefaultLocale());
}

What do you think? And how can we integrate that in the current test setup?

Contributor

liviascapin commented Sep 18, 2017

I'm not sure how, since we call I18n::getLocale() in the setup of the test: https://github.com/cakephp/cakephp/blob/master/tests/TestCase/I18n/I18nTest.php#L44 which sets the default class variable before we can actually test if setLocale() overwrites it. So we can not test this correctly with the current setup and I'm unsure how to proceed.

But the general idea would be to read the 'default' without setting the class variable, then setting another locale (using setLocale()) and then asserting that getLocale() returns the set locale, while getDefaultLocale returns the default we extracted earlier.

I imagine the test to go something like this:

public function testDefaultLocale() {
    $default = Locale::getDefault() ?: I18n::DEFAULT_LOCALE;
    $newLocale = 'de_DE';
    I18n::setLocale($newLocale);
    $this->assertEquals($newLocale, I18n::getLocale());
    $this->assertEquals($default, I18n::getDefaultLocale());
}

What do you think? And how can we integrate that in the current test setup?

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 18, 2017

Member

@liviascapin I think that test looks good. We could also read the class properties with phpunit as well.

Member

markstory commented Sep 18, 2017

@liviascapin I think that test looks good. We could also read the class properties with phpunit as well.

@markstory markstory self-assigned this Sep 18, 2017

@markstory markstory added the i18n label Sep 18, 2017

@liviascapin

This comment has been minimized.

Show comment
Hide comment
@liviascapin

liviascapin Sep 18, 2017

Contributor

Thanks! :)
But the test won't work properly as long as we call I18n::getLocale() in the setUp() method (https://github.com/cakephp/cakephp/blob/master/tests/TestCase/I18n/I18nTest.php#L44). It will probably work fine, but it will not be able to determine if setLocale() overwrites the default or not (and that is the point of all of this). What can we do about that?

Contributor

liviascapin commented Sep 18, 2017

Thanks! :)
But the test won't work properly as long as we call I18n::getLocale() in the setUp() method (https://github.com/cakephp/cakephp/blob/master/tests/TestCase/I18n/I18nTest.php#L44). It will probably work fine, but it will not be able to determine if setLocale() overwrites the default or not (and that is the point of all of this). What can we do about that?

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 18, 2017

Member

You should be able to use PHPUnit or Reflection APIs to read the value of the protected property.

Member

markstory commented Sep 18, 2017

You should be able to use PHPUnit or Reflection APIs to read the value of the protected property.

@liviascapin

This comment has been minimized.

Show comment
Hide comment
@liviascapin

liviascapin Sep 19, 2017

Contributor

Ah I think I get it now :)

I tried to write the test, but I still have a problem with it and I feel kind of stuck. I tried to ensure that the test fails in 3.5.2 and doesn't fail with this fix. However, if I run all the tests, this test passes in both cases. If I only run this specific test, it fails as expected (in 3.5.2). I guess it is because other tests (such as the FloatTypeTest or the NumberTest) use getLocale() too...

Contributor

liviascapin commented Sep 19, 2017

Ah I think I get it now :)

I tried to write the test, but I still have a problem with it and I feel kind of stuck. I tried to ensure that the test fails in 3.5.2 and doesn't fail with this fix. However, if I run all the tests, this test passes in both cases. If I only run this specific test, it fails as expected (in 3.5.2). I guess it is because other tests (such as the FloatTypeTest or the NumberTest) use getLocale() too...

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 19, 2017

Member

Yeah. I think we're going to have a really hard time testing this. I'll merge as the tests are still useful to have.

Member

markstory commented Sep 19, 2017

Yeah. I think we're going to have a really hard time testing this. I'll merge as the tests are still useful to have.

@markstory markstory merged commit 7900c6a into cakephp:master Sep 19, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment