[Intl] Refactored Locale component into two new components Icu and Intl #7386

webmozart · 2013-03-15T09:16:16Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#5279
License	MIT
Doc PR	symfony/symfony-docs#2312

The Intl component is now a simple drop-in replacement layer for the C intl extension. Install it via Composer and have it available automatically if the intl extension is not available.

Additionally, the component ships data from the ICU library which can be accessed through the methods:

use Symfony\Component\Intl\Intl;

Intl::getCurrencyBundle()->...
Intl::getLanguageBundle()->...
Intl::getLocaleBundle()->...
Intl::getRegionBundle()->...

If the intl extension is installed, Composer will install the ICU data for the ICU version in the intl extension. If the intl extension is not installed, Composer will use stub ICU data for the latest ICU version (see Intl::getStubIcuVersion()).

See the README for more information.

Todo:

finish the Intl README file
update the Icu README file
update the documentation
make parameter $locale optional (default to \Locale::getDefault()) in resource bundle methods
remove (Icu)?Version::compare calls in the tests
solve deployment problem when trying to install incompatible symfony/icu version listed in composer.lock

Create the following branches in the Icu component:

1.0.x
1.1.x
1.2.x

stof · 2013-03-15T10:24:38Z

The circular dependency between symfony/intl and symfony/icu looks weird to me.

stof · 2013-03-15T10:27:41Z

And IMO, \Locale::setDefault() should not throw an exception if you set the default to en as it is the supported case

stof · 2013-03-15T10:31:33Z

And some of the utils used to build the ICU data should be moved to Icu instead of being in Intl IMO

webmozart · 2013-03-15T11:17:41Z

@stof The circular dependency can't be removed. One component cannot be installed without the other (by design). The separation into separate components has a single purpose: separate versioning.

The first release version of symfony/intl will be 2.3.0
The first release versions of symfony/icu will be 1.x.0 for x in [40, 42, 44, 46, 48, 49, 50]

Equally, the symfony/intl component has one master branch while the symfony/icu repository has seven (40-master, 42-master etc.).

To make maintenance easier, every piece of code that is not directly related to the ICU data of a specific version is contained in the Intl component. That's why the scripts are there and not in the Icu component.

I'm ok for changing \Locale::setDefault(), I just left the old behavior in place here.

stof · 2013-03-15T11:28:15Z

@bschussek But circular dependencies may cause some weird issues when solving.
Thus, you still have an issue: when you don't have lib-ICU installed, the latest version of symfony/Icu will always be installed. But this suppose that the stub is always written against the latest ICU version (which will be wrong as the stub will not be upgraded to a newer ICU in tagged releases when a new ICU version gets supported in symfony/Icu).
The stub package has to require a particular version based on the ICU version it uses.

Btw, why naming the branch 48-master and then aliasing it as 1.48.x-dev (requiring to do the alias for all branches) instead of naming the branches 1.48.x directly ?

webmozart · 2013-03-15T11:48:35Z

@stof If you don't have lib-ICU installed, it doesn't matter which symfony/icu version is installed. It will not be used but be simply ignored.

I wasn't aware that I could simply name the branch 1.48.x. Will requiring 1.48.x-dev work then?

stof · 2013-03-15T11:55:43Z

@bschussek It will matter unless the stub implementation is able to load the ResourceBundle for any ICU version (including future versions) which is not the case as the format of the ICU file is not BC (which is what you are trying to fix for the case where the real Intl is used). The stub implementation of ResourceBundle will be tied to an ICU version.

For the branch name, yes. This is exactly how the 2.0, 2.1 and 2.2 branches are working (the .x at the end is optional, as well as the v at the beginning)

And btw, the Symfony\Component\Intl\ResourceBundle\Stub\* classes are wrong. They are trying to load the data from the Intl component, where they are not available.

webmozart · 2013-03-15T12:32:48Z

@stof Again, the files in the Icu component are completely ignored if the intl extension is not available. That means:

If the intl extension is available with ICU version N, the Icu component 1.N.* is loaded. The intl extension classes are used (version N), the resource bundles are loaded from Icu/Resources/data (N <= version < N+1).
If the intl extension is not available, the latest Icu component is loaded, but not used. The stub classes from the Intl component are used (version 50.1.0), the resource bundles are loaded from Intl/Resources/data (50.1.0 <= version < 51).

