Lists with items containing non-ascii characters are not sorted correctly

Tested on 10.1.x and following is my observation

1. The issue summary is clear and explains the overall problem
2. Testing steps have been provided and was able to reproduce the issue
3. Patch #24 applies cleanly and the non-ASCII config entity sort correctly
4. Tests have been included and passes for #24
5. Did code review and no issues were identified

Marking this as RTBC, not including additional screenshots. Already provided in #27

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -226,14 +226,22 @@ public function createDuplicate() {
+      $language_id = \Drupal::service('language_manager')
...
+      $a_label = \Drupal::service('transliteration')->transliterate(
...
+      $b_label = \Drupal::service('transliteration')->transliterate(

A bit concerned about calling all these services inside the sort callback, because it feels like this is not going to be very performant in a case when there are hundreds of items or more to sort.

We could at least extract the transliteration service to a variable, and also skip transliterating the empty string if it is falsy? Is there a way of injecting at least the language ID, given that it never changes, or the transliteration service itself?

Also, do we have the same concerns about sorting in different languages as was raised in #3262017-90: Country list is not correctly sorted when it's localized with accents (e.g. German, Turkish) → ? Is transliterating the right thing to do in all cases?

@longwave : I remove transliteration and added the Collator like in 🐛 Country list is not correctly sorted when it's localized with accents (e.g. German, Turkish) Postponed .

To prevent MethodArgumentValueNotImplemented exception in Collator, language_id en is used if intl extension is not installed.

The new polyfill and intl extension have the same considerations as 🐛 Country list is not correctly sorted when it's localized with accents (e.g. German, Turkish) Postponed so this will need to wait for the decision over there first.

Reroll

The last submitted patch, 38: 2265487-38.patch, failed testing. View results →

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

Also posted to the #needs-review-queue-initative slack channel so hopefully a framework manager can take a look at that one.

Unpostpone it. 🐛 Country list is not correctly sorted when it's localized with accents (e.g. German, Turkish) Postponed is postponed because nobody wants to break the API.

Converted to MR

Can the issue summary be updated to use the standard template please.

Hiding all patches for clarity as fix is in MR now.

Will need framework manager review for the package being added.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Took a look at the MR before pinging a framework manager and seems we are updating several packages that seem unrelated to this. Can those be reverted please.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rerolled again

I can't make the call about the new package but the MR does fix the problem described in the issue summary. Moving to RTBC to put in front of the framework managers.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rerolled again

It's fine to add a new Symfony polyfill, (more a release management decision than a framework one) but there should really be a dependency evaluation in the issue summary here per https://www.drupal.org/about/core/policies/core-dependency-policies/depe... → - can be very short because we know the answers in this case.

Dependency evaluation added to issue summary

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Remarking as dependency eval has been added to summary.

I read the IS, comments and the MR. The only possible thing to consider is @longwave's point in #31 about calling services inside the sort callback "is not going to be very performant in a case when there are hundreds of items or more to sort". The transliteration service was removed from an earlier iteration of the MR but is still is getting the LanguageManager.

Leaving at RTBC.

#31 still to be addressed.
MR no longer applies in the meantime.

The service is moved to a new function

public static function sortEntities(array &$entities): bool

which calls a new compare function public static function compare(ConfigEntityInterface $a, ConfigEntityInterface $b, \Collator $collator): int.
The sort functionpublic static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) is now deprecated in all child classes.

appears to have merge conflict but the tag no-needs-review-bot kept the bot from getting it.

Rerolled

Left a review on the MR
Would it be simpler (less re-rolls etc) to get the new dependency in first in a separate issue?

@larowlan all references to 11.1 are replaced by 11.2