Navigation Menu

Skip to content
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] Update the ICU data to 64.1 #30781

Merged
merged 1 commit into from Mar 31, 2019
Merged

[Intl] Update the ICU data to 64.1 #30781

merged 1 commit into from Mar 31, 2019

Conversation

jakzal
Copy link
Contributor

@jakzal jakzal commented Mar 30, 2019

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes, including the intl-data group
Fixed tickets -
License MIT
Doc PR -

http://site.icu-project.org/download/64

@jakzal jakzal force-pushed the icu-64.1 branch 2 times, most recently from 0830697 to fa95d90 Compare March 31, 2019 00:07
@jakzal jakzal changed the title WIP: [Intl] Update the ICU data to 64.1 [Intl] Update the ICU data to 64.1 Mar 31, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Mar 31, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(failures unrelated)

@jakzal
Copy link
Contributor Author

jakzal commented Mar 31, 2019

Tests on travis are failing for several reasons:

  1. vardumper tests seem to be broken
  2. deps=high tests for the form component install dev-master of intl
  3. deps=low tests for the form component install 2.8 version of intl

1 is unrelated and needs to be looked into in separation.
2 will be resolved once this PR is merged to 3.4 and master, but I guess for 3 I need to make the test less reliant on specific data as we won't make it work with 2.8.

@nicolas-grekas
Copy link
Member

  1. deps=low

can't we bump some lowest dep in a composer.json?

@jakzal
Copy link
Contributor Author

jakzal commented Mar 31, 2019

can't we bump some lowest dep in a composer.json?

We could, but it's not necessary as the failing test just relies on data too much.

I just pushed a change that's hopefully gonna fix it.

@fabpot
Copy link
Member

fabpot commented Mar 31, 2019

Thank you @jakzal.

@fabpot fabpot merged commit ae2cb6f into symfony:3.4 Mar 31, 2019
fabpot added a commit that referenced this pull request Mar 31, 2019
This PR was squashed before being merged into the 3.4 branch (closes #30781).

Discussion
----------

[Intl] Update the ICU data to 64.1

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes, including the intl-data group
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

http://site.icu-project.org/download/64

Commits
-------

ae2cb6f [Intl] Update the ICU data to 64.1
@jakzal jakzal deleted the icu-64.1 branch March 31, 2019 18:21
@nicolas-grekas
Copy link
Member

Looks like this creates merge conflicts with changes made in #30588 (eg und in jv.json)
@ro0NL we're going to need your help to ensure your contribution is not lost. E.g. why does ks.json contain mul in 3.4 but not in 4.2? Looks like the script misses blacklisting und, zxx and mul in 3.4, isn't it? I removed them from 4.2 while merging. See git diff 3.4..4.2 -- src/Symfony/Component/Intl/Resources/data/languages/

@ro0NL
Copy link
Contributor

ro0NL commented Apr 1, 2019

@nicolas-grekas #28833 was merged into 4.2 (new feature), since filtering languages could be considered a BC break: https://github.com/symfony/intl/blob/45f4510b4fa85f44ad8c588e238f21e06d602ffe/CHANGELOG.md#420

So Intl data updates are merged into lowest branch, but each branch should be re-comiled to apply these filters.

@jakzal
Copy link
Contributor Author

jakzal commented Apr 1, 2019

I'll re-generate data for 4.2 to see if anything needs updating.

fabpot added a commit that referenced this pull request Apr 1, 2019
This PR was merged into the 4.2 branch.

Discussion
----------

Re-generate icu 64.1 data

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? |no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #30781 (comment)
| License       | MIT
| Doc PR        | -

Commits
-------

6fb2c86 Re-generate icu 64.1 data
@ToonSpinISAAC
Copy link

The fix here seems to be to check if locale_compose returns false, but at the time of writing, the PHP documentation doesn't say it ever returns false. Maybe there should be an extra check for the empty string there? That way you don't rely on undocumented behavior.

@jakzal
Copy link
Contributor Author

jakzal commented Apr 15, 2019

@ToonSpinISAAC regardless of what's documented, locale_compose can return false since ICU 64.1. Not sure what an empty string check would change. Unless you can add a failing test to Symfony I don't think there's anything to fix here.

@ToonSpinISAAC
Copy link

If there's something to fix here it's probably a documentation bug in PHP. If locale_compose is supposed to return false in these cases then it should be documented, but as it is now it isn't, so strictly speaking nobody can rely on that.

Having said that I don't know what the Symfony team is supposed to do about the above. The reason I'm proposing a check for an empty string is that according to the PHP documentation, the function always returns a string - I don't feel strongly enough about this to be disappointed if the check doesn't happen though. 😄

@jakzal
Copy link
Contributor Author

jakzal commented Apr 15, 2019

@ToonSpinISAAC I think it should be fixed in the php docs. Previously locale_compose returned null in scenarios it returns false now. It wasn't documented either.

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.

None yet

6 participants