-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
Can you add a test that was failing before your fix? |
I've added a case to the test for |
$this->context->expects($this->never()) | ||
->method('addViolation'); | ||
|
||
$this->validator->validate($existingCountry, new Country()); |
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.
You need to add some assertion, otherwise this test will be marked as fail when run (phpunit) in strict mode.
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.
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)?
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.
@stloyd $this->context->expects($this->never())
is an assertion
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.
Again forgot something about phpunit 😢
BTW: Is it really only the country data that's not merged back or is it everything? |
This issue only refers to countries as this is the concern of the RegionBundle, but it would be worth checking other data sets. |
As it happens, yes, it looks like currencies and languages would suffer from the same issue. |
I've made a similar change to the |
…using a country-specific locale
Any chance of a merge? (Or, consideration of the |
This would also help me out; it's holding back a production update from 2.2 to 2.3 |
ping @bschussek |
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. |
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
Hi. 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. |
@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. |
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? |
I don't know when 2.3.5 will be released, but my expectation is that it'll be pretty soon. |
@penfold45 fyi 2.3.5 was tagged/released this morning. |
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.