That means that if the intl extension is not available, the Intl component emulates a specific intl version (50.1.0 right now). We don't even have to load the Icu component in this case, but there is currently no way to avoid this with Composer.

webmozart · 2013-03-15T12:40:59Z

They are trying to load the data from the Intl component, where they are not available.

@stof Have a closer look ;)

stof · 2013-03-15T14:16:05Z

@bschussek Having a close look is not as easy as usually when github refuses to display the diff for the PR :)

Btw, it looks like the bin script are trying to use an inexistant autoload file: https://github.com/bschussek/symfony/blob/c3b98a066e3ca80874701d8c299f726c676ef621/src/Symfony/Component/Intl/Resources/bin/icu-version.php#L15

webmozart · 2013-03-15T14:57:26Z

Anybody else wondering, you can look at the source view here: https://github.com/bschussek/symfony/tree/intl/src/Symfony/Component/Intl

@stof I added the autoload file now.

stof · 2013-03-15T15:21:28Z

@bschussek Sure we can see the source there, but not easily the difference compared to master...

bendavies · 2013-03-15T15:27:56Z

@stof that's not really @bschussek 's fault, is it?
https://github.com/symfony/symfony/pull/7386.patch
https://github.com/symfony/symfony/pull/7386.diff

stof · 2013-03-15T15:36:56Z

@bendavies noit is not. It is simply because the intl data are lots of files

bendavies · 2013-03-15T15:42:10Z

@stof i am aware of why github won't show the diff.
I was merely pointing out there there are many ways of viewing the the PR changes, because you complained twice about it.

stof · 2013-03-15T15:43:25Z

@bendavies Seeing the patch does not allow commenting on it to do the PR review.

@bschussek in CurrencyBundleInterface (and the other bundle interfaces), the phpdoc mentions \Locale::getLocale() instead of \Locale::getDefault()

Tobion · 2013-03-15T15:48:42Z

I would suggest to open another PR with the binary changes and keep it off from this PR to ease reviewing.

webmozart · 2013-03-15T16:05:00Z

@stof Fixed.

@Tobion I would ask you to use the patch view, since splitting the PR is unnecessarily complicated now.

asm89 · 2013-03-15T16:35:06Z

Why are the separate branches necessary? Can't the component ship all binary data?

stof · 2013-03-15T16:43:55Z

@asm89 the goal is precisely to avoid having all the different versions together and having to choose the right ones (which is why the Locale component has a rule to ignore the other versions currently)

webmozart · 2013-03-16T10:57:44Z

I built all the ICU branches now. Only some glitches in the documentation are missing now, unless you have further feedback.

And does anybody have a clue what causes the failing tests on Travis?

webmozart · 2013-03-16T10:58:08Z

@schmittjoh What's the way to go to ignore the leftover Scrutinizer errors?

schmittjoh · 2013-03-16T11:21:28Z

You can use the delete button on the comment (top right), then they are
ignored for good.

On Sat, Mar 16, 2013 at 11:58 AM, Bernhard Schussek <
notifications@github.com> wrote:

@schmittjoh https://github.com/schmittjoh What's the way to go to
ignore the leftover Scrutinizer errors?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/7386#issuecomment-15002952
.

webmozart · 2013-03-16T12:12:33Z

Apparently I cannot when I'm logged in. Do I need admin rights on symfony/symfony?

webmozart · 2013-03-16T12:20:04Z

Forget it, it works now. Sorry.

shoomyth · 2013-03-16T12:23:55Z

Oh, ResourceBundle? I think this will confuse developers when use with symfony's bundle system.

webmozart · 2013-03-17T12:17:24Z

There is one major stumble stone left before this PR can be merged. We want to load only symfony/icu versions that are compatible with the ICU library bundled with the local PHP installation. This was achieved by:

"require": { "symfony/icu": "1.*" } in symfony/intl
"conflict": { "lib-ICU": "<4.0,>=4.1" } in symfony/icu 1.40
"conflict": { "lib-ICU": "<4.2,>=4.3" } in symfony/icu 1.42
etc.

This way

when lib-ICU is not present, the latest symfony/icu version would be installed (not needed but ok)
when lib-ICU is present, the matching symfony/icu version would be installed, since all other versions conflict with it

Apparently though composer cannot handle inverse ranges in the conflict clause (e.g. <4.0,>=4.1), so this doesn't work.

