Skip to content

[Intl] Twig is not using bundled ICU data #15007

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

Closed
teohhanhui opened this issue Jun 16, 2015 · 15 comments
Closed

[Intl] Twig is not using bundled ICU data #15007

teohhanhui opened this issue Jun 16, 2015 · 15 comments

Comments

@teohhanhui
Copy link
Contributor

Of course this is perfectly understandable, but it creates inconsistencies in the results...

e.g.

use Symfony\Component\Intl\Intl;

Intl::getCurrencyBundle()->getCurrencySymbol('MYR', 'en_MY');
// RM

vs

{{ 100|localizedcurrency('MYR', 'en_MY') }}
{# MYR100 #}
{# twig/extensions - Twig_Extensions_Extension_Intl #}

with ICU 52 (Ubuntu 14.04 "trusty")

@stof
Copy link
Member

stof commented Jun 16, 2015

Can you run this code to see what you get ?

<?php

$formatter = NumberFormatter::create('en_MY', NumberFormatter::CURRENCY);

var_dump($formatter->formatCurrency(100, 'MYR'));

@stof
Copy link
Member

stof commented Jun 16, 2015

hmm, the Intl component ships with ICU 54.1 data. This may be the reason of the inconsistency between calls done based on the ICU data bundled inside PHP and the calls done based on the data bundled in the Intl component (because the intl extension does not give access to all ICU data). The currency might have changed in ICU between these versions.

@teohhanhui
Copy link
Contributor Author

@stof

Can you run this code to see what you get ?

formatCurrency(100, 'MYR'));

"MYR100.00"

Yes, I believe the ICU data has changed. And just as #11887 I do think that Symfony/Twig can move away from PHP's Intl component completely (again, as mentioned before it's been shown to be quite possible with https://github.com/commerceguys/intl)

@stof
Copy link
Member

stof commented Jun 16, 2015

Well, reimplementing the intl extension in PHP rather than using the C code is likely to be slower for the same task. so this would look weird. A benchmark comparing how commerceguys/intl behaves compared to ext-intl would be very important there.

and the Twig-extensions repo cannot rely on a Symfony component. Twig is totally independant of Symfony.

@jakzal jakzal added the Intl label Jul 7, 2015
@javiereguiluz
Copy link
Member

@jakzal after upgrading ICU data to 55 version (#18019), do you think there could be a way to use in Twig's localizedcurrency filter the same ICU data as in the rest of the Symfony application? Thanks!

@jakzal
Copy link
Contributor

jakzal commented Mar 8, 2016

Symfony 2.3-dev on a system with ICU 55.1:

$ php curr.php
Twig: RM100.00
CurrencyBundle::getCurrencySymbol(): RM
Intl version: 55.1
Intl stub version: 55.1
Intl data version: 55.1

Symfony 2.3-dev on a system with ICU 52.1:

$ php curr.php
Twig: MYR100.00
CurrencyBundle::getCurrencySymbol(): RM
Intl version: 52.1
Intl stub version: 55.1
Intl data version: 55.1

Symfony 2.3.36 on a system with ICU 55.1:

$ php curr.php
Twig: RM100.00
CurrencyBundle::getCurrencySymbol(): RM
Intl version: 55.1
Intl stub version: 51.2
Intl data version: 54.1

Symfony 2.3.36 on a system with ICU 52.1:

$ php curr.php
Twig: MYR100.00
CurrencyBundle::getCurrencySymbol(): RM
Intl version: 52.1
Intl stub version: 51.2
Intl data version: 54.1

@jakzal
Copy link
Contributor

jakzal commented Mar 8, 2016

Twig will always use the ICU data installed on the system to format the currency. However, getting the currency symbol alone is not exposed through the intl extension, so Symfony needs to load it from ICU data bundled with the intl component.

We'll have the same issue when using the intl extension classes directly if the ICU version installed on the system is different to the one bundled with the intl component.

\NumberFormatter::create('en_MY', \NumberFormatter::CURRENCY)->formatCurrency(100, 'MYR');
will use the system ICU data, while
Intl::getCurrencyBundle()->getCurrencySymbol('MYR', 'en_MY');
will use the ICU data bundled with the intl component.

There's not much we can do about it, apart from keeping ICU data bundled with the intl component up to date. End users should keep their system versions up to date as well. The correct version is always the latest released one.

The issue started as "Twig is not using bundled ICU data". I think we can close it as twig we won't make twig rely on Symfony.

We could potentially warn users if they have a version mismatch (something for SE's check.php?), and add a warning in the documentation (if it's not there yet).

@javiereguiluz
Copy link
Member

Closing for the reasons given by @jakzal. Thank you very much for this amazingly detailed analysis! I've opened https://github.com/symfony/symfony-standard/issues/952 to consider adding that warning message when there is an ICU version mismatch. Thanks!

@webmozart
Copy link
Contributor

@jakzal @javiereguiluz 👍

@kachkaev
Copy link

Hi guys. I got here from symfony's config.php and I'm pretty confused with this:

In most cases you should be fine, but please verify there is no inconsistencies between data provided by Symfony and the intl extension. See https://github.com/symfony/symfony/issues/15007 for an example of inconsistencies you might run into.

What should one do? What commands or scripts to run to see the difference in versions and understand what action might be required? Spent about 30 minutes on a research, but with no results.

@jakzal
Copy link
Contributor

jakzal commented Aug 9, 2016

@kachkaev I think the current message is confusing, could you please have a look at sensiolabs/SensioDistributionBundle#274 to see if it's any better? I'd appreciate some feedback.

fabpot added a commit to sensiolabs/SensioDistributionBundle that referenced this issue Aug 17, 2016
This PR was merged into the 5.0.x-dev branch.

Discussion
----------

Make the icu data version check less confusing

People [get](symfony/symfony#19453) [confused](symfony/symfony#15007 (comment)) by the current message added in #262. It only makes sense to be on the latest ICU data, as any other distribution is invalid (outdated). Therefore I suggest we keep the icu data up to date (symfony/symfony#19468 was just merged), and raise a notice if the user is on an older version.

Commits
-------

67713ce Make the icu data version check less confusing
@ahmed-sammari
Copy link

I have a problem installing symfony 3.1 in php7, nginx and ubuntu 16.04, i have this error:

intl ICU version installed on your system is outdated (55.1) and does not match the ICU data bundled with Symfony (57.1) To get the latest internationalization data upgrade the ICU system package and the intl PHP extension.

@nicolasodin
Copy link

Hi,

I have the same error @ahmed0406 ...
I don't understand ! Have you find ?

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Nov 9, 2016

It is exactly as the warning message suggests. You need to upgrade the ICU package on your system. But whether such an updated package is available depends on your OS / distro.

You might get some data mismatch if you ignore this warning. Not really a big deal for most users, I suppose.

@grillnoff
Copy link

I have a problem installing symfony 3.4 in php7.2, nginx and ubuntu 18.04, i have this error:

intl ICU version installed on your system is outdated (60.2) and does not match the ICU data bundled with Symfony (63.1)
To get the latest internationalization data upgrade the ICU system package and the intl PHP extension.

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

No branches or pull requests

9 participants