[2.1][Locale] ICU data update #5107

Merged
merged 15 commits into from Aug 7, 2012

Conversation

Projects
None yet
7 participants
@ghost

ghost commented Jul 30, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets:
Todo: -
License of the code: MIT

Build Status

The tests are passing in Travis only because it uses ICU 4.4. The tests fails in ICU 4.8 and 49.1. The data files now are up-to-date with ICU 49.1.2. The fails are consistent between 32 and 64 bit for the PHP 5.3.3, 5.3.14 and 5.4.4 (each one compiled with ICU 4.2 to 49).

Basically tests that relies on Locale will have some idiosyncrasies:

  • Some tests will assert a different value for different ICU versions, mostly for currency/date formatting
  • Some tests will skip if not the latest ICU release, code that format/parse something that is directly based on ICU behavior will always implement the behavior from the latest release
  • Some tests will make different assertions for different PHP versions (we already have them, just to note)

The Composer script handler was a very quick implementation, I accept suggestions about it. It basically invokes the update-data.php script. I will also rename this script later since it now behave a little bit different from the original script.

@fabpot, @stof: should I add mine Composer post-install-cmd script in both the project root and component composer.json file?

I'm planning to configure a CI server with a setup similar with mine Symfony development environment to run jobs with the different PHP/ICU releases for the Locale component. But most importantly is to document how I did it since it took a lot of time to have an enviroment that supports all this "ICU hell development". Also, there are some pitfalls regarding testing and to generating the resource bundles for the different ICU versions.

Almost there! I'll try to update later tomorrow (monday).

Owner

fabpot commented Jul 30, 2012

I would not have added the script in the composer.json file but in the .travis.yml file directly instead. We don't want this script to be run whenever someone run composer on the repository.

@ghost

ghost commented Jul 31, 2012

Ok, moved the script call to the .travis.yml file.

@stof stof commented on an outdated diff Jul 31, 2012

