[Locale] Fixed the StubLocaleTest for ICU versions lower than 4.8 #6515
Conversation
Symbol for Brazilian Real is BR$ only in ICU 4.8. All previous and later versions use R$. Links to the data files: * http://source.icu-project.org/repos/icu/icu/tags/release-4-8/source/data/curr/en.txt * http://source.icu-project.org/repos/icu/icu/tags/release-4-4/source/data/curr/en.txt * http://source.icu-project.org/repos/icu/icu/tags/release-49-1/source/data/curr/en.txt
|
Note that currently USE_ICU_DATA_VERSION is required for some tests to pass. This is inconsistent with Locale::getIcuDataVersion() which returns Locale::ICU_DATA_VERSION if USE_ICU_DATA_VERSION is not set. Technically intl extension shouldn't be needed to run the stub tests. Following code could fix the issue for StubLocale tests: class StubLocaleTest extends LocaleTestCase
{
// ...
protected function getIntlExtensionIcuVersion()
{
// Locale::getIcuDataVersion(), which is used to determine the directory of stubs,
// uses Locale::ICU_DATA_VERSION if USE_INTL_ICU_DATA_VERSION is not given
if (!getenv('USE_INTL_ICU_DATA_VERSION')) {
return Locale::ICU_DATA_VERSION;
}
return parent::getIntlExtensionIcuVersion();
}
}This code cannot be placed in the LocaleTestCase as intl tests need the icu data version from the intl extension. I'm not sure if this is desired and therefore I didn't include it in my PR. Let me know your thoughts and wether I should create another PR for this, use the current PR or forget about it. |
This PR was merged into the 2.1 branch. Commits ------- ef6f241 [Locale] Fixed the StubLocaleTest for ICU versions lower than 4.8. Discussion ---------- [Locale] Fixed the StubLocaleTest for ICU versions lower than 4.8 Bug fix: no Feature addition: no Backwards compatibility break: no Symfony2 tests pass: no but for other reason in an another component Fixes the following tickets: #5517 Todo: - License of the code: MIT Documentation PR: - Symbol for Brazilian Real is BR$ only in ICU 4.8. All previous and later versions use R$. Links to the relevant data files: * http://source.icu-project.org/repos/icu/icu/tags/release-4-8/source/data/curr/en.txt * http://source.icu-project.org/repos/icu/icu/tags/release-4-4/source/data/curr/en.txt * http://source.icu-project.org/repos/icu/icu/tags/release-49-1/source/data/curr/en.txt --------------------------------------------------------------------------- by jakzal at 2012-12-29T19:23:25Z Note that currently *USE_ICU_DATA_VERSION* is required for some tests to pass. This is inconsistent with *Locale::getIcuDataVersion()* which returns *Locale::ICU_DATA_VERSION* if *USE_ICU_DATA_VERSION* is not set. Technically intl extension shouldn't be needed to run the stub tests. Following code could fix the issue for StubLocale tests: ```php class StubLocaleTest extends LocaleTestCase { // ... protected function getIntlExtensionIcuVersion() { // Locale::getIcuDataVersion(), which is used to determine the directory of stubs, // uses Locale::ICU_DATA_VERSION if USE_INTL_ICU_DATA_VERSION is not given if (!getenv('USE_INTL_ICU_DATA_VERSION')) { return Locale::ICU_DATA_VERSION; } return parent::getIntlExtensionIcuVersion(); } } ``` This code cannot be placed in the LocaleTestCase as intl tests need the icu data version from the intl extension. I'm not sure if this is desired and therefore I didn't include it in my PR. Let me know your thoughts and wether I should create another PR for this, use the current PR or forget about it.
This PR was merged into the master branch. Commits ------- 97bc257 [Locale] Rolled back normalizeIcuVersion() method in unit tests. Discussion ---------- [Locale] Rolled back normalizeIcuVersion() method in unit tests. We just need to check if version is 4.4 or 4.8 and we do not need to know if it is 4.8.0 or 4.8.1.1 (at least for now there are no big differences between minor ICU versions). This change basically rolls back modifications made in #6109 to fix #5288. Those modifications didn't really fix the issue since ICU>=4.9 wasn't handled properly. Part of the roll back was made in #6515 (2.1 branch). This change makes master branch consistent with what we have in 2.0 and 2.1 (bespoke tests are passing in those branches). Bug fix: no Feature addition: no Backwards compatibility break: no Symfony2 tests pass: one test fails in the Propel1 bridge Fixes the following tickets: #5517 Todo: - License of the code: MIT Documentation PR: -
|
@jakzal: I just noticed some failures running 2.1 tests locally. I believe it was the fault of the ICU version comparison methods in the base Locale TestCase class. Specifically, all of those methods normalized the versions using: My current system reported ICU was 4.8.1.1, but after normalization, this was equal to 4.8. I tried changing the three ICU comparison methods to use Is there any reason we're using Brazilian currency here? It seems to have been the cause of all of these silly edge cases. |
|
@jmikola I'm not the original author of these tests. Imho it's not a good practice to make tests dependent on the environment they're run (if statements should be avoided in general). I don't understand why can't we simply make stubs to use the latest stable intl version (and avoid checking the version). |
|
@jakzal The stub tests are comparing the stub implementation to the real intl implementation (to check that it behaves the same). Forcing the user to have the latest intl to run the tests is a bad idea IMO |
|
@stof that's not what I meant. In the end what stubs are doing is they're loading data from the filesystem. Why should we be comparing it to the real intl implementation? We're getting the data files from intl anyway, right? |
|
@jakzal the stub is not just loading the file and returning the raw data. There is some logic applied (which should be consistent with the logic applied by Intl) |
|
Again, please check #7386. This discussion is obsolete. |
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no but for other reason in an another component
Fixes the following tickets: #5517
Todo: -
License of the code: MIT
Documentation PR: -
Symbol for Brazilian Real is BR$ only in ICU 4.8. All previous and later versions use R$.
Links to the relevant data files: