The circular dependency betweensymfony/intl andsymfony/icu looks weird to me.

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

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

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

@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 ofsymfony/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 insymfony/Icu).
The stub package has to require a particular version based on the ICU version it uses.

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

@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 branch1.48.x. Will requiring1.48.x-dev work then?

@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 the2.0,2.1 and2.2 branches are working (the.x at the end is optional, as well as thev at the beginning)

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

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

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

@stof Have a closer look ;)

@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

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.

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

@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

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

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

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

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

@stof Fixed.

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

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

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

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?

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

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:

@schmittjohhttps://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
.

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

Forget it, it works now. Sorry.

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

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?

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

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

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

@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

?

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?

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 classResourceBundle 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?

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.

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

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

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.

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",},

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?

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

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