src/Symfony/Component/Locale/Locale.php
@@ -43,7 +43,10 @@ class Locale extends \Locale
public static function getDisplayCountries($locale)
{
if (!isset(self::$countries[$locale])) {
- $bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/region');
+ $dataDirectory = \Symfony\Component\Locale\Stub\StubLocale::getDataDirectory();
@stof

stof Jul 31, 2012

Member

you should add a use statement

@stof stof commented on an outdated diff Jul 31, 2012

src/Symfony/Component/Locale/Locale.php
@@ -43,7 +43,10 @@ class Locale extends \Locale
public static function getDisplayCountries($locale)
{
if (!isset(self::$countries[$locale])) {
- $bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/region');
+ $dataDirectory = \Symfony\Component\Locale\Stub\StubLocale::getDataDirectory();
+
+ $bundle = \ResourceBundle::create($locale, $dataDirectory.'/region');
+ //$bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/current/region');
@stof

stof Jul 31, 2012

Member

why adding some commented code ?

@stof stof commented on an outdated diff Jul 31, 2012

src/Symfony/Component/Locale/Locale.php
@@ -98,7 +101,10 @@ public static function getCountries()
public static function getDisplayLanguages($locale)
{
if (!isset(self::$languages[$locale])) {
- $bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/lang');
+ $dataDirectory = \Symfony\Component\Locale\Stub\StubLocale::getDataDirectory();
+ $bundle = \ResourceBundle::create($locale, $dataDirectory.'/lang');
+
+ //$bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/current/lang');
@stof

stof Jul 31, 2012

Member

same here

@stof stof commented on an outdated diff Jul 31, 2012

.../Component/Locale/Stub/DateFormat/FullTransformer.php
@@ -175,7 +175,11 @@ public function getReverseMatchingRegExp($pattern)
{
$that = $this;
- $escapedPattern = preg_quote($pattern, '/');
+ // $escapedPattern = preg_quote($pattern, '/');
@stof

stof Jul 31, 2012

Member

you should not comment old code but remove it

@stof stof commented on an outdated diff Jul 31, 2012

.../Component/Locale/Stub/DateFormat/FullTransformer.php
@@ -175,7 +175,11 @@ public function getReverseMatchingRegExp($pattern)
{
$that = $this;
- $escapedPattern = preg_quote($pattern, '/');
+ // $escapedPattern = preg_quote($pattern, '/');
+
+ // ICU 4.8 recognizes slash ("/") in a value to be parsed as a dash ("-") when parsing a value that
@stof

stof Jul 31, 2012

Member

either that should be removed, or the end of the sentence is missing

@stof stof commented on an outdated diff Jul 31, 2012

...onent/Locale/Tests/Stub/StubIntlDateFormatterTest.php
@@ -857,6 +876,14 @@ public function parseErrorProvider()
array('y-LLLLL-d', '1970-J-1'),
array('y-LLLLL-d', '1970-S-1'),
);
+
+ // TODO: remove this, just to check the regexps
@stof

stof Jul 31, 2012

Member

apparently, you should remove it :)

@ghost

ghost commented Jul 31, 2012

@stof The last commit was just to backup WIP changes and to ask something to @igorw, it is mine "stage" :)

This pull request fails (merged c6a1dcd1 into 20d2e5a).

This pull request fails (merged 76868419 into 20d2e5a).

This pull request fails (merged 7e768eb2 into 20d2e5a).

This pull request fails (merged edaca423 into 20d2e5a).

@ghost

ghost commented Aug 6, 2012

Ready for primetime. The failed test is not related with mine changes.

This pull request passes (merged 386d20c8 into 842b599).

Owner

fabpot commented Aug 7, 2012

After merging this PR, I have more failing tests on my machine:

PHP 5.3.8 - ICU 4.4.1

Is it expected? Is yes, how do we need to run the tests locally?

@ghost

ghost commented Aug 7, 2012

@fabpot Yep, it is expected. You'll need to build the ICU data for this version and export the USE_INTL_ICU_DATA_VERSION before running the tests:

php src/Symfony/Component/Locale/Resources/data/build-data.php
export USE_INTL_ICU_DATA_VERSION=true
phpunit src/Symfony/Component/Locale/Tests

The errors happens because the implementation is in sync with the behavior of ICU 49.1.2 and the shipped data is from data version too. Maybe I should add the steps above in the components README.md since it is more straightforward.

Owner

fabpot commented Aug 7, 2012

@eriksencosta Yes, that would be good to add some more information in the README file.

@ghost

ghost commented Aug 7, 2012

@fabpot Done!

@ghost

ghost commented Aug 7, 2012

And rebased.

This pull request passes (merged 33105e0 into b91a4a8).

Owner

fabpot commented Aug 7, 2012

merged! Thanks you very much for your hard work on this matter.

@fabpot fabpot added a commit that referenced this pull request Aug 7, 2012

@fabpot fabpot merged branch eriksencosta/icu-data-update (PR #5107)
Commits
-------

33105e0 [Locale] added more information to the README.md regarding the need to build the ICU data when the intl extension ICU data version mismatch the one shipped with Symfony
3191c70 [Validator] fixed tests, ICU 4.4 (Travis version) does not have the "my" locale
d7b6bb3 [Locale] updated README.md
6456361 [Locale] added note about ICU data building: use the same PHP intl/ICU version as the desired version to build
025f972 [Locale] renamed function
8da99ca [Locale] updated CHANGELOG.md
d909360 [Locale] don't create a "current" directory anymore, uses only the ICU version as the name of the data directory
3f2b4bf [Locale] changed build data script name
61e3539 [Form][Locale] updated minimum ICU version to 4.0
0d442c7 [Locale] as of ICU 4.8+, slash ("/") and dash ("-") are interchangeable, you can use any of them in a date/time pattern
90d6dc3 [Locale] fixed tests
86da1b3 [Locale] added ICU update-data script to be run before the test suite
5fd4eb1 [Locale] updated ICU data paths
64ccee7 [Locale] updated ICU data
0d0a8d4 [Locale] updated script to generate separated data directories for different ICU releases

Discussion
----------

[2.1][Locale] ICU data update

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets:
Todo: -
License of the code: MIT

[![Build Status](https://secure.travis-ci.org/eriksencosta/symfony.png?branch=icu-data-update)](http://travis-ci.org/eriksencosta/symfony)

The tests are passing in Travis only because it uses ICU 4.4. The tests fails in ICU 4.8 and 49.1. The data files now are up-to-date with ICU 49.1.2. The fails are consistent between 32 and 64 bit for the PHP 5.3.3, 5.3.14 and 5.4.4 (each one compiled with ICU 4.2 to 49).

Basically tests that relies on Locale will have some idiosyncrasies:

- Some tests will assert a different value for different ICU versions, mostly for currency/date formatting
- Some tests will skip if not the latest ICU release, code that format/parse something that is directly based on ICU behavior will always implement the  behavior from the latest release
- Some tests will make different assertions for different PHP versions (we already have them, just to note)

The Composer script handler was a very quick implementation, I accept suggestions about it. It basically invokes the `update-data.php` script. I will also rename this script later since it now behave a little bit different from the original script.

@fabpot, @stof: should I add mine Composer `post-install-cmd` script in both the project root and component `composer.json` file?

I'm planning to configure a CI server with a setup similar with mine Symfony development environment to run jobs with the different PHP/ICU releases for the Locale component. But most importantly is to document how I did it since it took a lot of time to have an enviroment that supports all this "ICU hell development". Also, there are some pitfalls regarding testing and to generating the resource bundles for the different ICU versions.

Almost there! I'll try to update later tomorrow (monday).

---------------------------------------------------------------------------

by fabpot at 2012-07-30T08:14:29Z

I would not have added the script in the `composer.json` file but in the `.travis.yml` file directly instead. We don't want this script to be run whenever someone run composer on the repository.

---------------------------------------------------------------------------

by eriksencosta at 2012-07-31T03:07:58Z

Ok, moved the script call to the `.travis.yml` file.

---------------------------------------------------------------------------

by eriksencosta at 2012-07-31T12:38:23Z

@stof The last commit was just to backup WIP changes and to ask something to @igorw, it is mine "stage" :)

---------------------------------------------------------------------------

by travisbot at 2012-08-06T03:05:16Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2044244) (merged c6a1dcd1 into 20d2e5a).

---------------------------------------------------------------------------

by travisbot at 2012-08-06T03:16:12Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2044311) (merged 76868419 into 20d2e5a).

---------------------------------------------------------------------------

by travisbot at 2012-08-06T03:52:20Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2044425) (merged 7e768eb2 into 20d2e5a).

---------------------------------------------------------------------------

by travisbot at 2012-08-06T04:04:30Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2044461) (merged edaca423 into 20d2e5a).

---------------------------------------------------------------------------

by eriksencosta at 2012-08-06T04:33:55Z

Ready for primetime. The failed test is not related with mine changes.

---------------------------------------------------------------------------

by travisbot at 2012-08-07T02:32:19Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2053806) (merged 386d20c8 into 842b599).

---------------------------------------------------------------------------

by fabpot at 2012-08-07T13:51:59Z

After merging this PR, I have more failing tests on my machine:

PHP 5.3.8 - ICU 4.4.1

Is it expected? Is yes, how do we need to run the tests locally?

---------------------------------------------------------------------------

by eriksencosta at 2012-08-07T14:56:23Z

@fabpot Yep, it is expected. You'll need to build the ICU data for this version and export the `USE_INTL_ICU_DATA_VERSION` before running the tests:

    php src/Symfony/Component/Locale/Resources/data/build-data.php
    export USE_INTL_ICU_DATA_VERSION=1
    phpunit src/Symfony/Component/Locale/Tests

The errors happens because the implementation is in sync with the behavior of ICU 49.1.2 and the shipped data is from data version too. Maybe I should add the steps above in the components `README.md` since it is more straightforward.

---------------------------------------------------------------------------

by fabpot at 2012-08-07T14:58:44Z

@eriksencosta Yes, that would be good to add some more information in the README file.

---------------------------------------------------------------------------

by eriksencosta at 2012-08-07T15:21:04Z

@fabpot Done!

---------------------------------------------------------------------------

by eriksencosta at 2012-08-07T15:21:29Z

And rebased.
7dbadbf

@fabpot fabpot merged commit 33105e0 into symfony:master Aug 7, 2012

Member

lyrixx commented Aug 7, 2012

@fabpot @eriksencosta :

Tests are always broken on my laptop.

Before the merge :


1) Symfony\Component\Locale\Tests\Stub\StubIntlDateFormatterTest::testFormatWithDefaultTimezoneIntl
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1970-01-01 00:00:00'
+'1970-01-01 01:00:00'

After the merge :


There were 4 failures:

1) Symfony\Component\Locale\Tests\Stub\StubIntlDateFormatterTest::testFormatWithDefaultTimezoneIntl
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1970-01-01 00:00:00'
+'1970-01-01 01:00:00'