@Seldaek Do you see any solution?

webmozart · 2013-03-17T12:27:58Z

To clarify, symfony/icu is not needed when lib-ICU is not present. With the current design it would be installed anyway.

Seldaek · 2013-03-19T01:29:33Z

@bschussek IMO if things get too crazy the best option is to always enforce it's installation, and skip using it if the ICU version is unusable, or never require it but throw an exception when it's missing but ICU is avail, or solve it via documentation somehow.

…event the stub class tests from failing

…h the changed Icu component versioning

Seldaek · 2013-04-05T08:26:34Z

@bschussek I think the problem of the lock file I mentioned before is still true, that if you run update with a given ICU version, then you can't install from that lock file on another machine with different ICU, if the symfony/icu package requires that specific ICU version. In a way that's good because it means people will notice it early, and that's better than doing it in a logged warning IMO, but that's probably going to cause some support load.

webmozart · 2013-04-05T08:28:24Z

@Seldaek I see, thank you. What would people need to do in this case? Is it possible to update the lock file entry for symfony/icu by first calling

composer update symfony/icu

and then

composer install

?

Seldaek · 2013-04-05T08:38:50Z

Well, composer update --no-dev symfony/icu should install everything
from lock file + update this one, so it should work fine.

Another hack could be to require explicitly the symfony/icu that your
server needs to force devs to use the correct version, and if they can't
you can always add:

"provide": {
    "lib-ICU": "*"
}

Which will make all symfony/icu installable, but that should be done
very carefully I suppose.

What are the consequences of running symfony/icu on an incorrect ICU data?

webmozart · 2013-04-05T08:49:44Z

Well, composer update --no-dev symfony/icu should install everything
from lock file + update this one, so it should work fine.

I just tried to reproduce this but failed. When not having a vendor directory, this command also installed the newest version of other packages and overwrote the versions in my lock file.

What are the consequences of running symfony/icu on an incorrect ICU data?

Potentially fatal errors. We are using the class ResourceBundle from ext/intl (which is using the C implementation from lib-ICU) for reading the data files in symfony/icu. The data file formats are incompatible between some ICU versions.

I would prefer a solution where people don't have to worry about ICU at all. Which version of symfony/icu is installed is irrelevant for the dev, he just wants a working one.

Is it possible to exclude symfony/icu from the lock file?

mvrhov · 2013-04-05T09:24:32Z

It would be nice if developer could somehow override the icu data version. e.g the developer knows that the icu data files for 5.0 are compatible with the 4.8 library it's server has installed.

webmozart · 2013-04-05T09:28:28Z

I will do more research on which ICU versions exactly are compatible in this regard.

… symfony/icu with the ICU version installed on the system

webmozart · 2013-04-05T10:17:59Z

I've done some research and testing and reduced the number of symfony/icu branches to three:

Branch	Description	4.0	4.2	4.4	4.6	4.8	49	50
1.0.x	stub fIles	x	x	x	x	x	x	x
1.1.x	.res files format v1.*	x	x	x	x	x	x	x
1.2.x	.res files format v2.*			x	x	x	x	x

The format of the .res files was changed once with ICU 4.4 and is not BC with 4.0 and 4.2.

A problem with deployment can now only occur if:

the development machine uses ICU >= 4.4
the production machine uses ICU < 4.4

Solution: Add the following configuration to your project's composer.json:

"require": {
    "symfony/icu": "~1.1.0",
},

This problem should be solved now. I will add this information to the documentation of the Intl component.

webmozart · 2013-04-05T10:44:22Z

Another potential problem might be when

the development machine has ICU installed
the production machine does not

Again, the solution is adding the following lines to the project's composer.json:

"require": {
    "symfony/icu": "~1.0.0",
},

Seldaek · 2013-04-06T18:12:04Z

I don't know what the current situation is, but it sounds to me like this PR is just adding complexity. I never had problems with ICU and I don't know if that's because I was lucky or not. What's this PR solving exactly? And if there are only two data set formats isn't it possible to ship a single package with both formats included in their latest available versions? I understand that there is a waste of space, but is it that big?

webmozart · 2013-04-11T08:49:05Z

@Seldaek The PR solves three issues:

