Country list is not correctly sorted when it's localized with accents (e.g. German, Turkish)

The proposal looks correct to me, thank you for working on this.

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -26,9 +43,18 @@ class CountryManager implements CountryManagerInterface {
+    $this->transliteration = $transliteration;
+    $this->languageManager = $language_manager;

We need to add backward compatibility here in case someone has extended the class and calls this constructor without the arguments.

It would also be good to add a test to prove that the countries are sorted as expected, if possible.

A few review points, nothing big just some questions/notes:

1. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -26,9 +45,30 @@ class CountryManager implements CountryManagerInterface {
   +  public function __construct(
   +    ModuleHandlerInterface $module_handler,
   +    TransliterationInterface $transliteration = NULL,
   +    LanguageManagerInterface $language_manager = NULL) {
   

   Usually we just declare constructors on one line instead of breaking them like this.

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -300,10 +340,6 @@ class CountryManager implements CountryManagerInterface {
   -    natcasesort($countries);
   

   As this is a public method is it worth keeping this natcasesort anyway, so callers that use the method directly still get the previous sort order?

3. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -319,10 +355,29 @@ class CountryManager implements CountryManagerInterface {
   +    $language_id = $this->languageManager
   +      ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
   

   This will be called every time a sort comparison is done, is there a way to cache this outside of the compare function?

4. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -319,10 +355,29 @@ class CountryManager implements CountryManagerInterface {
   +    if ($a == $b) {
   +      return 0;
   +    }
   +    return ($a < $b) ? -1 : 1;
   

   Here you can use the spaceship operator:

   return $a <=> $b;
   

Thank you, this all looks good to me now.

@sleitner, yes this needs a patch for 10.0.x. The current patch applies to D10 so I started a test on D10.

@sleitner, thanks for fixing this!

I came back and had a look at the patch.

The patch has a deprecation, adding tag for a change record.

1. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -26,9 +52,27 @@ class CountryManager implements CountryManagerInterface {
   +    if (is_null($transliteration)) {
   

   These if blocks can be a bit simpler and could follow existing examples in core. Such as https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/file....

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -26,9 +52,27 @@ class CountryManager implements CountryManagerInterface {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $transliteration argument is deprecated in drupal:9.4.0 and it will be required in drupal:10.0.0.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $language_manager argument is deprecated in drupal:9.4.0 and it will be required in drupal:10.0.0.', E_USER_DEPRECATED);
   

   According to the deprecation policy these need a reference to the change record. See @trigger_error() format

3. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,68 @@
   +      // cSpell:disable
   +      'Zimbabwe is in front of Åland Islands, wrong alphabetical order');
   +    // cSpell:enable
   

   The indentation for the // cSpell:disable and it can be replaces with // cSpell:disable-next-line , and therefore dropping the second 'cspell' line.

I can review this. Can you provide a tests-only patch and exactly where you are seeing this issue?

Tested out

The summary is clear and accurate.
Verified the issue existed (see before_patch)
Verified the issue was fixed with the patch (see after_patch)
The tests-only patch failed showing the test case is valid.
And the change record is attached

Looks good!

This is a bit borderline for whether it should land in 9.5.x or not (bug fix, but also a new deprecation in a service), however we definitely shouldn't be deprecating in 9.5.x for removal in 10.0.x unless it's a module being removed or other specific circumstances. Given the bug itself is quite minor, let's update to deprecate in 10.1.0 for removal in 11.0.0 (discussed this with @quietone and @xjm in slack).

Also, this has the 'Needs change record' tag. That needs to be done as well, before this is RTBC.

I have to agree with @sleitner on that.

Can try and update for the deprecation today but not sure what's needed for the change record.

I perhaps should have said "not major" instead of "minor". It is definitely a usability bug -- sorry for implying otherwise. :) My reasoning for "normal" priority instead of "major" is that adding a new language is a rare task rather than a daily one. (Also I think you can find the language by clicking on or tabbing to the select box, and typing the first letter of the localized language name? That's what I always do for my own country in fields that list all countries.)

TLDR, it doesn't have implications for security, data integrity, backward compatibility, the upgrade path, etc., so it's not critical to backport. (Note that all minor-only issues will be 10.1.x-only in about a month anyway.) Thanks!

Added a patch for D9 with changes as mentioned in comment #33. Still need works for other things.

I can take a crack at the change record but would would it entail exactly?

Instructions for writing a change record (CR) draft: https://www.drupal.org/community/contributor-guide/task/write-a-change-r...

Sorry got distracted and forgot about that change notice. Took a look and it makes sense to me. The before and after really helped. Moving back to RTBC

As we mentioned before, this issue needs to be targeted for 10.1.x, not 9.5.x. This release management decision was posted in #33. Now that 9.5.0-beta1 and 10.0.0-beta1 have been released, this is actually true of all issues that require changes like new APIs that require a minor release.

It may be that the "Drupal 9" patch in #40 will actually work for 10.1.x. I will queue a test.

Thanks everyone!

1. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -319,10 +361,26 @@ public function getList() {
   +      $this->languageId = $this->languageManager
   +        ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
   +      // Sort the list.
   +      uasort($this->countries, [$this, 'compareTransliterated']);
   

   Rather than have a new languageId property I think we should use an anonymous function here and then languageId can be a local variable. Less state on the object property.

   Also this makes me realise that $this->countries is going to depend on the language ID so we need to cache by that. So $this->countries is going to need to change to $this->countries[$languageId] and we're going to need to change what we pass to the alter as well.

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -319,10 +361,26 @@ public function getList() {
   +        ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
   

   Why type content - I would have thought this is about the UI language. Well I guess it is going to depend on where it is called from. Maybe we're going to need to allow the user to pass this in and have a sensible default.

Confirmed the tests fail without the fix.
They pass with the fix.

To me the fix matches the issue summary.

When I set my default language to german
Go to de/admin/config/regional/settings I see that Österreich appears to the correct spot. Updating issue summary also.

Change record is added for deprecation call

+1 from me.

Thanks @sleitner and @smustgrave. It sounds like the change in #51 needs test coverage.

Since it also requires moving away from the normal best practice of only having a string literal as the first parameter of t(), I think we should have a translation subsystem maintainer signoff on that in particular (and probably the whole implementation overall). Assigning to Gábor for that.

Fixing credit attribution.

Forgot this tag for test coverage for the change required in #51.

Thanks for working on this! I agree its a bug definitely worthy of resolving!

Also thanks for tagging me (subsystem maintainer). I think the implementation to sort it using the transliterated version probably makes sense. At least I don't know a better solution either :D

For removing the t() on the country names, it seems to be for "cosmetic reasons", given that the additional language code argument is added and it would look odd to repeat it in all t()s. However, if we are to remove the t()s from all the country names, Translation template extractor will not find the source country name list and so Drupal 10.1 onwards would not have a country list as part of its translatable strings on localize.drupal.org, etc. While we could hardwire the country list in Translation template extractor to overcome this, I don't think that is quite future proof as it would need to be maintained to be in sync and verioned alongside Drupal. I think that is a comparatively large undertaking as opposed to updating the already repetitive t() calls to also repeat the prepared array argument (and update the existing format checking test to make sure it is repeated as such for all country names). That would keep country names sourced from the source code rather than require a potentially fragile workaround.

Seems the changes from 55 + 56 have been addressed

Reviewed the change record and seems straight forward.

Think this would be a good one to finally get in.

Thanks for carrying this forward. I reviewed again and found mostly small stuff, but one potentially serious bug (see point 6 below).

1. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,330 @@ class CountryManager implements CountryManagerInterface {
   +    if (!$transliteration) {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $transliteration argument is deprecated in drupal:10.1.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3280207', E_USER_DEPRECATED);
   +      $this->transliteration = \Drupal::service('transliteration');
   +    }
   +    else {
   +      $this->transliteration = $transliteration;
   +    }
   +    if (!$language_manager) {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $language_manager argument is deprecated in drupal:10.1.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3280207', E_USER_DEPRECATED);
   +      $this->languageManager = \Drupal::service('language_manager');
   +    }
   +    else {
   +      $this->languageManager = $language_manager;
   +    }
   

   This code can be simplified. We can have

   $this->languageManager = $language_manager;
   $this->transliteration = $transliteration;
   if (!$transliteration) {
     @trigger_error('Calling ' . __METHOD__ . ' without the $transliteration argument is deprecated in drupal:10.1.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3280207', E_USER_DEPRECATED);
     $this->transliteration = \Drupal::service('transliteration');
   }
     if (!$language_manager) {
     @trigger_error('Calling ' . __METHOD__ . ' without the $language_manager argument is deprecated in drupal:10.1.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3280207', E_USER_DEPRECATED);
     $this->languageManager = \Drupal::service('language_manager');
   }
   

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,330 @@ class CountryManager implements CountryManagerInterface {
   +   * @param string|null $languageId
   +   *   (optional) The language code for translation.
   +   *
   ...
   +    // Translate country names.
   +    $translateOptions = ($languageId === NULL) ? [] : ['langcode' => $languageId];
   

   The inline comment here does not seem to be describing what the next line is actually doing. Can we make it actually describe the purpose of setting the translation options?

   Also, I think the variables should be $language_id and $translation_options under current policy. The rest of the local variables in this class use snake case, and we currently don't allow mixing snake and camel case in the same file. #2678238: Remove prohibition on mixing snake_case and camelCase at least for constructors

3. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,330 @@ class CountryManager implements CountryManagerInterface {
   +  public static function getStandardList($languageId = NULL) {
   

   The new parameter can have a nullable string typehint: ?string

4. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,330 @@ class CountryManager implements CountryManagerInterface {
        // cSpell:enable
   -
   

   I think this line removal is out of scope?

5. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -310,19 +352,33 @@ public static function getStandardList() {
   +   * @param string $languageType
   ...
   +  public function getList($languageType = LanguageInterface::TYPE_INTERFACE) {
   

   Same issue here about snake_case vs. camelCase.

6. +++ b/core/scripts/update-countries.sh
   @@ -103,7 +103,7 @@ function t($string) {
   +  $out .= '      ' . var_export($code, TRUE) . ' => t(' . $name . ', [], $translateOptions),' . "\n";
   

   Unless I'm mistaken, $translateOptions is not defined in this scope yet, so this code seems broken. Is there test coverage for this? Has it been manually tested?

7. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,94 @@
   + * Test Country Manager functionality.
   

   Nit: Tests.

8. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,94 @@
   + * @group CountryManager
   

   Should it also have @covers?

9. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,94 @@
   +  protected $adminUser;
   

   This could be typehinted.

10. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
    @@ -0,0 +1,94 @@
    +   * Tests translate countries.
    

    This isn't a sentence. Maybe "Tests the translation of country names"?

11. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
    @@ -0,0 +1,94 @@
    +      'Espagne country name does not exist in French list');
    ...
    +      'Spain country name does not exist in English list');
    ...
    +      'Zimbabwe is in front of Åland Islands, wrong alphabetical order');
    

    Our best practice is to omit assertion messages. The last one about Zimbabwe could be moved to an inline comment above the assertion. The first two messages could be omitted since they are self-explanatory from the code.

Finally, I think the change record should be about the whole change (i.e., "Country names are now sorted by the transliterated translation of the country name, rather than the English name") and the constructor parameter additions are incidental changes that we can mention.

Thanks!

Looking at the points #62 and the points have been addressed and the comments in #64 help answer why some remain.

Only one I'll lean on xjm about is

?string would allow NULL as parameter, but not no parameter.

@sleitner, thanks for updating the patch. The duplicated comments in #63 and #64 caught me by surprise.

1. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,328 @@ class CountryManager implements CountryManagerInterface {
       * An array of country code => country name pairs.
   

   The structure of the array has changed and this is no longer true.

   The description should explain that this is now an array of country code, country name pairs keyed by language code.

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,328 @@ class CountryManager implements CountryManagerInterface {
       * Get an array of all two-letter country code => country name pairs.
   

   This also needs to be updated.

3. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -16,291 +19,328 @@ class CountryManager implements CountryManagerInterface {
   -
   

   Unnecessary change

4. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -310,19 +350,33 @@ public static function getStandardList() {
   +   *   (optional) The language type; e.g., the interface or the content
   

   I'd rather we didn't use the abbreviation, e.q. There is an issue to remove them #2637336: Replace i.e. and e.g. with English words in /core/includes and /core/misc directory

5. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,91 @@
   +    // cSpell:disable-next-line
   </li>

   This can be removed. The one before 'Spain'.

Thanks for the updates to the change record. I think that the intention of the last paragraph in #62 has not been met, that is the focus should be on what the change means to the user/developer. Having the changes to code does need to say we just a need a bit to say when this change will be seen and when a developer needs to change code. At lest, that is what I think.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Same issue with the bot as before, excluding this issue from the list

From what I can see all the points have been addressed by @sleitner so lets try this again.

Committed and pushed to 10.1.x, thanks! Also published the change record.

Added follow-up.

I have to say that this commit is quite disruptive to any modules that use, enhance, or decorate the country_manager service - this is most certainly not a "minor" change. Specifically it changes the API of the service in a way that's not backwards compatible.

In addition to the break described in #3344424: Add tests for hook_countries_alter(), this patch neglected to change CountryManagerInterface.

The core country_manager service is provided by the CountryManager class.

CountryManager implements CountryManagerInterface.

This patch changes the signature of one of the implemented methods without actually modifying the CountryManagerInterface. This is wrong, so I've reopened this issue.

Additionally, if CountryManager::getStandardList() is to be a primary method on the service, it should be described in the interface (it's not currently).

The change record is also not very good, because it doesn't mention the service or the class that is being modified, and refers to the class as a "form", which it is not. I have done a quick edit of that change record so that searches can more easily find this change to the country_manager service, but the change record should be properly reviewed to ensure consistency with the changes here.

May I ask why the order of the countries in English changed? What is the reason for this? If the order has to change, that should be mentioned in the change record, and any countries that have been moved in the list should be called out so we don't have to compare the old list of 258 countries with the new list of 258 countries to try to figure out what has changed.

And why are we involving transliteration here? The sorting of countries in any specific language does not require transliteration to a different character set - the sorting should be done in that language's character set.

Rolled this back for now, to handle the change of values in the hook and the concerns raised in #80.

Thank you! Makes sense to incorporate test and fix from #3344424: Add tests for hook_countries_alter() and close it

The order did not change, only the parameter $translation_options was added to each line.

While the order of the countries in the array declaration inside getStandardList() seems to be the same, that's not important, because the declaration order is prior to the sort.

What is important is that the order of the countries returned by getList() DID change, because it is sorted differently after the patch. And that is, of course, one of the main things the patch was intended to do! I know this because my test cases broke, and that led me to this issue.

You can see this change in order by running the following script before and after the patch:
drush php-eval '$array = \Drupal::service("country_manager")->getList(); print_r(array_map(function ($country) { return (string) $country; }, $array));'

Specifically, look at [AX] => Åland Islands, which shows up at the bottom of the list (element 257, numbered 0-257) before the patch and shows up as number two in the list (element 1, numbered 0-257) after the patch. There seem to be other changes as well, but as I said scanning through 258 countries to see exactly what changed is a pain.

When transliteration is done, Å -> A, so Åland Islands gets sorted and put somewhere in the middle of all the countries that begin with "A". But this is not correct. "Å" is a distinct letter, so all the countries that begin with "Å" should be grouped together, and not interleaved with all the countries that begin with "A". I would also expect all these "Å" countries to appear in the list BEFORE all the "A" countries, as that is the accepted way (in English) to alphabetize words that begin with numerals, special characters, etc.

Now this is not that apparent with "Å" countries in English, because there is only one of them. But the same situation happens with other languages. To use your example, "Ö" in German is a different letter than "O", so they should be listed in separate groups and not interleaved together like they would be with transliteration involved.

I don't think transliteration is the right approach, because transliterating loses the information that these non-native characters are distinct characters.

There are many authorities on how to alphabetize - in English for example I would refer to the Chicago Manual of Style. What this patch seems to do is to invent a brand-new "Drupal" style of alphabetizing, which seems wrong. We are already inventing and maintaining our own list of countries that does not 100% agree with any standard, why are we working so hard to invent a new solution to alphabetizing as well?

To implement this new feature of being able to obtain the country list in a specific language, I would suggest that, instead of adding an argument to getList(), a NEW method should be declared for getting the list. Something like CountryManagerInterface::getLocalizedList($language) where $language is a language ID. This would still involve injecting the language_manager service, though. Off the top of my head I don't know if that is an allowable change for a minor-point release, but I do know if this is done it will make my D10.0 module incompatible with D10.1.

And to bring up another point, hooks are not how we alter services in Drupal. Symfony has perfectly good mechanisms for altering services, which we document in Drupal and use elsewhere. This hook_countries_alter() hook seems to be an artifact of the early porting days when this code was initially moved from D7 procedural code, and it was never touched after that even after everyone became more familiar with services and how we wanted to implement them in Drupal. There is no need to define a hook in order to alter this service. And indeed, since the altering function is not described in CountryManagerInterface (see #2487351: Not inheriting parent documentation for CountryManager::getList()) I will argue that this alter hook is not part of the public country_manager service API and should be deprecated here and dropped for D11. In any case, the hook is not part of the current API and if I override the service in the standard manner I will lose the hook functionality - having two standard ways to alter services is prone to error.

I think I agree that hook_countries_alter() should be deprecated, but let's do that in a separate issue, rather than expand the scope of this one.

@sleitner Thank you for working on this. At present Drupal doesn't require the intl extension and I don't think we can add that requirement here. It might be possible to add a dependency on symfony/polyfill-intl-icu instead, which looks to provide the Collator class for environments that do not have the intl extension - does your code work with that? But this is still quite a big change and will require wider review.

1. +++ b/composer.json
   @@ -12,7 +12,8 @@
   +        "symfony/polyfill-intl-icu": "^1.27"
   

   This doesn't need to be added here, only in core/composer.json.

2. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,76 @@
   +    $this->assertEquals('ES', array_search(
   +    // cSpell:disable-next-line
   +      'Espagne',
   +      $countryList));
   

   This is hard to read, this should probably just be one line.

3. +++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,76 @@
   +    $this->assertEquals('ES', array_search(
   +      'Spain',
   +      $countryList));
   

   Same

4. +++ b/core/tests/Drupal/KernelTests/Core/Locale/CountryManagerTest.php
   @@ -0,0 +1,47 @@
   +    $this->assertTrue(array_search('AX', $countryIds)
   +      < array_search('ZW', $countryIds));
   

   Can use assertLessThan here.

@TR does this latest patch fix all the concerns you had with the previous version?

This also needs a release note given that we are adding a new dependency and that Intl extends this functionality further.

I also wonder if we should add the Intl extension to the suggest section of core/composer.json?

Tagging for framework manager review, as we are adding a new soft dependency on the intl extension and we need an opinion as to whether this is the right direction.

Also tagging for release manager review; I don't see any issue with the new dependencies but would like a second opinion there too.

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -325,4 +342,28 @@ public function getList() {
+   */
+  public function getLocalizedList($language_id) {
+    // Populate the country list if it is not already populated.
+    if (!isset($this->localizedCountries[$language_id])) {
+      $this->localizedCountries[$language_id] = static::getStandardList($language_id);
+
+      // Sorts the country array depending on language rules.
+      $collator = \Collator::create((!extension_loaded('intl')) ? ('en') : ($language_id));
+      $collator->asort($this->localizedCountries[$language_id]);
+    }
+
+    return $this->localizedCountries[$language_id];
+  }

Do we need a hook_requirements() somewhere encouraging people to install the intl extension (maybe in locale module?).

I was also wondering if we could just skip the sorting completely if the locale is 'en' and not add the symfony polyfill in that case, but it's probably cleaner to have less code paths.

Don't really see a problem with the soft dependency, and also can't think of an alternative.

Untagging for release manager review since both me and longwave think this is OK in principle.

One more thought - should locale module decorate the country manager and handle the sorting? Probably not worth it though for the complexity that would add.

1. +++ b/core/composer.json
   @@ -112,6 +113,7 @@
   +        "ext-intl": "Needed to extend symfony/polyfill-intl-icu capability with the sorting of non-english languages.",
   

   "English" should be capitalised.

2. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -18,9 +18,18 @@ class CountryManager implements CountryManagerInterface {
   +  protected $localizedCountries = [];
   

   We can use typehints for new properties.

3. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -34,270 +43,278 @@ public function __construct(ModuleHandlerInterface $module_handler) {
   +  public static function getStandardList($language_id = NULL) {
   

   What if someone has already extended this class and replaced this method? https://3v4l.org/0WTuR

4. +++ b/core/lib/Drupal/Core/Locale/CountryManager.php
   @@ -325,4 +342,28 @@ public function getList() {
   +  public function getLocalizedList($language_id) {
   

   We can use a typed argument and a return type here.

Going to go ahead and mark this to keep it moving forward. @catch as a release manager did a review #109 but he's also a framework manager. Will leave the tag if we think a separate framework manager is needed but think we should be good.

1. Rerolled due to composer changes.
2. Fixed the version constraint to be inline with other constraints in our composer. Did this as part of resolving the conflicts. Changed ~v1.27.0 to ^1.27 as the tilde constraint on a patch release is not what we want.
3. Improved the docs to not duplicate the between the class and interface and more detail about what is in the array

None of these changes affect the logic of the change therefore leaving at RTBC.

I think there are some tensions with the existing getList() method...

  public function getList() {
    // Populate the country list if it is not already populated.
    if (!isset($this->countries)) {
      $this->countries = static::getStandardList();
      $this->moduleHandler->alter('countries', $this->countries);
    }

    return $this->countries;
  }

It has an alter hook...

and the new method...

  /**
   * {@inheritdoc}
   */
  public function getLocalizedList(string $language_id) : array {
    // Populate the country list if it is not already populated.
    if (!isset($this->localizedCountries[$language_id])) {
      $this->localizedCountries[$language_id] = static::getCountryList($language_id);

      // Sorts the country array depending on language rules.
      $collator = \Collator::create((!extension_loaded('intl')) ? ('en') : ($language_id));
      $collator->asort($this->localizedCountries[$language_id]);
    }

    return $this->localizedCountries[$language_id];
  }

So if you alter the first one you don't get to alter the other. I don;'t think the best solution is to add another alter hook. I think the best solution would be to able to execute code in a different language contexts (there is an issue for this but I'm having trouble to find it) - so we can call getList() without having to add a hook or do anything particularly special.

+++ b/core/tests/Drupal/FunctionalTests/Core/Locale/CountryManagerTest.php
@@ -0,0 +1,72 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected $profile = 'minimal';

This is necessary so that we get translations from localise.drupal.org - but that makes the test fragile. We should be setting up a few translations in the test rather than doing this.

What is this issue posptoned on?

This came up in the Bug Smash PMNMI triage. It looks like we haven't had a response to why this issue is postponed. Is someone able to update this issue on whether it is still something that needs work?

I did some testing on 11.x and confirmed the country list is not sorted per language. Therefor, I converted the latest patch to an MR and updated the functional test so that it tests the displayed data. I also noted that the new method is not called when viewing the country list at admin/config/regional/settings.

The related issue, #3403443: Sort country names using Collator, uses PHP Colllator::sort. Maybe this should do the same.