Sounds reasonable.

Codecov Report

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

@@             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

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.

Is there anyway we could test for this?

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?

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

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?

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

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...

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.