There were numerous problems reported by people who could not use the shipped ICU data in the Locale component due to incompatible ICU versions on their systems. These people had to rebuilt the ICU data manually, which takes time and expertise.
The Locale tests were configuration dependent and failed on some systems while they succeeded on others. We frequently had to adapt the tests without a real chance of fixing them once and for all.
The ICU data builder scripts in the Locale component had a lot of code complexity which was refactored into classes.

Considering waste of space, the 1.1.x branch of the symfony/intl component has more than 19M, while the 1.2.x branch (used by the majority of users) has only 9M due to an optimized data format in later ICU versions. Even the 9M add considerable waiting time when installing composer dependencies. Combining both branches into one package would unnecessarily increase this time even more.

webmozart · 2013-04-11T09:34:51Z

I improved the inline documentation and finished the symfony-docs PR now. Please review this PR again, it is otherwise ready to be merged.

This PR was merged into the master branch. Discussion ---------- [Intl] Refactored Locale component into two new components Icu and Intl | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #5279 | License | MIT | Doc PR | symfony/symfony-docs#2312 The Intl component is now a simple drop-in replacement layer for the C intl extension. Install it via Composer and have it available automatically if the intl extension is not available. Additionally, the component ships data from the ICU library which can be accessed through the methods: ```php use Symfony\Component\Intl\Intl; Intl::getCurrencyBundle()->... Intl::getLanguageBundle()->... Intl::getLocaleBundle()->... Intl::getRegionBundle()->... ``` If the intl extension is installed, Composer will install the ICU data for the ICU version in the intl extension. If the intl extension is not installed, Composer will use stub ICU data for the latest ICU version (see `Intl::getStubIcuVersion()`). See the [README](/bschussek/symfony/blob/intl/src/Symfony/Component/Intl/README.md) for more information. Todo: - [x] finish the Intl README file - [x] update the Icu README file - [x] update the documentation - [x] make parameter `$locale` optional (default to `\Locale::getDefault()`) in resource bundle methods - [x] remove `(Icu)?Version::compare` calls in the tests - [x] solve deployment problem when trying to install incompatible symfony/icu version listed in composer.lock Create the following branches in the [Icu component](https://github.com/symfony/Icu): - [x] 1.0.x - [x] 1.1.x - [x] 1.2.x Commits ------- 9118b4a [Locale] Removed "Stub" prefixes in Intl component b4cccfd [Intl] Removed "Stub" prefix from stub classes 60f31d1 [Intl] Improved inline documentation c2d37e6 [Intl] Improved error messages in the build scripts 1249f01 [Intl] Added scripts to test the compatibility of various versions of symfony/icu with the ICU version installed on the system 9dbafd7 [Intl] Split update-stubs.php script into two scripts to function with the changed Icu component versioning e2c11cb [Intl] Added a check for the ICU data version to IntlTestHelper to prevent the stub class tests from failing 427d24a [Intl] Outsourced bundle reader creation to Icu component 0160fd5 [Intl] Moved stub data to Icu component 1.0.x dbca3b7 [Intl] Added empty directory needed for the tests a717ce9 [Intl] Removed ICU version comparisons from the tests 5d17de5 [Intl] Fixed version comparisons in the transformation rules 470927d [Intl] Improved build scripts aceb20d [Form] Improved tests to use the IntlTestHelper class 3dd75ff [Locale] Improved tests to use the IntlTestHelper class 03b78b0 [Validator] Improved tests to use the IntlTestHelper class 9d9c389 [Intl] Simplified tests c55c4a2 [Intl] Only the StubNumberFormatterTest requires stub data 17a480b [Intl] Added IntlTestHelper class for convenience 1dcdcd3 [Locale] Fixed failing tests f6b75b9 [Intl] Changed composer.json to disallow future versions of the Icu component 080c880 [Intl] Bumped the stub version to 50.1.2 dd2d013 [Intl] Improved the bundle compilation process f47e60a [Intl] Fixed small bugs in the resource bundle transformation 467cc93 [Intl] Fixed various problems in the resource compilation process 4a5c453 [Intl] Moved the content of the README file to symfony/symfony-docs 9899de7 [Intl] Updated the README bfec58a [Intl] Fixed flawed PHPDoc 21323ba [Intl] Updated the README file 209a9cb [Validator] Adapted to latest Intl changes f2a0aec [Form] Adapted to latest Intl changes 0f6277f [Locale] Adapted to latest Intl changes 2cd1be8 [Intl] Made the $locale parameter optional in the bundle interfaces b9e9cb2 [Intl] Added autoload.php which was ignored by .gitignore 838798f [Intl] Removed method IntlTestCase::skipIfInsufficientIcuVersion() dde1d34 [Intl] Changed Intl::getIcuVersion() to return the stub version if the intl extension is not loaded 99f6f8a [Form] Fixed failing tests 5d0b849 Fixed PHPDoc b60866c [Intl] Changed Intl::getStubIcuVersion() to Intl::getIcuStubVersion() b902b6b [Locale] Added default locale 01d0ee8 [Validator] Changed component to use the Intl component 0c1fe39 [Form] Changed component to use the Intl component 5917a2e [Intl] Refactored Locale component into two new components Icu and Intl

