Skip to content

Commit

Permalink
Revert to encoding query with RFC3986
Browse files Browse the repository at this point in the history
Bug fixed in league/uri-components 2.4.2 and 7.1.
  • Loading branch information
trowski committed Aug 21, 2023
1 parent f6846bc commit 5640564
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 7 deletions.
6 changes: 3 additions & 3 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
"php": ">=8.1",
"amphp/hpack": "^3",
"amphp/parser": "^1.1",
"league/uri-components": "^2.4 || ^7",
"psr/http-message": "^1 || ^2"
"league/uri-components": "^2.4.2 | ^7.1",
"psr/http-message": "^1 | ^2"
},
"require-dev": {
"phpunit/phpunit": "^9",
"amphp/php-cs-fixer-config": "^2",
"league/uri": "^6.8 || ^7",
"league/uri": "^6.8 | ^7.1",
"psalm/phar": "^5.4"
},
"scripts": {
Expand Down
2 changes: 1 addition & 1 deletion src/HttpRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ private function updateUriWithQuery(array $query): void
\array_push($pairs, ...\array_map(static fn ($value) => [$key, $value], $values));
}

$this->uri = $this->uri->withQuery(QueryString::build($pairs, '&', \PHP_QUERY_RFC1738) ?? '');
$this->uri = $this->uri->withQuery(QueryString::build($pairs, '&', \PHP_QUERY_RFC3986) ?? '');
$this->queryMap = $query;
$this->queryPairs = $pairs;
}
Expand Down
17 changes: 14 additions & 3 deletions test/HttpRequestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,15 @@ public function testQueryWithEmptyValues(): void

public function testQueryWithEncodedChars(): void
{
$query = 'key%5B1%5D=1%201&key%5B2%5D=2%261&key%5B3%5D=3%5B1%5D';
$query = 'key%5B1%5D=1+1&key%5B2%5D=2%261&key%5B3%5D=3%5B1%5D';
$request = $this->createTestRequest($query);
self::assertSame('1 1', $request->getQueryParameter('key[1]'));
self::assertSame('2&1', $request->getQueryParameter('key[2]'));
self::assertSame('3[1]', $request->getQueryParameter('key[3]'));

$request->setQueryParameter('key[3]', '3[2]');
$query = \str_replace('3%5B1%5D', '3%5B2%5D', $query);
$query = \str_replace('%20', '+', $query);
$query = \str_replace('+', '%20', $query);

This comment has been minimized.

Copy link
@kelunik

kelunik Aug 22, 2023

Member

Instead of using str_replace, I'd rather state the full expected value explicitly here.

self::assertSame($query, $request->getUri()->getQuery());
}

Expand Down Expand Up @@ -184,7 +184,7 @@ public function testRemoveQuery(): void

public function testQueryWithEmptyPairs(): void
{
$query = '&&&=to&&key=value&empty&encoded=%2B+%2B';
$query = '&&&=to&&key=value&empty&encoded=%2B%20%2B';
$request = $this->createTestRequest($query);

self::assertSame([
Expand Down Expand Up @@ -233,6 +233,17 @@ public function testSettingNonStringValues(): void
$request->setQueryParameter('key4', [true]);
}

public function testBothRfc1738AndRfc3986EncodingAccepted(): void
{
$rfc1738 = 'key=1+1';
$rfc3986 = 'key=1%201';

self::assertSame(
$this->createTestRequest($rfc1738)->getQueryParameter('key'),
$this->createTestRequest($rfc3986)->getQueryParameter('key'),
);

This comment has been minimized.

Copy link
@kelunik

kelunik Aug 22, 2023

Member

These should be two asserts both asserting the value to be equal to '1 1', otherwise this is also green for both values being null for example.

}

public function testIsIdempotent(): void
{
self::assertTrue($this->createTestRequest('', 'HEAD')->isIdempotent());
Expand Down

0 comments on commit 5640564

Please sign in to comment.