From 84b66f0f18719695d96df4820cf6cddc897bc12b Mon Sep 17 00:00:00 2001 From: ignace nyamagana butera Date: Fri, 29 Sep 2023 21:44:14 +0200 Subject: [PATCH] Bugfix/query parameters constructors encoding values (#122) bugfix inconsistencies when converting variables into query --- components/CHANGELOG.md | 6 +- components/Components/Query.php | 53 ++++-- components/Components/QueryTest.php | 32 ++-- components/Components/URLSearchParams.php | 64 ++++++- components/Components/URLSearchParamsTest.php | 179 +++++++++++++++++- components/Modifier.php | 10 +- docs/components/7.0/query.md | 10 +- docs/components/7.0/urlsearchparams.md | 8 +- 8 files changed, 315 insertions(+), 47 deletions(-) diff --git a/components/CHANGELOG.md b/components/CHANGELOG.md index d006de80..f46e524a 100644 --- a/components/CHANGELOG.md +++ b/components/CHANGELOG.md @@ -6,7 +6,8 @@ All Notable changes to `League\Uri\Components` will be documented in this file ### Added -- None +- `Query::fromVariable` +- `UrlSearchParams::fromVariable` ### Fixed @@ -14,7 +15,8 @@ All Notable changes to `League\Uri\Components` will be documented in this file ### Deprecated -- None +- `Query::fromParameters` use `Query::fromVariable` instead +- `UrlSearchParams::fromParameters` use `UrlSearchParams::fromVariable` instead ### Removed diff --git a/components/Components/Query.php b/components/Components/Query.php index ba3b644b..a6d254e4 100644 --- a/components/Components/Query.php +++ b/components/Components/Query.php @@ -22,6 +22,7 @@ use League\Uri\QueryString; use Psr\Http\Message\UriInterface as Psr7UriInterface; use Stringable; + use Traversable; use function array_column; @@ -35,7 +36,6 @@ use function http_build_query; use function implode; use function is_int; -use function iterator_to_array; use function preg_match; use function preg_quote; use function preg_replace; @@ -69,27 +69,13 @@ public static function new(Stringable|string|null $value = null): self } /** - * Returns a new instance from the result of PHP's parse_str. + * Returns a new instance from the input of http_build_query. * * @param non-empty-string $separator */ - public static function fromParameters(object|array $parameters, string $separator = '&'): self + public static function fromVariable(object|array $parameters, string $separator = '&'): self { - if ($parameters instanceof QueryInterface) { - return self::fromPairs($parameters, $separator); - } - - $parameters = match (true) { - $parameters instanceof Traversable => iterator_to_array($parameters), - default => $parameters, - }; - - $query = match ([]) { - $parameters => null, - default => http_build_query(data: $parameters, arg_separator: $separator), - }; - - return new self($query, Converter::fromRFC1738($separator)); + return new self(http_build_query(data: $parameters, arg_separator: $separator), Converter::fromRFC1738($separator)); } /** @@ -668,4 +654,35 @@ public function withoutPair(string ...$keys): QueryInterface { return $this->withoutPairByKey(...$keys); } + + /** + * DEPRECATION WARNING! This method will be removed in the next major point release. + * + * @param non-empty-string $separator + * + * @see Query::fromVariable() + * + * @codeCoverageIgnore + * Returns a new instance from the result of PHP's parse_str. + * + * @deprecated Since version 7.0.0 + */ + public static function fromParameters(object|array $parameters, string $separator = '&'): self + { + if ($parameters instanceof QueryInterface) { + return self::fromPairs($parameters, $separator); + } + + $parameters = match (true) { + $parameters instanceof Traversable => iterator_to_array($parameters), + default => $parameters, + }; + + $query = match ([]) { + $parameters => null, + default => http_build_query(data: $parameters, arg_separator: $separator), + }; + + return new self($query, Converter::fromRFC1738($separator)); + } } diff --git a/components/Components/QueryTest.php b/components/Components/QueryTest.php index e8bf6e47..aa52e89e 100644 --- a/components/Components/QueryTest.php +++ b/components/Components/QueryTest.php @@ -349,7 +349,7 @@ public static function providePairsToBeRemoved(): iterable */ public function testwithoutParam(array $origin, array $without, string $expected): void { - self::assertSame($expected, Query::fromParameters($origin)->withoutParameters(...$without)->toString()); + self::assertSame($expected, Query::fromVariable($origin)->withoutParameters(...$without)->toString()); } public static function withoutParamProvider(): array @@ -408,7 +408,7 @@ public function testWithoutParamDoesNotChangeParamsKey(): void ], ]; - $query = Query::fromParameters($data); + $query = Query::fromVariable($data); self::assertSame('foo%5B0%5D=bar&foo%5B1%5D=baz', $query->value()); self::assertTrue($query->hasParameter('foo')); @@ -437,13 +437,13 @@ public function testCreateFromParamsWithTraversable(): void ], ]; - self::assertSame($data, Query::fromParameters(new ArrayIterator($data))->parameters()); + self::assertSame([], Query::fromVariable(new ArrayIterator($data))->parameters()); } public function testCreateFromParamsWithQueryObject(): void { $query = Query::new('a=1&b=2'); - self::assertEquals($query->value(), Query::fromParameters($query)->value()); + self::assertEquals('pairs%5B0%5D%5B0%5D=a&pairs%5B0%5D%5B1%5D=1&pairs%5B1%5D%5B0%5D=b&pairs%5B1%5D%5B1%5D=2&separator=%26¶meters%5Ba%5D=1¶meters%5Bb%5D=2', Query::fromVariable($query)->value()); } public static function testWithoutNumericIndices(): void @@ -464,7 +464,7 @@ public static function testWithoutNumericIndices(): void $withIndices = 'filter%5Bfoo%5D%5B0%5D=bar&filter%5Bfoo%5D%5B1%5D=baz&filter%5Bbar%5D%5Bbar%5D=foo&filter%5Bbar%5D%5Bfoo%5D=bar'; $withoutIndices = 'filter%5Bfoo%5D%5B%5D=bar&filter%5Bfoo%5D%5B%5D=baz&filter%5Bbar%5D%5Bbar%5D=foo&filter%5Bbar%5D%5Bfoo%5D=bar'; - $query = Query::fromParameters($data); + $query = Query::fromVariable($data); self::assertSame($withIndices, $query->value()); self::assertSame($data, $query->parameters()); @@ -488,43 +488,43 @@ public function testWithoutNumericIndicesReturnsAnother(): void public function testWithoutNumericIndicesDoesNotAffectPairValue(): void { - $query = Query::fromParameters(['foo' => 'bar[3]']); + $query = Query::fromVariable(['foo' => 'bar[3]']); self::assertSame($query, $query->withoutNumericIndices()); } public function testCreateFromParamsOnEmptyParams(): void { - $query = Query::fromParameters([]); + $query = Query::fromVariable([]); self::assertSame($query, $query->withoutNumericIndices()); } public function testGetContentOnEmptyContent(): void { - self::assertNull(Query::fromParameters([])->value()); + self::assertSame('', Query::fromVariable([])->value()); } public function testGetContentOnHavingContent(): void { - self::assertSame('foo=bar', Query::fromParameters(['foo' => 'bar'])->value()); + self::assertSame('foo=bar', Query::fromVariable(['foo' => 'bar'])->value()); } public function testGetContentOnToString(): void { - self::assertSame('foo=bar', (string) Query::fromParameters(['foo' => 'bar'])); + self::assertSame('foo=bar', (string) Query::fromVariable(['foo' => 'bar'])); } public function testWithSeperatorOnHavingSeparator(): void { - $query = Query::fromParameters(['foo' => '/bar']); + $query = Query::fromVariable(['foo' => '/bar']); self::assertSame($query, $query->withSeparator('&')); } public function testWithoutNumericIndicesOnEmptyContent(): void { - $query = Query::fromParameters([]); + $query = Query::fromVariable([]); self::assertSame($query, $query->withoutNumericIndices()); } @@ -800,4 +800,12 @@ public function testItFailsToCreateFromRFCSpecificationWithEmptySeparator(): voi Query::fromRFC1738('foo=b%20ar;foo=baz', ''); /* @phpstan-ignore-line */ } + + public function testInstantiationFromURLSearchParams(): void + { + $expected = ['foo' => 'bar']; + $query = Query::fromVariable(URLSearchParams::fromVariable($expected)); + + self::assertSame('', $query->value()); + } } diff --git a/components/Components/URLSearchParams.php b/components/Components/URLSearchParams.php index f156bd80..5e062295 100644 --- a/components/Components/URLSearchParams.php +++ b/components/Components/URLSearchParams.php @@ -29,15 +29,20 @@ use Stringable; use function array_is_list; +use function array_key_exists; use function array_keys; use function array_map; use function count; use function func_get_arg; use function func_num_args; +use function get_object_vars; use function is_array; use function is_iterable; use function is_object; +use function is_scalar; +use function iterator_to_array; use function json_encode; +use function spl_object_hash; use function str_starts_with; use const JSON_PRESERVE_ZERO_FRACTION; @@ -206,11 +211,48 @@ public static function fromUri(Stringable|string $uri): self } /** - * Returns a new instance from the result of PHP's parse_str. + * Returns a new instance from the input of PHP's http_build_query. */ - public static function fromParameters(object|array $parameters): self + public static function fromVariable(object|array $parameters): self { - return new self(Query::fromParameters($parameters)); + return self::fromPairs(self::parametersToPairs($parameters)); + } + + private static function parametersToPairs(array|object $data, string|int $prefix = '', array &$recursive = []): array + { + $yieldParameters = static fn (object|array $data): array => is_array($data) ? $data : get_object_vars($data); + + $pairs = []; + foreach ($yieldParameters($data) as $name => $value) { + if (is_object($data)) { + $id = spl_object_hash($data); + if (!array_key_exists($id, $recursive)) { + $recursive[$id] = 1; + } + } + + if (is_object($value)) { + $id = spl_object_hash($value); + if (array_key_exists($id, $recursive)) { + return []; + } + + $recursive[$id] = 1; + } + + if ('' !== $prefix) { + $name = $prefix.'['.$name.']'; + } + + $pairs = match (true) { + is_array($value), + is_object($value) => [...$pairs, ...self::parametersToPairs($value, $name, $recursive)], + is_scalar($value) => [...$pairs, [$name, self::uvString($value)]], + default => $pairs, + }; + } + + return $pairs; } public function value(): ?string @@ -424,7 +466,7 @@ public function delete(?string $name): void $this->updateQuery(match (func_num_args()) { 1 => $this->pairs->withoutPairByKey($name), 2 => $this->pairs->withoutPairByKeyValue($name, self::uvString(func_get_arg(1))), /* @phpstan-ignore-line */ - default => throw new ArgumentCountError(__METHOD__.' requires at least one r as the pair name and a second optional argument as the pair value.'), + default => throw new ArgumentCountError(__METHOD__.' requires at least one argument as the pair name and a second optional argument as the pair value.'), }); } @@ -439,4 +481,18 @@ public function sort(): void { $this->updateQuery($this->pairs->sort()); } + + /** + * DEPRECATION WARNING! This method will be removed in the next major point release. + * + * @deprecated Since version 7.4.0 + * @see URLSearchParams::fromVariable() + * + * @codeCoverageIgnore + * + */ + public static function fromParameters(object|array $parameters): self + { + return new self(Query::fromParameters($parameters)); + } } diff --git a/components/Components/URLSearchParamsTest.php b/components/Components/URLSearchParamsTest.php index bc9c950e..9b53dddd 100644 --- a/components/Components/URLSearchParamsTest.php +++ b/components/Components/URLSearchParamsTest.php @@ -19,6 +19,7 @@ use League\Uri\Exceptions\SyntaxError; use PHPUnit\Framework\TestCase; use stdClass; +use Stringable; use Traversable; final class URLSearchParamsTest extends TestCase @@ -741,7 +742,7 @@ public function testFromParameters(): void ], ]; - $params = URLSearchParams::fromParameters($parameters); + $params = URLSearchParams::fromVariable($parameters); self::assertCount(4, $params); self::assertSame('bar baz', $params->get('filter[foo][0]')); self::assertSame('bar', $params->get('filter[bar][foo]')); @@ -805,4 +806,180 @@ public function testInstantiateWithString(): void self::assertSame('b', URLSearchParams::new('%3Fa=b')->get('?a')); } + + public function testFromParametersRespectURLSpecTypeConversion(): void + { + $parameters = new DateInterval('P12MT23H12S'); + + self::assertSame( + URLSearchParams::fromAssociative($parameters)->toString(), + URLSearchParams::fromVariable($parameters)->toString(), + ); + } + + /** + * @see https://github.com/php/php-src/tree/master/ext/standard/tests/http/http_build_query + * + * @dataProvider providesParametersInput + */ + public function testFromParametersWithDifferentInput(object|array $data, string $expected): void + { + self::assertSame($expected, URLSearchParams::fromVariable($data)->toString()); + } + + public static function providesParametersInput(): iterable + { + yield 'from an object with public properties' => [ + 'data' => ['foo' => 'bar', 'baz' => 1, 'test' => "a ' \" ", 'abc', 'float' => 10.42, 'true' => true, 'false' => false], + 'expected' => 'foo=bar&baz=1&test=a+%27+%22+&0=abc&float=10.42&true=true&false=false', + ]; + + yield 'empty parameters' => [ + 'data' => [], + 'expected' => '', + ]; + + yield 'encoding of the plus sign and the float number' => [ + 'data' => ['x' => 1E+14, 'y' => '1E+14'], + 'expected' => 'x=100000000000000.0&y=1E%2B14', + ]; + + yield 'class public properties' => [ + 'data' => new class () { + public string $public = 'input'; + protected string $protected = 'hello'; + private string $private = 'world'; /* @phpstan-ignore-line */ + }, + 'expected' => 'public=input', + ]; + + yield 'empty class' => [ + 'data' => new class () {}, + 'expected' => '', + ]; + + yield 'just stringable class' => [ + 'data' => new class () implements Stringable { + public function __toString(): string + { + return $this::class; + } + }, + 'expected' => '', + ]; + + yield 'stringable object' => [ + 'data' => ['hello', new class () implements Stringable { + public function __toString(): string + { + return $this::class; + } + }], + 'expected' => '0=hello', + ]; + + yield 'stringable class with public properties' => [ + 'data' => new class () implements Stringable { + public string $public = 'input'; + protected string $protected = 'hello'; + private string $private = 'world'; /* @phpstan-ignore-line */ + public function __toString(): string + { + return $this::class; + } + }, + 'expected' => 'public=input', + ]; + + $parent = new class () { + public mixed $public = 'input'; + protected string $protected = 'hello'; + private string $private = 'world'; /* @phpstan-ignore-line */ + }; + + $child = new class () { + public mixed $public = 'input'; + protected string $protected = 'hello'; + private string $private = 'world'; /* @phpstan-ignore-line */ + }; + + $parent->public = $child; + + yield 'nested classes' => [ + 'data' => $parent, + 'expected' => 'public%5Bpublic%5D=input', + ]; + + yield 'nested arrays' => [ + 'data' => [ + 20, + 5 => 13, + '9' => [ + 1 => 'val1', + 3 => 'val2', + 'string' => 'string', + ], + 'name' => 'homepage', + 'page' => 10, + 'sort' => [ + 'desc', + 'admin' => [ + 'admin1', + 'admin2' => [ + 'who' => 'admin2', + 2 => 'test', + ], + ], + ], + ], + 'expected' => '0=20&5=13&9%5B1%5D=val1&9%5B3%5D=val2&9%5Bstring%5D=string&name=homepage&page=10&sort%5B0%5D=desc&sort%5Badmin%5D%5B0%5D=admin1&sort%5Badmin%5D%5Badmin2%5D%5Bwho%5D=admin2&sort%5Badmin%5D%5Badmin2%5D%5B2%5D=test', + ]; + + yield 'test with mathematical expression' => [ + 'data' => [ + 'name' => 'main page', + 'sort' => 'desc,admin', + 'equation' => '10 + 10 - 5', + ], + 'expected' => 'name=main+page&sort=desc%2Cadmin&equation=10+%2B+10+-+5', + ]; + + yield 'test with array containing null value' => [ + 'data' => [null], + 'expected' => '', + ]; + + yield 'test with resource' => [ + 'data' => [STDIN], + 'expected' => '', + ]; + + $recursive = new class () { + public mixed $public = 'input'; + }; + $recursive->public = $recursive; + + yield 'test object recursion' => [ + 'data' => $recursive, + 'expected' => '', + ]; + + $v = 'value'; + $ref = &$v; + + yield 'using reference in array' => [ + 'data' => [$ref], + 'expected' => '0=value', + ]; + + yield 'using a traversable' => [ + 'data' => new class () implements IteratorAggregate { + public function getIterator(): Traversable + { + yield from ['foo' => 'bar']; + } + }, + 'expected' => '', + ]; + } } diff --git a/components/Modifier.php b/components/Modifier.php index 0738e6ac..c0995603 100644 --- a/components/Modifier.php +++ b/components/Modifier.php @@ -30,8 +30,8 @@ use Psr\Http\Message\UriFactoryInterface; use Psr\Http\Message\UriInterface as Psr7UriInterface; use Stringable; -use Traversable; +use function get_object_vars; use function ltrim; use function rtrim; use function str_ends_with; @@ -150,7 +150,7 @@ public function appendQueryPairs(iterable $pairs): self */ public function appendQueryParameters(object|array $parameters): self { - return $this->appendQuery(Query::fromParameters($parameters)->value()); + return $this->appendQuery(Query::fromVariable($parameters)->value()); } /** @@ -186,10 +186,10 @@ public function mergeQueryPairs(iterable $pairs): self /** * Merge PHP query parameters with the existing URI query. */ - public function mergeQueryParameters(iterable $parameters): self + public function mergeQueryParameters(object|array $parameters): self { $parameters = match (true) { - $parameters instanceof Traversable => iterator_to_array($parameters), + is_object($parameters) => get_object_vars($parameters), default => $parameters, }; @@ -200,7 +200,7 @@ public function mergeQueryParameters(iterable $parameters): self $currentParameters === $parameters => $this, default => new static($this->uri->withQuery( self::normalizeComponent( - Query::fromParameters([...$currentParameters, ...$parameters])->value(), + Query::fromVariable([...$currentParameters, ...$parameters])->value(), $this->uri ) )), diff --git a/docs/components/7.0/query.md b/docs/components/7.0/query.md index 47c49faf..c5fa3bac 100644 --- a/docs/components/7.0/query.md +++ b/docs/components/7.0/query.md @@ -174,13 +174,17 @@ $newQuery->__toString(); //return baz=toto&foo=bar&foo=toto ## Using the Query as a PHP data transport layer ~~~php -public static Query::fromParameters($params, string $separator = '&'): self +public static Query::fromVariable($params, string $separator = '&'): self public Query::parameters(): array public Query::parameter(string $name): mixed public Query::withoutNumericIndices(): self public Query::withoutParameter(...string $offsets): self ~~~ +

Query::fromVariable replaced the deprecated +Query::fromParameters named constructor which was +not inconsistent against http_build_query algorithm.

+ ### Using PHP data structure to instantiate a new Query object Historically, the query string has been used as a data transport layer of PHP variables. The `fromParams` uses @@ -189,7 +193,7 @@ PHP own data structure to generate a query string *à la* `http_build_query`. ~~~php parse_str('foo=bar&bar=baz+bar', $params); -$query = Query::fromParameters($params, '|'); +$query = Query::fromVariable($params, '|'); echo $query->value(); // returns 'foo=bar|bar=baz%20bar' ~~~ @@ -244,7 +248,7 @@ If your query string is created with `http_build_query` or the `Query::fromParam The `Query::withoutNumericIndices` removes any numeric index found in the query string as shown below: ~~~php -$query = Query::fromParameters(['foo' => ['bar', 'baz']]); +$query = Query::fromVariable(['foo' => ['bar', 'baz']]); echo $query->value(); //return 'foo[0]=bar&foo[1]=baz' $new_query = $query->withoutNumericIndices(); echo $new_query->value(); //return 'foo[]=bar&foo[]=baz' diff --git a/docs/components/7.0/urlsearchparams.md b/docs/components/7.0/urlsearchparams.md index 61549186..0c441e66 100644 --- a/docs/components/7.0/urlsearchparams.md +++ b/docs/components/7.0/urlsearchparams.md @@ -43,7 +43,7 @@ or one of the more specialized named constructors to avoid subtle bugs described - The `URLSearchParams::fromUri` instantiate from a URI. - The `URLSearchParams::fromPairs` instantiate from a collection of pairs. - The `URLSearchParams::fromAssociative` instantiate from an associative array or any object with public properties or generic key/value iterator (nested value are not supported). -- The `URLSearchParams::fromParameters` instantiate from the result of `parse_str` or the input of `http_build_query`. +- The `URLSearchParams::fromVariable` instantiate from the result of `parse_str` or the input of `http_build_query`. ```php $parameters = [ @@ -55,7 +55,7 @@ $parameters = [ ], ]; -$params = URLSearchParams::fromParameters($parameters); +$params = URLSearchParams::fromVariable($parameters); $params->get('filter[dateRange][start]'); //returns '2023-01-01 echo $params; //display "filter%5BdateRange%5D%5Bstart%5D=2023-01-01&filter%5BdateRange%5D%5Bend%5D=2023-08-31" @@ -68,6 +68,10 @@ echo URLSearchParams::fromAssociative($interval)->toString();

To adhere to the specification, if a string starts with the character ?; URLSearchParams::new will ignore it before parsing the string.

+

URLSearchParams::fromVariable replaced the deprecated +URLSearchParams::fromParameters named constructor which was +not inconsistent against http_build_query algorithm.

+ ```php use League\Uri\Components\URLSearchParams;