Skip to content

Fix Query::sort ordering of non-ASCII names - #233

Merged
nyamsprod merged 1 commit into
thephpleague:masterfrom
gggg5151:fix-query-sort-code-units
Nov 2, 2026
Merged

nyamsprod merged 1 commit into
thephpleague:masterfrom
gggg5151:fix-query-sort-code-units

Conversation

@gggg5151

@gggg5151 gggg5151 commented Nov 1, 2026

Copy link
Copy Markdown
Contributor
Information Description
Package league/uri-components
Version master (313ed46)
PHP version 8.4 (the two affected test files also pass on 8.1)
OS Platform Linux

Summary

Query::sort and URLSearchParams::sort do not always follow the order required by the
URL Standard, which compares names by UTF-16 code units.

The current code turns each name into decimal code points and compares them with strcmp,
so it compares digits as text. For example, a becomes 97 and あ becomes 12354, and あ is sorted first.

$params = new URLSearchParams('あ=1&い=2&a=3');
$params->sort();
echo $params->toString();

The order of the names is あ, い, a. The expected order, which Node's URLSearchParams also returns, is a, あ, い.

The existing ffi&🌈 test passes only by coincidence, and the WPT case é&e�&é was not in the test list.

Changes

  • Compare names as UTF-16BE strings. strcmp compares bytes, so this matches code unit order.
  • Keep the existing ValueError for invalid UTF-8 with mb_check_encoding, and add a test for it.
  • Add the missing WPT case and a test that mixes ASCII and non-ASCII names.
  • Update the URLSearchParams::sort docblock and the changelog.

The order changes for some queries with non-ASCII names. That is the intended result of this fix.

Testing

The full test suite and PHPStan pass on PHP 8.4. php-cs-fixer reports only interfaces/HostRecord.php, which is also reported on master and is not changed here.

I used Claude Code for this investigation and for drafting the change. I reviewed the change and can answer questions about it.

🤖 Generated with Claude Code

The URL Standard compares names by UTF-16 code units. Compare UTF-16BE strings with strcmp instead of decimal code point strings.
@nyamsprod
nyamsprod merged commit 8a2158c into thephpleague:master Nov 2, 2026
11 checks passed
@nyamsprod

Copy link
Copy Markdown
Member

@gggg5151 the PR is approved and accepted but I may change its implementation. Thanks for the fix.

@gggg5151

gggg5151 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Sounds good. Thanks for the quick review!

@nyamsprod

nyamsprod commented Nov 2, 2026 •

Copy link
Copy Markdown
Member

The rewritten implementation using your improvements.

    public function sort(): self
    {
        if ([] === $this->pairs) {
            return $this;
        }

        $toUtf16 = static fn (string $str): string => mb_check_encoding($str, 'UTF-8')
            ? mb_convert_encoding($str, 'UTF-16BE', 'UTF-8')
            : throw new ValueError('The parameter name (hex: '.bin2hex($str).') must contain valid UTF-8 characters.')
        ;
        $parameters = array_map(static fn (array $pair): array => ['pair' => $pair, 'name' => $toUtf16($pair[0])], $this->pairs);
        usort($parameters, static fn (array $a, array $b): int => strcmp($a['name'], $b['name']));
        $pairs = array_column($parameters, 'pair');

        return $pairs === $this->pairs ? $this : self::fromPairs($pairs, $this->separator);
    }

@gggg5151

gggg5151 commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the rewrite, this looks cleaner! Glad I could help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants