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] Fixed support of Locale::getFallback #24157

Merged
merged 1 commit into from Sep 30, 2017

Conversation

Projects
None yet
6 participants
@lyrixx
Member

lyrixx commented Sep 11, 2017

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

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Sep 12, 2017

@lyrixx lyrixx removed the BC Break label Sep 12, 2017

if (self::$defaultFallback === $locale) {
return 'root';
}
if (function_exists('locale_parse')) {

This comment has been minimized.

@stof

stof Sep 12, 2017

Member

this looks weird to me. This function is part of ext-intl, so the polyfill will never be used when this function exists

@stof

stof Sep 12, 2017

Member

this looks weird to me. This function is part of ext-intl, so the polyfill will never be used when this function exists

This comment has been minimized.

@lyrixx

lyrixx Sep 12, 2017

Member

That's the point. did I miss something?

@lyrixx

lyrixx Sep 12, 2017

Member

That's the point. did I miss something?

This comment has been minimized.

@stof

stof Sep 12, 2017

Member

ah, I missed the fact that this is not a class from the polyfill. so fine

@stof

stof Sep 12, 2017

Member

ah, I missed the fact that this is not a class from the polyfill. so fine

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 12, 2017

Member

Tests don't appreciate at all this change, see appveyor and travis... :)

Member

nicolas-grekas commented Sep 12, 2017

Tests don't appreciate at all this change, see appveyor and travis... :)

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 13, 2017

Member

Here we go. The PR is green ;)

Member

lyrixx commented Sep 13, 2017

Here we go. The PR is green ;)

array(null, 'root'),
);
if (function_exists('locale_parse')) {

This comment has been minimized.

@javiereguiluz

javiereguiluz Sep 13, 2017

Member

Shouldn't we also add these complex locales (sl_Latn_IT, etc.) to the normal tests when locale_parse() doesn't exist? The result would be of course different ... but I think we should test that too.

@javiereguiluz

javiereguiluz Sep 13, 2017

Member

Shouldn't we also add these complex locales (sl_Latn_IT, etc.) to the normal tests when locale_parse() doesn't exist? The result would be of course different ... but I think we should test that too.

This comment has been minimized.

@lyrixx

lyrixx Sep 13, 2017

Member

I have added more tests.

@lyrixx

lyrixx Sep 13, 2017

Member

I have added more tests.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 13, 2017

Member

Note: I don't understand something. PHP seems inconsistent:

  • the parameter of locale_parse should follow the FCP47 RFC. but I can also be an ISO_3166-1
  • the return of locale_compose seems the ISO_3166-1.

edit:

Anyway, with this patch, my issue issue is solved in both case (with or without the int function)

Member

lyrixx commented Sep 13, 2017

Note: I don't understand something. PHP seems inconsistent:

  • the parameter of locale_parse should follow the FCP47 RFC. but I can also be an ISO_3166-1
  • the return of locale_compose seems the ISO_3166-1.

edit:

Anyway, with this patch, my issue issue is solved in both case (with or without the int function)

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Sep 21, 2017

Member

👍

Member

lyrixx commented Sep 21, 2017

👍

@fabpot

fabpot approved these changes Sep 30, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 30, 2017

Member

Thank you @lyrixx.

Member

fabpot commented Sep 30, 2017

Thank you @lyrixx.

@fabpot fabpot merged commit 2560552 into symfony:2.7 Sep 30, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 30, 2017

bug #24157 [Intl] Fixed support of Locale::getFallback (lyrixx)
This PR was merged into the 2.7 branch.

Discussion
----------

[Intl] Fixed support of Locale::getFallback

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

Commits
-------

2560552 [Intl] Fixed support of Locale::getFallback

This was referenced Oct 5, 2017

@lyrixx lyrixx deleted the lyrixx:local branch May 3, 2018

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