Skip to content

Fix issue #5542 Sorting by name #5550

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

bernardo-campos
Copy link
Contributor

Related Issue: Issue #5542

Summary:

Replaces the <=> operator with PHP’s Collator class for locale-aware string comparisons in App\Sorting\SortSetOperationComparisons. Using <=> with strtolower() mishandles accented characters (e.g., "é" sorts after "z"), which is unreliable for languages with diacritics. Collator ensures accurate sorting based on the user’s locale.

Changes:

Added a new protected static method getCollator():

Inside class App\Sorting\SortSetOperationComparisons, I've added

 protected static function getCollator(): ?\Collator
 {
    $locale = user()->getLocale()->isoLocale(); // e.g., 'eu_ES', 'fr_FR'
    return class_exists(\Collator::class) ? collator_create($locale) : null;
}
  • $locale is fetched from the user’s settings via BookStack\Translation\LocaleManager.
  • class_exists(\Collator::class) checks for the intl extension, returning null if unavailable (graceful fallback).
  • The returned Collator object (or null) is used in the class’s comparison functions to handle sorting.

Impact:

  • Improves sorting for users with locales like fr_FR or eu_ES.
  • No breaking changes; users without intl fall back to existing behavior.

TL;DR: Swaps <=> for Collator to fix accented character sorting, with a fallback for missing intl.

@ssddanbrown
Copy link
Member

Thanks for offering @bernardo-campos, but I'm not too keen on us having diverging logic for minor functionality depending on available extensions. I'd also be concerned about the use of locale, since the user locale isn't assured to match the content and I don't want sort logic to change depending on user performing the action which causes changes.

The codebase already uses the https://github.com/voku/portable-ascii library via Laravel, could we maybe use the to_transliterate function of that instead? Keeping the unknown chars as a fallback instead of replacing/removing.
That looks to use the intl extension logic under the hood, with fallbacks for environments without the extension which should hopefully cover most common cases.

Also, it would be good to have some added tests to cover this scenario, similar to our existing name sort tests:

public function test_name_alphabetical_ordering()

@bernardo-campos
Copy link
Contributor Author

Ok. I see the point. Yep, using to_transliterate it works like a charm!

@bernardo-campos
Copy link
Contributor Author

bernardo-campos commented Mar 23, 2025

I’ve changed:
return strtolower($a->name) <=> strtolower($b->name);
to
return strtolower(ASCII::to_transliterate($a->name)) <=> strtolower(ASCII::to_transliterate($b->name));

I know the logic got a bit lengthy, but I realized that just using ASCII::to_transliterate alone didn’t work because it preserves uppercase and lowercase letters, causing the comparison to sort them separately (uppercase first, then lowercase). Using strtolower inside doesn’t help either, since strtolower('É') still keeps it as É.

Another idea is to use laravel helper Str::slug($somestring, ' ') maybe it is simpler and slug function is already used in several parts of the project

@ssddanbrown ssddanbrown added this to the v25.02.2 milestone Mar 24, 2025
@ssddanbrown
Copy link
Member

Thanks @bernardo-campos, although I think we'd want to specifically pass in null as the second parameter to the to_transliterate calls, since otherwise unknown non-ascii chars will be converted to ?, whereas i think it's better to retain the original utf8 values so that sorting remains stable in such cases.

Another idea is to use laravel helper Str::slug($somestring, ' ') maybe it is simpler and slug function is already used in several parts of the project

That would lead to a bit too much conversion of the original text, similar to what I mention above, which would lead to less stable/expected sorting outcomes.

ssddanbrown added a commit that referenced this pull request Apr 2, 2025
@ssddanbrown ssddanbrown merged commit abe7467 into BookStackApp:development Apr 2, 2025
1 check passed
@ssddanbrown
Copy link
Member

Thanks again @bernardo-campos,
Changes now all merged in for next patch release.

I followed this up with 1ba0d26 to address my comment above.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants