-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
👍 closes #19315. |
"EUR": [ | ||
"€", | ||
"Euro", | ||
{} |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
👍 @jakzal is this still WIP or is it ready to be merged? Thanks! |
@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). |
👍 |
Thank you @jakzal. |
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
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
I think the only thing that makes sense with ICU is to always be on the latest available version.