webmozart mentioned this pull request Mar 15, 2013

Added documentation for the Intl component symfony/symfony-docs#2312

Merged

webmozart added 8 commits April 5, 2013 10:14

[Intl] Improved build scripts

470927d

[Intl] Fixed version comparisons in the transformation rules

5d17de5

[Intl] Removed ICU version comparisons from the tests

a717ce9

[Intl] Added empty directory needed for the tests

dbca3b7

[Intl] Moved stub data to Icu component 1.0.x

0160fd5

[Intl] Outsourced bundle reader creation to Icu component

427d24a

[Intl] Added a check for the ICU data version to IntlTestHelper to pr…

e2c11cb

…event the stub class tests from failing

[Intl] Split update-stubs.php script into two scripts to function wit…

9dbafd7

…h the changed Icu component versioning

[Intl] Added scripts to test the compatibility of various versions of…

1249f01

… symfony/icu with the ICU version installed on the system

[Intl] Improved error messages in the build scripts

c2d37e6

webmozart added 3 commits April 11, 2013 11:19

[Intl] Improved inline documentation

60f31d1

[Intl] Removed "Stub" prefix from stub classes

b4cccfd

[Locale] Removed "Stub" prefixes in Intl component

9118b4a

webmozart mentioned this pull request Apr 17, 2013

[Locale] Fixed the StubLocaleTest for ICU versions lower than 4.8 #6515

Merged

fabpot merged commit 9118b4a into symfony:master Apr 18, 2013

mvrhov mentioned this pull request Apr 18, 2013

[2.3] Added currency locale data, form type and validator #6566

Closed

fabpot mentioned this pull request Apr 20, 2013

[2.2] [Locale] add StubResourceBundle #4081

Closed

groucho-zz mentioned this pull request Jun 3, 2013

Stub : Form Money type / decimal type without intl installed don't work #6045

Closed

[Intl] Refactored Locale component into two new components Icu and Intl #7386

[Intl] Refactored Locale component into two new components Icu and Intl #7386

Conversation

webmozart commented Mar 15, 2013

stof commented Mar 15, 2013

stof commented Mar 15, 2013

stof commented Mar 15, 2013

webmozart commented Mar 15, 2013

stof commented Mar 15, 2013

webmozart commented Mar 15, 2013

stof commented Mar 15, 2013

webmozart commented Mar 15, 2013

webmozart commented Mar 15, 2013

stof commented Mar 15, 2013

webmozart commented Mar 15, 2013

stof commented Mar 15, 2013

bendavies commented Mar 15, 2013

stof commented Mar 15, 2013

bendavies commented Mar 15, 2013

stof commented Mar 15, 2013

Tobion commented Mar 15, 2013

webmozart commented Mar 15, 2013

asm89 commented Mar 15, 2013

stof commented Mar 15, 2013

webmozart commented Mar 16, 2013

webmozart commented Mar 16, 2013

schmittjoh commented Mar 16, 2013

webmozart commented Mar 16, 2013

webmozart commented Mar 16, 2013

shoomyth commented Mar 16, 2013

webmozart commented Mar 17, 2013

webmozart commented Mar 17, 2013

Seldaek commented Mar 19, 2013

Seldaek commented Apr 5, 2013

webmozart commented Apr 5, 2013

Seldaek commented Apr 5, 2013

webmozart commented Apr 5, 2013

mvrhov commented Apr 5, 2013

webmozart commented Apr 5, 2013

webmozart commented Apr 5, 2013

webmozart commented Apr 5, 2013

Seldaek commented Apr 6, 2013

webmozart commented Apr 11, 2013

webmozart commented Apr 11, 2013