Skip to content

[Intl] made RegionBundle merge fallback data if using a country-specific locale #8564

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

Merged
merged 1 commit into from
Sep 11, 2013

Conversation

shieldo
Copy link
Contributor

@shieldo shieldo commented Jul 24, 2013

See #8442 and symfony/Icu#2.

Essentially, country data fetches from the Intl component do not currently work when using a locale with a country specifier, e.g. fr_FR. This change forces a merge on the data against the root language locale, thus making country data available for such locales.

@fabpot
Copy link
Member

fabpot commented Jul 24, 2013

Can you add a test that was failing before your fix?

@shieldo
Copy link
Contributor Author

shieldo commented Jul 24, 2013

I've added a case to the test for CountryValidator as that consumes the country data directly. I don't know whether that's the best place for this, but it's a test that was failing and now passes.

$this->context->expects($this->never())
->method('addViolation');

$this->validator->validate($existingCountry, new Country());
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add some assertion, otherwise this test will be marked as fail when run (phpunit) in strict mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't the same apply to all the other test cases in that class currently there (and most of the other tests against validators, for that matter)?

Copy link
Member

Choose a reason for hiding this comment

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

@stloyd $this->context->expects($this->never()) is an assertion

Copy link
Contributor

Choose a reason for hiding this comment

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

Again forgot something about phpunit 😢

@mvrhov
Copy link

mvrhov commented Jul 26, 2013

BTW: Is it really only the country data that's not merged back or is it everything?
e.g also currencies, languages,...

@shieldo
Copy link
Contributor Author

shieldo commented Jul 26, 2013

This issue only refers to countries as this is the concern of the RegionBundle, but it would be worth checking other data sets.

@shieldo
Copy link
Contributor Author

shieldo commented Jul 31, 2013

As it happens, yes, it looks like currencies and languages would suffer from the same issue.

@shieldo
Copy link
Contributor Author

shieldo commented Aug 4, 2013

I've made a similar change to the LanguageBundle and put it into the same commit - I couldn't reproduce the issue for either the LocaleBundle or the CurrencyBundle. It could be, though, that the method signature for AbstractBundle::readEntry should have the $mergeFallback parameter defaulting to true instead, as it seems to me this should probably be default behaviour.

@shieldo
Copy link
Contributor Author

shieldo commented Aug 27, 2013

Any chance of a merge? (Or, consideration of the $mergeFallback parameter as above?) This would fix a big BC break for me. Thanks!

@kriswillis
Copy link

This would also help me out; it's holding back a production update from 2.2 to 2.3

@fabpot
Copy link
Member

fabpot commented Aug 27, 2013

ping @bschussek

@penfold45
Copy link

Hi just tripped over this issue too while upgrading to 2.3. Cannot go live with this till there is a fix and really want to start playing with 2.3 please.

fabpot added a commit that referenced this pull request Sep 11, 2013
This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] made RegionBundle merge fallback data if using a country-specific locale

See #8442 and symfony/Icu#2.

Essentially, country data fetches from the Intl component do not currently work when using a locale with a country specifier, e.g. `fr_FR`.  This change forces a merge on the data against the root language locale, thus making country data available for such locales.

Commits
-------

52d8676 [Intl] made RegionBundle and LanguageBundle merge fallback data when using a country-specific locale
@fabpot fabpot merged commit 52d8676 into symfony:2.3 Sep 11, 2013
@penfold45
Copy link

Hi.
Thanks for this fix it appears to be working for us now.

However this fix his does not yet appear to have made it into 2.3.5-DEV any reason for this? also when is 2.3.5 likely to be released with this fix in place?

Thanks for all your hard work.

@shieldo
Copy link
Contributor Author

shieldo commented Sep 22, 2013

@penfold45 This fix is currently in the 2.3 branch - I'm not sure what you are checking out as 2.3.5-DEV but it should contain the change.

@penfold45
Copy link

mmm odd. Yes I can now see it in github on 2.3 Branch but not in the code I pulled. I will take a look again at the code I pulled and see what I did wrong. Sorry about that.

Any idea when 2.3.5 is going to be released? I am happy to test the code works but don't really want to go on to production on the branch?

@shieldo
Copy link
Contributor Author

shieldo commented Sep 23, 2013

I don't know when 2.3.5 will be released, but my expectation is that it'll be pretty soon.

@shieldo
Copy link
Contributor Author

shieldo commented Sep 27, 2013

@penfold45 fyi 2.3.5 was tagged/released this morning.

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

Successfully merging this pull request may close these issues.

7 participants