/home/gregoire/dev/symfony/src/Symfony/Component/Locale/Tests/Stub/StubIntlDateFormatterTest.php:481

2) Symfony\Component\Locale\Tests\Stub\StubNumberFormatterTest::testFormatCurrencyWithCurrencyStyleCostaRicanColonsRoundingStub with data set #0 (100, 'CRC', '₡', '%s10
0')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'₡100'
+'CRC100'

/home/gregoire/dev/symfony/src/Symfony/Component/Locale/Tests/Stub/StubNumberFormatterTest.php:164

3) Symfony\Component\Locale\Tests\Stub\StubNumberFormatterTest::testFormatCurrencyWithCurrencyStyleCostaRicanColonsRoundingStub with data set #1 (-100, 'CRC', '₡', '(%s100)')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'(₡100)'
+'(CRC100)'

/home/gregoire/dev/symfony/src/Symfony/Component/Locale/Tests/Stub/StubNumberFormatterTest.php:164

4) Symfony\Component\Locale\Tests\Stub\StubNumberFormatterTest::testFormatCurrencyWithCurrencyStyleCostaRicanColonsRoundingStub with data set #2 (1000.12, 'CRC', '₡', '%s1,000')
Failed asserting that two strings are equal.

--- Expected
+++ Actual
@@ @@
-'₡1,000'
+'CRC1,000'

