Hey Everyone,
As I mentioned in a previous email of mine (
https://externals.io/message/129486#130077),
I recently separated the query parameter handling sub-proposal from
https://wiki.php.net/rfc/uri_followup into its own RFC because it was way
too complex.
Therefore I'm officially opening its discussion.
After the separation, I reworked the proposal quite a lot: the single
biggest change
is that now, only a single class would be added: Uri\QueryParams instead of
both an RFC 3986
and a WHATWG URL compatible implementation.
The focus of the RFC is now to move away from the usage of the $_GET
superglobal,
which goal comes with two additional expectations:
- the new implementation should have comparable performance to $_GET
- the new implementation should support most capabilities of $_GET (e.g.
arrays)
The first one is probably straightforward to achieve, the latter one has
fundamental
problems: PHP's feature set (mostly: array support) is not compatible with
the WHATWG URL,
so some behavior likely wouldn't comply with this specification. That's why
the RFC
still has some TBD parts (e.g. Array API), or some contradicting info
related to some getters' and setters' signature/behavior.
Other than the API itself, the proposal doesn't have many questions, except
one thing: whether the new class should be readonly or not? I tend to make
it readonly, but I'm still not sure (since
this class can be used as a Builder).
Regards,
Máté
Hi
Therefore I'm officially opening its discussion.
You forgot to link the RFC. I'm doing that here:
https://wiki.php.net/rfc/query_params
Best regards
Tim Düsterhus
Hi
Other than the API itself, the proposal doesn't have many questions, except
one thing: whether the new class should be readonly or not? I tend to make
it readonly, but I'm still not sure (since
this class can be used as a Builder).
I don't think it is as necessary to make it readonly as it is with the
URI classes, since the query parameters class is likely to be used for
“local modification” and is less likely to be passed around as a value
object. If efficient copy-on-write is possible internally, then making
it readonly is the safer choice, of course.
With regard to the RFC text, I've given it a first quick skim and have
the following notes:
- Within the "Relation to the query component" section.
The $uri = new Uri\Rfc3986\Uri("https://example.com?foo=a b"); example
is invalid. An URI containing a space is not valid and the Uri class
will throw an exception. Can you double-check that part of the RFC?
Without looking it up, I suspect that the space should be %20 for RFC3986?
- Within the "Modification" section:
“Neither append(), nor set() do any percent-encoding or decoding of
their arguments.”
This should explain that during stringification, the % will be encoded
as %25. Thus the example will result in:
foo%255B%255D=ab%2563&bar%255B%255D=de%2566
- Within the "Modification" section:
“Finally, sort() sorts the query parameter list alphabetically:”
It is implied by the example, but should be spelled out explicitly:
Parameters with the same name should keep their original order and
should not be sorted by value.
- Within the "Supported types" section.
“The above conversion rules work for both UriQueryParams and
UrlQueryParams.”
and the examples also use Uri\Rfc3986\UriQueryParams /
Uri\WhatWg\UrlQueryParams.
This seems like an oversight from the original implementation, since
there are no longer separate classes. Can you double-check the entire
section?
- Within the "Array API" section.
This also uses Uri\Rfc3986\UriQueryParams in the examples.
- Within the "RFC Impact" section.
The ecosystem impact can probably be “None”.
I'll give this an in-depth read later, when the obvious mistakes are fixed.
Best regards
Tim Düsterhus
Hey Tim,
Thank you for the help, I forgot to link the RFC indeed.
Let me answer your questions:
I don't think it is as necessary to make it readonly as it is with the
URI classes, since the query parameters class is likely to be used for
“local modification” and is less likely to be passed around as a value
object. If efficient copy-on-write is possible internally, then making
it readonly is the safer choice, of course.
Yes, as far as I know from Ilija, it's possible to optimize modification
based on
refcount: if a readonly property which is to be modified has a refcount of
1 and
there's no weakref, it can be safely updated in-place.
With regard to the RFC text, I've given it a first quick skim and have
the following notes:
- Within the "Relation to the query component" section.
The
$uri = new Uri\Rfc3986\Uri("https://example.com?foo=a b");example
is invalid. An URI containing a space is not valid and theUriclass
will throw an exception. Can you double-check that part of the RFC?
Without looking it up, I suspect that the space should be%20for
RFC3986?
Nice catch, I'll review and fix the examples using "a b". It is indeed
incorrect, and %20
should be used instead. as you said.
- Within the "Supported types" section.
“The above conversion rules work for both UriQueryParams and
UrlQueryParams.”and the examples also use Uri\Rfc3986\UriQueryParams /
Uri\WhatWg\UrlQueryParams.This seems like an oversight from the original implementation, since
there are no longer separate classes. Can you double-check the entire
section?
I intentionally haven't updated this section yet: because there's important
info about
the difference between how WHATWG specification and uriparser handles null,
so I didn't want to "lose" this (I know it remains in the changelog).
- Within the "Array API" section.
This also uses Uri\Rfc3986\UriQueryParams in the examples
Same situation as above, but basically the whole section is a big question
mark:
I couldn't come up with a good enough idea how arrays could be handled
efficiently and ergonomically, and in the same time, how to (mostly) conform
to WHATWG URL.
Some of my problems:
- How should $params->get("foo[bar]") behave? Should we try to parse the
supplied key as an array and return the
nested item if it exists? I suppose, we would internally store the query
params as an array (just like how $_GET does),
so then we would definitely need parsing in order to be able to return item
"bar" stored within array "foo". - However, what should a $params->append("foo[bar]") call do when the query
param "foo" is already added with a string value?
Should it modify the existing item to be an array (but then how?) or we
should just add "foo[bar]" to the query params as-is? - How to recompose list parameters? Ideally, we should not append [] to
these parameters (e.g.
"list=1&list=2" vs "list[]=1&list[]=2"), and actually, it is not needed for
arrays in the root level, but as far as I understood,
it's a must-have for nested lists (e.g. "map[list]=1&map[list]=2" vs.
map[list[]]=1&map[list[]]=2), otherwise the representation
would be ambiguous for lists with 1 item: "map[list]=1" would mean that the
map has a list item with a value of "1". So
we either always have to use the [] suffix for lists, or we should make a
distinction between root-level and nested lists.
I know, most of my questions revolve around parsing the $name parameter,
but I would really want to minimize the number
of times when the $name param has to be parsed run-time so that the
performance is comparable to native array key
lookups. I don't have any idea how much the parsing would cost though...
So at this point, coming up with a sensible plan is needed, and I can fix
most inconsistencies in the RFC afterwards.
Regards,
Máté
Hi
Same situation as above, but basically the whole section is a big question
mark:
I couldn't come up with a good enough idea how arrays could be handled
efficiently and ergonomically, and in the same time, how to (mostly) conform
to WHATWG URL.
The internal storage should not know about arrays. It should represent
things as a list of key-value pairs, with arrays only being materialized
for the methods that are intended to deal with arrays.
Given that the class is generic, matching the WHATWG API 1:1 is not
necessary in my opinion.
Some of my problems:
- How should $params->get("foo[bar]") behave? Should we try to parse the
supplied key as an array and return the
nested item if it exists? I suppose, we would internally store the query
params as an array (just like how $_GET does),
so then we would definitely need parsing in order to be able to return item
"bar" stored within array "foo".
->get() does not currently exist within the RFC. The WHATWG get() is
equivalent to ->getFirst().
- However, what should a $params->append("foo[bar]") call do when the query
param "foo" is already added with a string value?
Should it modify the existing item to be an array (but then how?) or we
should just add "foo[bar]" to the query params as-is?
It should add foo[bar] as-is. foo and foo[bar] are different keys.
The square-bracket syntax is something that is specific to PHP.
- How to recompose list parameters? Ideally, we should not append [] to
these parameters (e.g.
The recomposition should be "dumb" and just use the keys as-is. The
complex logic should happen in the getter or setter. Only the 'array'
getters and setters deal with the PHP-specific square-bracket syntax.
To give an example:
"k=1&k=2"
[['k', '1'], ['k', '2']]
getAll('k') // ['1', '2']
getArray('k') // UriException: 'k' is not an array.
"k[]=1&k[]=2"
[['k[]', '1'], ['k[]', '2']]
getAll('k') // []
getAll('k[]') // ['1', '2']
getArray('k') // ['1', '2']
"k[a]=1&k[b]=2"
[['k[a]', '1'], ['k[b]', '2']]
getAll('k') // []
getAll('k[a]') // ['1']
getArray('k') // ['a' => '1', 'b' => '2']
"k=1&k[]=2"
[['k', '1'], ['k[]', '2']]
getAll('k') // ['1']
getAll('k[]') // ['2']
getArray('k') // UriException: 'k' is not an array.
"k[]=2&k=3";
[['k[]', '2'], ['k', '3']];
getAll('k') // ['3']
getAll('k[]') // ['2']
getArray('k') // UriException: 'k' is not an array.
The string should internally be represented something like the arrays
and the getters should work as indicated in the comment.
And then for the setters:
// []
append('k', '1') // [['k', '1']]
append('k', '2') // [['k', '1'], ['k', '2']]
append('k[]', '3') // [['k', '1'], ['k', '2'], ['k[]', '3']]
set('k', '4') // [['k[]', '3'], ['k', '4']]
The setArray() method would then create a number of entries with
square brackets added as appropriate (and delete all entries that would
be returned by getArray()). The appendArray() method should probably
be dropped to avoid issues with "mixing non-array with array keys". So:
// []
setArray('k', ['1']) // [['k[]', '1']]
setArray('k', ['1', '2']) // [['k[]', '1'], ['k[]', '2']]
setArray('k', ['a' => '1', '2']) // [['k[a]', '1'], ['k[0]', '2']]
append('k', 'x') // [
// ['k[a]', '1'],
// ['k[0]', '2'],
// ['k', 'x'],
// ]
setArray('k', ['1']) // [['k[]', '1']]
Does that make sense?
Best regards
Tim Düsterhus
Hi Tim,
First of all, thanks for your detailed explanation!
Does that make sense?
Sure, this is a very sensible approach, similar to the algorithms I have
also considered myself before. The reason why I
started to search for alternatives is because lookups are inefficient this
way: all get*() methods have an O(n) time complexity
by default, while currently, looking up keys in $_GET is O(1). This
regression is what I tried to avoid with storing query params
along with an array key.
However, I realized in the meanwhile that it may be possible to eliminate
much of the performance overhead by using two hash tables:
one for exact search (WHATWG URL-alike API), and one for searching the
prefixes before the [] pair (for the array API). I haven't
tried this idea out yet in practice, so I'll need to do some POC first
before continuing with the RFC.
Regards,
Máté
Hello Tim et All,
I managed to implement a considerable part of the proposal efficiently, as
I planned. However, I came to a different conclusion
regarding the "Array API" (https://wiki.php.net/rfc/query_params#array_api)
than what I originally wrote:
First and foremost, I wasn't happy that the Array API would try to mimic
native array handling. For example, QueryParams::getArray("foo[bar][baz]")
would try to parse the supplied array dimensions and then find index "baz"
of index "bar" of array "foo". QueryParams::appendArray() and
QueryParams::setArray() would need a similar behavior. I wasn't keen on
reimplementing array operations within query params. In order to solve
this Gordian knot, I want to simplify the Array API to only two (or maybe
three) methods:
- QueryParams::fromArray(): Creates a new QueryParams object from an array
- QueryParams::withArray(): Does the same as above, but from an existing
instance (I'm not 100% sure about this method yet) - QueryParams::toArray(): Creates an array from the QueryParams instance
If someone wants to do any kind of array modifications, then they can do it
by retrieving the query params array first via toArray(), change what's
needed
by using PHP's native array manipulation functionalities, and finally,
convert the.array back to a QueryParams instance. I think this behavior is
simple, clear,
and also extensible later, if we learn about a better approach...
Besides, I also realized that supporting some kind of configuration will
also be necessary mostly for security reasons. These are the options
I'm considering to add:
- parsingMaxQueryStringLength: the maximum length of the input query string
that can be parsed by the parse*() methods - parsingMaxParamCount: The maximum number of parameters that the input
query string can contain when calling any parse*() methods (similar
to the "max_input_vars" php.ini option). - parsingMaxNestingLevel: The maximum nesting level of arrays that can be
parsed when calling any parse*() methods (similar
to the "max_input_nesting_level" php.ini option)
I figured that it might be useful to control how some specific type of
query parameter values are represented when adding a new query
parameter via calling e.g. QueryParams::append("param", $value).
- boolean values: by default, true would be represented as "1" and false
would be represented as "0" - null values: there are three ways to handle them: 1) omitting these; 2)
adding them without a "=", containing only the param name (e.g. "?foo");
- and converting the null value to an empty string (e.g. "?foo=")
I'm not really sure how useful these options are, so I'm curious to get
feedback about them. It's also an option to only accept string or null
values by the manipulator methods, but I think it would provide a better UX
to support all valid types (e.g. an int is a valid one, but an object
isn't).
It's also a question how and where these configs should be passed.
Currently, I group them together in a dedicated QueryParamOptions object
which has the following signature:
final readonly class QueryParamOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
public string $trueValue = "0",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}}
However, the properties prefixed by "parsing" are only applicable during
parsing, so maybe they should be passed separately to the parse*() methods?
What are your thoughts?
Regards,
Máté
Hello Tim et All,
I managed to implement a considerable part of the proposal efficiently, as
I planned. However, I came to a different conclusion
regarding the "Array API" (https://wiki.php.net/rfc/query_params#array_api)
than what I originally wrote:First and foremost, I wasn't happy that the Array API would try to mimic
native array handling. For example, QueryParams::getArray("foo[bar][baz]")
would try to parse the supplied array dimensions and then find index "baz"
of index "bar" of array "foo". QueryParams::appendArray() and
QueryParams::setArray() would need a similar behavior. I wasn't keen on
reimplementing array operations within query params. In order to solve
this Gordian knot, I want to simplify the Array API to only two (or maybe
three) methods:
- QueryParams::fromArray(): Creates a new QueryParams object from an array
- QueryParams::withArray(): Does the same as above, but from an existing
instance (I'm not 100% sure about this method yet)- QueryParams::toArray(): Creates an array from the QueryParams instance
If someone wants to do any kind of array modifications, then they can do
it by retrieving the query params array first via toArray(), change what's
needed
by using PHP's native array manipulation functionalities, and finally,
convert the.array back to a QueryParams instance. I think this behavior is
simple, clear,
and also extensible later, if we learn about a better approach...Besides, I also realized that supporting some kind of configuration will
also be necessary mostly for security reasons. These are the options
I'm considering to add:
- parsingMaxQueryStringLength: the maximum length of the input query
string that can be parsed by the parse*() methods- parsingMaxParamCount: The maximum number of parameters that the input
query string can contain when calling any parse*() methods (similar
to the "max_input_vars" php.ini option).- parsingMaxNestingLevel: The maximum nesting level of arrays that can be
parsed when calling any parse*() methods (similar
to the "max_input_nesting_level" php.ini option)I figured that it might be useful to control how some specific type of
query parameter values are represented when adding a new query
parameter via calling e.g. QueryParams::append("param", $value).
- boolean values: by default, true would be represented as "1" and false
would be represented as "0"- null values: there are three ways to handle them: 1) omitting these; 2)
adding them without a "=", containing only the param name (e.g. "?foo");
- and converting the null value to an empty string (e.g. "?foo=")
I'm not really sure how useful these options are, so I'm curious to get
feedback about them. It's also an option to only accept string or null
values by the manipulator methods, but I think it would provide a better
UX to support all valid types (e.g. an int is a valid one, but an object
isn't).It's also a question how and where these configs should be passed.
Currently, I group them together in a dedicated QueryParamOptions object
which has the following signature:final readonly class QueryParamOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
public string $trueValue = "0",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}}However, the properties prefixed by "parsing" are only applicable during
parsing, so maybe they should be passed separately to the parse*() methods?
What are your thoughts?Regards,
MátéHi Máté,
thanks for the feedback and for the work already done on this RFC.
- QueryParams::fromArray(): Creates a new QueryParams object from an
array- QueryParams::withArray(): Does the same as above, but from an existing
instance (I'm not 100% sure about this method yet)- QueryParams::toArray(): Creates an array from the QueryParams instance
IMHO the suffix "Array" is an implementation detail I would prefer
something like "fromQueryParams", "toQueryParams" and
"(with|merge)QueryParams" . This conveys more the nature of the operations
than the type of operation.
I think your reasoning to simplify this part of the RFC makes sense instead
of adding a complex API users may want to rely on their own "Collection"
like object structure to deal with Query parameters
modification as long as they adhere to the principle of only
allowing scalar type and null as possible values.
final readonly class QueryParamOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
public string $trueValue = "0",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}}
This seems to me 2 pairs of options: One used for parsing, the other for
building. I would split this class into two QueryParamParsingOptions and
QueryParamsBuildingOptions, names can be improved.
final readonly class QueryParamParsingOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
) {}
}
And
final readonly class QueryParamBuildingOptions
{
public function __construct(
public string $trueValue = "1",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}
}
The QueryParamParsingOptions should be an argument for all your parse*
methods with sensible default value the same for the QueryParamBuildingOptions
but for theto* methods. The behaviour toward
using null seems unclear to me when checking the class on itself. partly
because it is a boolean, partly because you state that there's 3 ways to
handle null values and a boolean only allows for 2 ways. Perhaps
using an Enum would be a better fit in this case ?
Best regards,
Ignace
On Tue, Jul 14, 2026 at 8:15 AM ignace nyamagana butera nyamsprod@gmail.com
wrote:
On Wed, Jul 8, 2026 at 11:31 PM Máté Kocsis kocsismate90@gmail.com
wrote:Hello Tim et All,
I managed to implement a considerable part of the proposal efficiently,
as I planned. However, I came to a different conclusion
regarding the "Array API" (
https://wiki.php.net/rfc/query_params#array_api) than what I originally
wrote:First and foremost, I wasn't happy that the Array API would try to mimic
native array handling. For example, QueryParams::getArray("foo[bar][baz]")
would try to parse the supplied array dimensions and then find index
"baz" of index "bar" of array "foo". QueryParams::appendArray() and
QueryParams::setArray() would need a similar behavior. I wasn't keen on
reimplementing array operations within query params. In order to solve
this Gordian knot, I want to simplify the Array API to only two (or
maybe three) methods:
- QueryParams::fromArray(): Creates a new QueryParams object from an array
- QueryParams::withArray(): Does the same as above, but from an existing
instance (I'm not 100% sure about this method yet)- QueryParams::toArray(): Creates an array from the QueryParams instance
If someone wants to do any kind of array modifications, then they can do
it by retrieving the query params array first via toArray(), change what's
needed
by using PHP's native array manipulation functionalities, and finally,
convert the.array back to a QueryParams instance. I think this behavior is
simple, clear,
and also extensible later, if we learn about a better approach...Besides, I also realized that supporting some kind of configuration will
also be necessary mostly for security reasons. These are the options
I'm considering to add:
- parsingMaxQueryStringLength: the maximum length of the input query
string that can be parsed by the parse*() methods- parsingMaxParamCount: The maximum number of parameters that the input
query string can contain when calling any parse*() methods (similar
to the "max_input_vars" php.ini option).- parsingMaxNestingLevel: The maximum nesting level of arrays that can
be parsed when calling any parse*() methods (similar
to the "max_input_nesting_level" php.ini option)I figured that it might be useful to control how some specific type of
query parameter values are represented when adding a new query
parameter via calling e.g. QueryParams::append("param", $value).
- boolean values: by default, true would be represented as "1" and false
would be represented as "0"- null values: there are three ways to handle them: 1) omitting these; 2)
adding them without a "=", containing only the param name (e.g. "?foo");
- and converting the null value to an empty string (e.g. "?foo=")
I'm not really sure how useful these options are, so I'm curious to get
feedback about them. It's also an option to only accept string or null
values by the manipulator methods, but I think it would provide a better
UX to support all valid types (e.g. an int is a valid one, but an object
isn't).It's also a question how and where these configs should be passed.
Currently, I group them together in a dedicated QueryParamOptions object
which has the following signature:final readonly class QueryParamOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
public string $trueValue = "0",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}}However, the properties prefixed by "parsing" are only applicable during
parsing, so maybe they should be passed separately to the parse*() methods?
What are your thoughts?Regards,
MátéHi Máté,
thanks for the feedback and for the work already done on this RFC.
- QueryParams::fromArray(): Creates a new QueryParams object from an
array- QueryParams::withArray(): Does the same as above, but from an existing
instance (I'm not 100% sure about this method yet)- QueryParams::toArray(): Creates an array from the QueryParams instance
IMHO the suffix "Array" is an implementation detail I would prefer
something like "fromQueryParams", "toQueryParams" and
"(with|merge)QueryParams" . This conveys more the nature of the operations
than the type of operation.I think your reasoning to simplify this part of the RFC makes sense
instead of adding a complex API users may want to rely on their own
"Collection" like object structure to deal with Query parameters
modification as long as they adhere to the principle of only
allowing scalar type and null as possible values.final readonly class QueryParamOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
public string $trueValue = "0",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}}This seems to me 2 pairs of options: One used for parsing, the other for
building. I would split this class into two QueryParamParsingOptions and
QueryParamsBuildingOptions, names can be improved.final readonly class QueryParamParsingOptions
{
public function __construct(
public int $parsingMaxQueryStringLength = 10000,
public int $parsingMaxParamCount = 1000,
public int $parsingMaxNestingLevel = 64,
) {}
}And
final readonly class QueryParamBuildingOptions
{
public function __construct(
public string $trueValue = "1",
public string $falseValue = "0",
public bool $useNullAsEmptyString = false
) {}
}The QueryParamParsingOptions should be an argument for all your
parse*
methods with sensible default value the same for the QueryParamBuildingOptions
but for theto*methods. The behaviour toward
usingnullseems unclear to me when checking the class on itself. partly
because it is a boolean, partly because you state that there's 3 ways to
handle null values and a boolean only allows for 2 ways. Perhaps
using an Enum would be a better fit in this case ?Best regards,
Ignace
Hi Máté,
As a follow up I would propose the following Enum but again names can be
change/improve
namespace Uri;
enum QueryNullPolicy {
case SkipTuple;
case KeyOnly;
case EmptyString;
}
and the class becomes
final readonly class QueryParamBuildingOptions
{
public function __construct(
public string $trueValue = "1",
public string $falseValue = "0",
public QueryNullPolicy $nullPolicy = QueryNullPolicy::SkipTuple
) {}
}
which is more readable and understandable IMHO
Best regards,
Ignace
Hi Ignace,
Thanks for your feedback!
I'm fine with separating the options to their own object. However, what I
would like to make sure about first is whether
the limits should really be applicable for parsing methods only, or rather
other methods should also respect them?
- $parsingMaxQueryStringLength: This option is only useful during parsing,
so it's out of scope for my question. - $parsingMaxParamCount: the append() and set() modification methods could
also respect a $maxParamCount option,
triggering an exception when the query parameter list exceeds the max
parameter count. - $parsingMaxNestingLevel: similarly, append() and set() could also respect
it (e.g. by counting the number
of opening brackets in the query param name (?)). Enforcing this option
during parsing and modification would result in
some performance penalty, since the param name doesn't need introspection
during these operations otherwise.
That's why an alternative is to make a $maxNestingLevel option exclusive to
the toArray() method, because that's the
only time nesting level actually comes into play (and the brackets need
parsing at that point anyway). The possible problem
with this delayed validation is that possibly harmful query strings can be
passed along more easily.
The reason why I originally thought about supporting these options during
only parsing is because I'd think that
that's the most uncontrollable part of query parameter handling. E.g. one
can enforce a maximum query param count
easily after parsing if this is needed (e.g. checking count() before
calling append() or set()).
... the same for the QueryParamBuildingOptions but for theto* methods.
Actually, the building options should be passed to append() and set(). It's
because their $value parameter is type juggled to
string or null immediately, thus their original type gets lost afterwards.
On one hand, this behavior is useful for the hasValue() or deleteValue()
methods which try to find their $value param in the
query param list (comparing the type juggled $value with the stored values
-> a boolean "true" $value will be equal to a string "1"
in the list without the need to type juggle all stored query params).
On the other hand though, this behavior might lead to interoperability
issues, e.g. when two libraries use different building options
to write to the same QueryParams.... Although, I think this is also a
possible problem even if building options are not supported,
so probably it's not a blocking issue. But most probably, passing them
during construction would slightly alleviate this problem,
because all writes would use the exact same config.
Regards,
Máté
Hi Ignace,
Thanks for your feedback!
I'm fine with separating the options to their own object. However, what I
would like to make sure about first is whether
the limits should really be applicable for parsing methods only, or
rather other methods should also respect them?
- $parsingMaxQueryStringLength: This option is only useful during
parsing, so it's out of scope for my question.- $parsingMaxParamCount: the append() and set() modification methods
could also respect a $maxParamCount option,
triggering an exception when the query parameter list exceeds the max
parameter count.- $parsingMaxNestingLevel: similarly, append() and set() could also
respect it (e.g. by counting the number
of opening brackets in the query param name (?)). Enforcing this option
during parsing and modification would result in
some performance penalty, since the param name doesn't need introspection
during these operations otherwise.
That's why an alternative is to make a $maxNestingLevel option exclusive
to the toArray() method, because that's the
only time nesting level actually comes into play (and the brackets need
parsing at that point anyway). The possible problem
with this delayed validation is that possibly harmful query strings can
be passed along more easily.The reason why I originally thought about supporting these options during
only parsing is because I'd think that
that's the most uncontrollable part of query parameter handling. E.g. one
can enforce a maximum query param count
easily after parsing if this is needed (e.g. checkingcount()before
calling append() or set()).... the same for the QueryParamBuildingOptions but for the
to*methods.Actually, the building options should be passed to append() and set().
It's because their $value parameter is type juggled to
string or null immediately, thus their original type gets lost afterwards.On one hand, this behavior is useful for the hasValue() or deleteValue()
methods which try to find their $value param in the
query param list (comparing the type juggled $value with the stored
values -> a boolean "true" $value will be equal to a string "1"
in the list without the need to type juggle all stored query params).On the other hand though, this behavior might lead to interoperability
issues, e.g. when two libraries use different building options
to write to the same QueryParams.... Although, I think this is also a
possible problem even if building options are not supported,
so probably it's not a blocking issue. But most probably, passing them
during construction would slightly alleviate this problem,
because all writes would use the exact same config.Regards,
Máté
Hi Màté,
I believe the question is more about domain boundaries.
AFAIK:
- HTTP servers already have a cap on how long an URL and thus a query
string can be - the query string nesting or parsing depends only on how it is being
consumed by the client and should not be restricted by the URI producer.
Caveats:
- the supported URI length depends on the server, CLI scripts and user
scripts are able to bypass those HTTP server restrictions.
Here's my thoughts:
- Parsing and building are at the boundary of the system and control the
I/O flow - Parsing makes the system behave like a consumer, Building makes the
system behave like a producer. - I consider that dynamic methods are part of the domain because they only
deal with PHP data not with external data (we do not allow concatenating 2
query string for instance)
So I would not add the parsing options to the class modifier because on
itself PHP should not add more constraints than needed. business rules might
but IMHO this is out of scope for a low level API.
Does PHP work with more nested levels or query parameters, the answer is
yes. Should we recommend doing so, hell no but this is a business
constraint unless it is a clear and known security issue when building the
query string.
On the other hand, I do agree that being able to configure how boolean and
null values are handled when using any modifier methods is a needed feature.
TL;DR:
Adding the QueryParamBuildingOptions options as the last argument with
sensible default to any modifier methods seems reasonable to me
but adding
public int $maxParamCount = 1000,
public int $maxNestingLevel = 64,
to QueryParamBuildingOptions without a clear and realistic example where
this is solving a security issue seems like too much of a restriction for
the API.
Best regards,
Ignace
Hey Máté,
Nice work, I really appreciate the effort! While adopting Uri I stumbled upon this missing feature as well.
In the examples you mention the usage of parse and toString. I assume there is no “default” and this is a placeholder for one of the given implementations. But it’s perhaps better to pick one of for the examples as it suggests it would also exist.
When it comes to the implementation I believe it would be nice to also have a getKeys() method. That would only return the keys.
You do mention the focus is to move away from $_GET. I like the idea, but implementing QueryString would still require something like the following:
Uri\QueryParams::fromArray($_GET);
If we really want to make $_GET obsolete, wouldn’t it be nice to have a fromRequest() which would directly parse $_GET or use $_SERVER["QUERY_STRING”].
As for whether or not this should be a readonly class. I think it should be. The class itself is following various specs. And since all other classes in the same namespace are also readonly, it should follow the same principle (also for consistency) I believe.
Regards,
Jordi
Hey Everyone,
As I mentioned in a previous email of mine (https://externals.io/message/129486#130077),
I recently separated the query parameter handling sub-proposal from https://wiki.php.net/rfc/uri_followup into its own RFC because it was way too complex.
Therefore I'm officially opening its discussion.After the separation, I reworked the proposal quite a lot: the single biggest change
is that now, only a single class would be added: Uri\QueryParams instead of both an RFC 3986
and a WHATWG URL compatible implementation.The focus of the RFC is now to move away from the usage of the $_GET superglobal,
which goal comes with two additional expectations:
- the new implementation should have comparable performance to $_GET
- the new implementation should support most capabilities of $_GET (e.g. arrays)
The first one is probably straightforward to achieve, the latter one has fundamental
problems: PHP's feature set (mostly: array support) is not compatible with the WHATWG URL,
so some behavior likely wouldn't comply with this specification. That's why the RFC
still has some TBD parts (e.g. Array API), or some contradicting info related to some getters' and setters' signature/behavior.Other than the API itself, the proposal doesn't have many questions, except one thing: whether the new class should be readonly or not? I tend to make it readonly, but I'm still not sure (since
this class can be used as a Builder).Regards,
Máté
Hi Jordi,
In the examples you mention the usage of parse and toString. I assume
there is no “default” and this is a placeholder for one of the given
implementations. But it’s perhaps better to pick one of for the examples as
it suggests it would also exist.
Ah, thanks for the info! Most of these were omission in fact, as I forgot
to update the original method names after I unified the two QueryParams
classes.
When it comes to the implementation I believe it would be nice to also
have agetKeys()method. That would only return the keys.You do mention the focus is to move away from $_GET. I like the idea, but
implementing QueryString would still require something like the following:Uri\QueryParams::fromArray($_GET);
If we really want to make
$_GETobsolete, wouldn’t it be nice to have a
fromRequest()which would directly parse$_GETor use
$_SERVER["QUERY_STRING”].
As far as I can see, Uri\QueryParams::fromArray($_GET); wouldn't be needed.
Do you have any use-case in mind where the suggested
"Uri\QueryParams::parseRfc3986($_SERVER["QUERY_STRING"]);" call wouldn't
work instead? I'm not against adding a fromRequest()
method though if others find it useful (I'm a bit neutral about it). And
I'd probably use a fromCurrentRequest() or a similar name to
highlight the fact that it uses the current request as source.
As for whether or not this should be a readonly class. I think it should
be. The class itself is following various specs. And since all other
classes in the same namespace are also readonly, it should follow the same
principle (also for consistency) I believe.
I think the most problematic case is to return a mutable class from a
readonly class (e.g. Uri\Rfc3986\Uri::getQueryParams()). So we should either
omit this method, or make Uri\QueryParams readonly indeed.
Regards,
Máté