Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Intl] Rename Regions to Countries#31350
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedMay 2, 2019
The region codes But otherwise, I agree about this renaming, as this class deals with a subset of ICU regions, not with all regions, and so is a different concept. |
ro0NL commentedMay 2, 2019
You're right, i suggest to consistently ignore all "non" valid codes, as done in#28833 for languages
See alsohttp://unicode.org/reports/tr35/#Unknown_or_Invalid_Identifiers (we almost conform) Do we agree excluding data is a new feature still? |
fabpot commentedMay 6, 2019
Thank you@ro0NL. |
This PR was merged into the 4.3-dev branch.Discussion----------[Intl] Rename Regions to Countries| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes (including intl-data group)| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Because that's what the current region data is about; country codes.This makes things consistent across the board; i.e. CountryType, CountryValidatorThis allows a possible other region subset (e.g "continents") to distinct. Thus having `Countries::class` + `Continents::class` both reading from the `region` data. By then data should be compiled under different keys.Current class is master only, so now or never :)The alternative approach would be `Regions::getCountryNames [,getContinentNames, etc.]`, which is harder to scale, and should also be decided for 4.3 ideally.Commits-------49aee67 [Intl] Rename Regions to Countries
javiereguiluz commentedMay 6, 2019
I think we should revert this change. Regions are not the same as countries. There are lots ofterritorial disputes in the world which (in some cases) makes it impossible to define what a country is. Even ICU itself declares that:
Also, all the smart developers out there in all programming languages use "Region". You can also see that in all operating systems. Even when displaying it for end-users, they choose "region" over "country": |
ro0NL commentedMay 6, 2019
@javiereguiluz we use the AFAIK the current data are true "countries", as we filter upstream ICU data. As such; did you find a case where a "2 letter region code" is NOT an expected country code? if so it should be patched IMHO (exclude those), see e.g.#31365 in general i think it's valid to provide a subset of regions; thus countries only. |
javiereguiluz commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ro0NL yes, I've found lots of errors :(
There are many other mistakes. The only reasonable way to fix all this is ... reverting this PR and using "Region" instead of "Country". That's what everybody else does. Please, consider a revert 🙏 Thanks! |
ro0NL commentedMay 6, 2019
from a form perspective, not by Symfony: Is it a but then again, renaming everything to Region is also an option. But it's a bigger step for the codebase in terms of deprecation etc. And if we stick with "regions", doeshttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Intl/Data/Generator/RegionDataGenerator.php#L43 still apply? Technically we dont care anymore 🤔 |
ro0NL commentedMay 6, 2019
i think we should filter byhttps://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes which states
|
ro0NL commentedMay 6, 2019
So following ISO-3316 seems like a good compromise? |
ro0NL commentedMay 6, 2019
currently Symfony provides 255 country codes ( removing the aliases fromhttps://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes gives us 249 codes. I think we're very close already, and probably if we remove the remaining "islands" and "territories" we should be good. |
fabpot commentedMay 6, 2019
But then, it becomes our responsibility to filter regions, which is probably something we want to do in the long run. Woudn't it "better" to rename |
ro0NL commentedMay 6, 2019
want or "dont want"? We already filter "some" regions today, but now im unsure what the intend is :) "exclude invalid regions" (what's an invalid region?) vs. "exclude invalid countries" (we know that's ISO-3166). If we dont filter "regions" at all the list effectively becomeshttps://github.com/unicode-org/icu/blob/master/icu4c/source/data/region/en.txt ... so from a domain perspective i value "countries" more than "regions", thus ISO-3166. |
fabpot commentedMay 6, 2019
Sorry, I means "don't want" here :) I do understand that we need to filter some entries that are not countries (World). But filtering countries is way more complex as Javier noted. I think it's not Symfony's responsibility to make choices here. So, sticking to ISO-3166 is an option, keeping regions is another one. I must admit that I don't have any preference here but I recognize that we need to make an informed decision here. |
ro0NL commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'd vote for sticking with ISO-3166 for now, thus "countries" and filter what's not an ISO-3166 code seehttps://www.diffchecker.com/OJeLGEYU it's doable. |
ro0NL commentedMay 6, 2019
the process is also easy IMHO, whenever we apply an ICU update we should verify only ISO-3166 country codes are added |
fabpot commentedMay 6, 2019
@symfony/deciders We need your point of view here. Thank you. |
ro0NL commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedMay 6, 2019
I'm for "Country" - the examples you gave Javier relate all to some geographical configuration. But if I want to create an "Address" field, I really want a "Country" label before entering "France". I think that's what this is for most of the time. |
Tobion commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sticking to ISO-3166 seems fine and then it's technically countries. If someone wants to use a more generic term like "region" they can just change the label. But for most cases it's for adress fields as Nicolas said, and there only countries make sense because you cannot send a postcard to the EU or EZ. Or can you? Would be funny to try out. |
ro0NL commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
TLDR;
Fromhttps://en.wikipedia.org/wiki/ISO_3166-1_alpha-2#Decoding_table
After#31365 SF will provide the "officially assigned ISO 3166-1 alpha-2 codes"; those we call "Countries", hence this PR. Thus, per ISO standards; e.g. Antarctica and Hong Honk are countries. Whether we can send a postcard to some address here is vague. For Hong Kong i believe yes, for Antarctica i believe not (yet there's a post office 😂https://theplanetd.com/a-post-office-in-antarctica-you-betcha/) Im not aware of why Antarctica (a continent) is assigned an official country code, i guess it's some practical reason (as it's a continent without countries). |
…z script code (ro0NL)This PR was merged into the 4.3-dev branch.Discussion----------[Intl] Made countries ISO 3166 compliant + exclude Zzzz script code| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes (including intl-data group)| Fixed tickets |#31350 (comment),#18613| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->For consistency :)Commits-------1bb7dde [Intl] Made countries ISO 3166 compliant + exclude Zzzz script code
lyrixx commentedMay 7, 2019
Actually, Antartica is split in pieces that rattached to existing countries:https://en.wikipedia.org/wiki/Antarctica#Antarctic_territories |



Because that's what the current region data is about; country codes.
This makes things consistent across the board; i.e. CountryType, CountryValidator
This allows a possible other region subset (e.g "continents") to distinct. Thus having
Countries::class+Continents::classboth reading from theregiondata. By then data should be compiled under different keys.Current class is master only, so now or never :)
The alternative approach would be
Regions::getCountryNames [,getContinentNames, etc.], which is harder to scale, and should also be decided for 4.3 ideally.