Skip to content

[Intl] Update ICU data to 57.1 #19468

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
wants to merge 2 commits into from
Closed

Conversation

jakzal
Copy link
Contributor

@jakzal jakzal commented Jul 28, 2016

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #19315
License MIT
Doc PR -

I think the only thing that makes sense with ICU is to always be on the latest available version.

@HeahDude
Copy link
Contributor

👍 closes #19315.

"EUR": [
"€",
"Euro",
{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it expected to have an empty object here alongside strings ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is generated. I'll double check if there's something wrong on our side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is generated, but this means our APIs exposing currency name will return invalid data (their contract is to return an array of strings).

If there is a bug in the original files, this should be reported to the ICU project, and we should filter out invalid values in our generator script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to be using this value. There are occurrences of this third element in previous versions of icu data (see here and here).

The original ICU file stores some additional formatting definitions here:

en_150{
    %%Parent{"en_001"}
    Currencies{
        EUR{
            "€",
            "Euro",
            {
                "¤#,##0.00",
                ",",
                ".",
            }
        }
    }
    Version{"2.1.19.76"}
}

We seem to be removing it.

@javiereguiluz
Copy link
Member

👍

@jakzal is this still WIP or is it ready to be merged? Thanks!

@jakzal jakzal changed the title [WIP][Intl] Update ICU data to 57.1 [Intl] Update ICU data to 57.1 Aug 9, 2016
@jakzal
Copy link
Contributor Author

jakzal commented Aug 9, 2016

@javiereguiluz this is indeed ready now. I was looking into updating ICU data on travis so we could run related tests, but I can do it as a separate PR (tests are not run for the current version either).

@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Aug 9, 2016

Thank you @jakzal.

fabpot added a commit that referenced this pull request Aug 9, 2016
This PR was squashed before being merged into the 2.7 branch (closes #19468).

Discussion
----------

[Intl] Update ICU data to 57.1

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19315
| License       | MIT
| Doc PR        | -

I think the only thing that makes sense with ICU is to always be on the latest available version.

Commits
-------

a48c00b [Intl] Update ICU data to 57.1
@fabpot fabpot closed this Aug 9, 2016
@jakzal jakzal deleted the icu-update-57.1 branch August 9, 2016 13:37
fabpot added a commit to sensiolabs/SensioDistributionBundle that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants