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

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

Merged
merged 43 commits into from Apr 18, 2013

Conversation

Projects
None yet
@webmozart
Contributor

webmozart commented Mar 15, 2013

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

This comment has been minimized.

Member

stof commented Mar 15, 2013

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

@stof

This comment has been minimized.

Member

stof commented Mar 15, 2013

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

@stof

This comment has been minimized.

Member

stof commented Mar 15, 2013

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

@webmozart

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

@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

This comment has been minimized.

Member

stof commented Mar 15, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

@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

This comment has been minimized.

Member

stof commented Mar 15, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

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

@stof Have a closer look ;)

@stof

This comment has been minimized.

Member

stof commented Mar 15, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

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

This comment has been minimized.

Member

stof commented Mar 15, 2013

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

@bendavies

This comment has been minimized.

Contributor

bendavies commented Mar 15, 2013

@stof

This comment has been minimized.

Member

stof commented Mar 15, 2013

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

@bendavies

This comment has been minimized.

Contributor

bendavies commented Mar 15, 2013

@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

This comment has been minimized.

Member

stof commented Mar 15, 2013

@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

This comment has been minimized.

Member

Tobion commented Mar 15, 2013

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

@webmozart

This comment has been minimized.

Contributor

webmozart commented Mar 15, 2013

@stof Fixed.

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

@asm89

This comment has been minimized.

Contributor

asm89 commented Mar 15, 2013

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

@stof

This comment has been minimized.

Member

stof commented Mar 15, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Mar 16, 2013

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

This comment has been minimized.

Contributor

webmozart commented Mar 16, 2013

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

@schmittjoh

This comment has been minimized.

Contributor

schmittjoh commented Mar 16, 2013

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

This comment has been minimized.

Contributor

webmozart commented Mar 16, 2013

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

@webmozart

This comment has been minimized.

Contributor

webmozart commented Mar 16, 2013

Forget it, it works now. Sorry.

@shoomyth

This comment has been minimized.

shoomyth commented Mar 16, 2013

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

@webmozart

This comment has been minimized.

Contributor

webmozart commented Mar 17, 2013

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

This comment has been minimized.

Contributor

webmozart commented Mar 17, 2013

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

@Seldaek

This comment has been minimized.

Member

Seldaek commented Mar 19, 2013

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

@Seldaek

This comment has been minimized.

Member

Seldaek commented Apr 5, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Apr 5, 2013

@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

This comment has been minimized.

Member

Seldaek commented Apr 5, 2013

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

This comment has been minimized.

Contributor

webmozart commented Apr 5, 2013

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

This comment has been minimized.

Contributor

mvrhov commented Apr 5, 2013

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

This comment has been minimized.

Contributor

webmozart commented Apr 5, 2013

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

[Intl] Added scripts to test the compatibility of various versions of…
… symfony/icu with the ICU version installed on the system
@webmozart

This comment has been minimized.

Contributor

webmozart commented Apr 5, 2013

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

This comment has been minimized.

Contributor

webmozart commented Apr 5, 2013

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

This comment has been minimized.

Member

Seldaek commented Apr 6, 2013

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

This comment has been minimized.

Contributor

webmozart commented Apr 11, 2013

@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

This comment has been minimized.

Contributor

webmozart commented Apr 11, 2013

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

fabpot added a commit that referenced this pull request Apr 18, 2013

merged branch bschussek/intl (PR #7386)
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

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

1 check failed

default Scrutinizer: 4 Comments, 0 Changed Files — Travis: Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment