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

Feedback appears to be addressed.

Do we need to deprecate sort()? Can we add the Collator as an optional argument to the existing method, and trigger a deprecation if it's not passed in?

Also there is a merge conflict, some out of scope changes in composer.json/lock.

@longwave you pointed 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 solution is to place the service outside the compare function sort().

Furthermore the function sort() name does not reflect its function. It is just comparing the two values, it is not sorting anything.

The merge conflict is resolved.

@longwave going to go on a limb and say this is good. The lock file seems to only show the new dependency I believe.

Rerolled

Rerolled

Unfortunately this needs another rebase, the MR looks good to me.

Rebased. What else needs to be done before the merge?

I don't think there's anything left to do here, RTBC queue has been very busy (hard to keep under 100 issues even with over 100 commits/month and many more reviews).

Committed/pushed to 11.x, thanks!

catch → closed merge request !6869

This blows up sites that don't have the intl extension

adam@8fa2a3a71304:data $ drush cr
PHP Fatal error:  Uncaught Error: Class "Collator" not found in /data/app/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:239

I think the intention of this code was to check the extension was loaded before initialising the $collator but I can't quite follow:

$collator = \Collator::create((!extension_loaded('intl')) ? ('en') : (\Drupal::service('language_manager')->getCurrentLanguage()->getId()));

::compare requires a Collator to be passed to it as well so we'll need to allow that the handle it being NULL?

I missed the symfony polyfill in the MR though which does look like it works

Just seen 🐛 Symfony\Polyfill\Intl\Icu\Collator::compare() is not implemented Active let's revert this and recommit with that fixed.

Reverted the commit and marked 🐛 Symfony\Polyfill\Intl\Icu\Collator::compare() is not implemented Active as duplicate. Let's fix that here and re-commit.

I think we should also open an upstream bug report against Symfony to implement the method, then we wouldn't need the checks added by that MR and could go back to the original code committed here.

Fixed the problem in MR11697 and tested it here in tugboat. Please review.

@acbramley : Test manually or automatically? Manually: "View live preview" via Tugboat next to the MR on the top of this issue page. In the tugboat PHP intl extension is not installed, at the moment. Same in Simplytest.me

Test manually or automatically

I meant in automated tests, i.e to catch the bug that caused this to be reverted

NW per the test. Also not sure why changing the API is necessary here.