So i follow the README :

  • I installed libicu-dev
  • I run php src/Symfony/Component/Locale/Resources/data/build-data.php
  • I exported export USE_INTL_ICU_DATA_VERSION=true

And now :


1) Symfony\Component\Locale\Tests\Stub\StubIntlDateFormatterTest::testFormatWithDefaultTimezoneIntl
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1970-01-01 00:00:00'
+'1970-01-01 01:00:00'

If you need more info about my laptop, you can read my previous issue #5082

@ghost

ghost commented Aug 7, 2012

@lyrixx I'll take a look at that. Thanks!

Member

lyrixx commented Aug 7, 2012

May be we should re-open the issue associated with this PR ? is it possible ?
ping @fabpot

@ghost

ghost commented Aug 7, 2012

@lyrixx The error is not associated with the data update.

Member

webmozart commented on 0d0a8d4 Feb 27, 2013

What's the reasoning behind creating separate directories for separate ICU versions? Why not install the data into the main directory as before?

I think that this change is responsible for the frequent "The * resource bundle could not be loaded for locale "*"" error messages, e.g. #5279.

Member

webmozart replied Feb 27, 2013

PR this was merged in: #5107

Member

stof replied Feb 27, 2013

@bschussek being able to generate the data for different ICU versions and using the right one, as you cannot load data generated for a different ICU version.

but the issue is that symfony ships with a single version (others are ignored), and the Locale component does not allow searching for it in another location than the component itself. We should probably have a way to distribute different versions based on which intl version you have.

Member

webmozart replied Feb 27, 2013

as you cannot load data generated for a different ICU version.

Why not? Are you sure about that? I mean, is it technically impossible to use \ResourceBundle from e.g. ICU 4.8 for loading *.res files from different ICU versions?

Member

stof replied Feb 27, 2013

@bschussek it works for some versions but not all. IIRC, the format of data is incompatible between ICU 42 and ICU 48

Member

webmozart replied Feb 27, 2013

ok, I will test this more thoroughly then

Contributor

mvrhov replied Feb 28, 2013

Finding an issue on github is near to impossible 👎 I've already done the testing. It was in one of the locale data issues.
AFAIR: The main problem is that the ICU res files changed format. Now the newer versions of genrb do support building the resources in the old format but the older versions of ICU were still unable to read those files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment