IntlLocales country codes inconsistent with ISO_3166-1

[amanpilgrim]

Affected Version

cwp/cwp-recipe-cms 2.2.3 (SilverStripe 4.3)

Description

Alpha-2 country codes are defined as uppercase in the ISO_3166-1 standard, e.g. NZ for New Zealand https://www.iso.org/obp/ui/#search

Uppercase format is consistent with the previously used third-party implementation inside SilverStripe 3.x framework (Zend_Locale), and the SilverStripe 4.3 list of $locales in IntlLocales::class (lang_COUNTRY format, e.g. en_NZ).

We are performing an upgrade from SilverStripe 3.x to 4.x and our client has existing user data stored in the standard format. In order to maintain the integrity of our data, and stay consistent with the standard, we currently need to apply array_change_key_case() to the returned array after each call to IntlLocales::singleton()->getCountries().

Can IntlLocales be reworked so that calls to IntlLocales::singleton()->getCountries() return country codes in the standard format?

Steps to Reproduce

1. Call IntlLocales::singleton()->getCountries()
2. Returned array of country codes and names are in ['nz' => 'New Zealand'] format

[dnsl48]

Hi @amanpilgrim, thank you for reporting that. Yes, even though we do not declare that getCountries returns ISO-3166-1 codes, I think you are right and this should be the case.

As a workaround, you could overwrite IntlLocales::$countries in your project YAML configs so it contains upper case country codes.

I think that changing format of getCountries result is a breaking change, so we should do it in SilverStripe 5.

[dnsl48]

As a workaround, you could overwrite IntlLocales::$countries in your project YAML configs so it contains upper case country codes.

After digging a little bit deeper in the source code I can see that may potentially break some stuff. It appears IntlLocales may have to get some refactoring for this to work properly. I think array_change_key_case is more stable way to handle it for now.

[michalkleiner]

Can it be patched by adding an extension point? Or adding a callable param to the getCountries method that would get applied if valid? In that case it won't break BC and devs will be able to apply the callback during the call or via an extension without having to remember it every time it's used. Then for SS5, update the behaviour to be back what it was in SS3.

[dnsl48]

I think simply overriding with YML would be easier than that. It's already configurable. https://github.com/silverstripe/silverstripe-framework/blob/4/src/i18n/Data/Intl/IntlLocales.php#L1541

My only concern that we may have some other assumptions around about getCountries output, but it actually may already work flawlessly - I just don't want to claim that without testing. Changing its output through an extension would have same effect as with YML configs I think, whereas changing with a callback is same as using array_change_key_case?

[amanpilgrim]

IntlLocales::class states "Country codes follow ISO 3166-1" and getCountries() says "@return array Map of country code => name", so surely it is implicit that the entire class would conform to the same standard?

Thanks for the suggestions. I guess this scenario didn't crop up during the SS3 to 4 upgrade testing and evaluation period. Maybe it's an edge case, but anyone upgrading from SS3 to 4 and using stored country code data in the standard format, will encounter this issue. The same will then also apply when upgrading to SS5 or 6 with saved SS4 data.

If it's going to stay as is in SS4, should workaround notes be added to the module and/or documentation?

[dnsl48]

so surely it is implicit that the entire class would conform to the same standard?

Unfortunately, this is not the case. There is also countryFromLocale() method, which explicitly casts its result to lower case.

On the other hand, configs are flexible enough so you could extend the class, override this 2 methods and replace IntlLocales with your custom implementation in the dependency injector - https://github.com/silverstripe/silverstripe-framework/blob/4/_config/i18n.yml#L64

If it's going to stay as is in SS4, should workaround notes be added to the module and/or documentation?

Do you have some particular docs in mind. This feels to be a too specific edge case to add it to the general upgrading docs

[michalkleiner]

No, actually, it should be in the upgrade docs. It's a thing that needs a code change when migrating from 3 to 4. Or a migration task (which is what we wrote to change the case in the db fields where it was used).

[dnsl48]

It's a thing that needs a code change when migrating from 3 to 4

I cannot find API returning countries in SS3 i18n implementation though.

[michalkleiner]

Ah, sorry, just checked and I got it confused with getting the languages (not countries), which we still had to lowercase for SS4.

[michalkleiner]

So it can still be used as an indication that it's not one for one and could be documented in the upgrade guide.

[amanpilgrim]

To give some context, the site is using the previously core field type CountryDropdownField.

SS 3.x CountryDropdownField::$default_country = 'NZ';

The constructor fetches a list of country options from Zend_Locale::getTranslationList(), returning a list of countries in the 'NZ' => 'New Zealand' format. UserForms EditableCountryDropdownField used CountryDropdownField to populate the list of options.

SS 4.x CountryDropdownField has been removed from core, though it is still listed on the Field Types page https://userhelp.silverstripe.org/en/4/optional_features/forms/field-types. EditableCountryDropdownField remains an available field type of silverstripe/silverstripe-userforms when using the silverstripe/recipe-form-building recipe.

The source for EditableCountryDropdownField::create() is set within the constructor by calling i18n::getData()->getCountries(). A call to this, or the underlying private static $countries variable, returns a list of countries in the 'nz' => 'New Zealand' format, despite IntlLocales declaring Country codes follow ISO 3166-1.

The issue is two fold:

1. The core class data doesn't comply with the stated standard regarding country codes.
2. Remedial action is needed to be taken by any site with data stored in the previously implemented format when upgrading, or any site wishing to store their data in ISO 3166-1 format.

Perhaps a note could be added to the IntlLocales class metadata to this effect?

[robbieaverill]

Let's bundle #9126 into this, which would be to update the list (if possible) to include missing locales