[RFC] [Discussion] Query Parameter Manipulation Support View as Markdown

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:

1. 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?

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.

4. 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).

5. 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 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 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 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é

Hi Tim,

Thanks for the detailed review! Let me share my answers inline:

1. I don't think QueryParamParsingOptions::$maxQueryStringLength is

necessary or useful to have. Checking the input length is easy to do
manually (it's just a strlen() call) and I don't see a practical way
to abuse the API with large input lengths alone: The API will not “blow
up” inputs into considerably larger data structures, the input strings
are mostly just “copied” over, not too different from e.g.
str_replace().

I was also thinking a lot about whether this option is necessary to have.
My main reason for including it was that people would otherwise easily
forget
to limit the input length, whereas the dedicated option enforces the limit
everywhere.

So the real question is whether QueryParams really cannot be abused via
long strings, and whether $maxParamCount alone could mitigate those risks?

And now by thinking about it more, neither I could identify any specific
vulnerability that
would become possible by not enforcing the upper limit. In the previous
iteration
of the RFC, limiting the length was more useful, because parsing would have
been
a more resource intensive operation due to the requirements of the old
"array API".
The operations that are performed now during parsing are cheap, so as far
as I can see,
it's possible to get rid of this option if no one can come up with a good
enough use-case.

3. If $maxQueryStringLength goes away, the QueryParamParsingOptions
   would be a single property object. Maybe the param count limit should
   become a regular parameter then, since I can't think of any other useful
   parse-time options.

Yes, without trying to future proof the API too much, I guess this is the
only sensible
choice, unless someone finds any other option which would be needed.

4. For the QueryParamBuildingOptions I don't think they should be

passed as parameter to the parsing functions (and thus not be stored as
an object property). Instead they should be passed to the ->to*()
functions just when they are needed. That keeps the QueryParams class’
state focused on the actual parameters.

Edit after getting further down the RFC: This choice is explained
further below, because it immediately affects the getters. This makes
sense, but I still believe this is a less than ideal API design. But I
don't have better proposals other than “force the user to make a
decision and only allow passing in string”. For int the implicit
conversion to string would just work and making an explicit choice for
bool is easy enough.

Yes, it was another inner debate for me whether relying on explicit casting
to a string
by the user or supporting automatic conversion is a better approach. I
believe
offering some kind of automatic casting approach is more ergonomic, so I
think
it's useful to try to keep this mechanism if possible.

Moreover, the fromArray() method reads the input array recursively, which
means
that in the absence of building options, users would need to make sure that
each
parameter value is casted to a supported type, even the nested ones. For
most legal cases,
the conversion is trivial (int, float, backed enum), so why require an
explicit conversion?

On the other hand, the query param representation of boolean or null values
may vary
case by case, and as I've just realized it's not as simple as I previously
thought:

Let's take a Consul API endpoint for example (this is just a random example
which I
found first):
https://developer.hashicorp.com/consul/api-docs/operator/usage#global
The "global" query parameter represents a bool value, but only the query
parameter name needs to be passed, so an example request URL looks like
this:
"http://127.0.0.1:8500/v1/operator/usage?global";

If one would build the query parameters, probably the following code would
be used:

$params = new Uri\QueryParams();
if ($global) {
$params = $params->set("global", null);
}

This example made me realize that neither the modification methods should
respect
the $nullPolicy (in addition to hasValue() and deleteValue()), because
otherwise
one couldn't be sure if "global" would be included in the request -- it
would be omitted
if $nullPolicy is "SkipTuple".

As a result, I'm fine with changing the input param type for append() and
set():
instead of "mixed", maybe we could accept
"string|int|float|BackedEnum|null"? I still
think that using a "?string" type would be too pedantic. Going with the
above signature
would mean that one would need to cast a bool input to any of the supported
ones (or
omit them e.g. in case of false, as seen in the example above), and they
would also need
to handle null values on a case by case basis, since passing a null value
would always
add the parameter without a value (in line with
QueryParamNullPolicy::KeyOnly),
and the behavior of QueryParamNullPolicy::SkipTuple could be emulated by
simply skipping
calling append() or set() on the input. I don't know if it makes sense to
you,
but that's the idea which currently sounds the best for me.

However, I'm still unsure in case of fromArray() what would be the correct
solution that is
also as ergonomic as possible.

5. Is having a separate parseRfc3986() useful when RFC 3986 doesn't
   specify the handling of query parameters and just delegates to RFC 1866
   instead? A big benefit of the URI extension for me is that it clearly
   references the relevant standards in its naming and strictly implements
   them, making it easy to discover and allowing the use of primary
   references to understand what they are supposed to do. The
   parseRfc3986() method would muddy this straight-forward standard’s
   compliant implementation, particularly since it is defined to accept
   malformed inputs such as #foo=bar, which I would find unexpected from
   a method referencing RFC 3986 in its name.

As far as I can see, supporting RFC 3986 parsing is necessary to keep full
compatibility with the URI class so that the meaning of literal +
characters in
the query string wouldn't change during query parameter parsing.
Let's consider this code:

$uri = new Uri\Rfc3986\Uri("?q=C++");

$rfc1866Params = Uri\QueryParams::parseRfc1866($uri->getQuery());
$rfc3986Params = Uri\QueryParams::parseRfc3986($uri->getQuery());

var_dump($rfc1866Params->getFirst("q")); // string(3)
"C "
var_dump($rfc3986Params->getFirst("q")); // string(3)
"C++"

Unfortunately, WHATWG URL's URL and URLSearchParams classes have different
query parameter parsing algorithms (see the "Percent-Encoding and Decoding"
section),
so the same issue is already present there, and unavoidable. I'd like not
to repeat it in
case of RFC 3986, that's why I think parseRfc3986() is needed. But let me
know if
I miss something.

Regarding accepting malformed input: uriparser's own query parameter
handling
functions (uriDissectQueryMalloc*()) behave the same way as the RFC currently
proposes.
Unfortunately, I could not really find any reasoning behind this choice of
uriparser.

To be clear, I don't insist on following uriparser's behavior for RFC 1866
and RFC 3986 (it's
not an option for WHATWG URL, since it specifies what needs to be done in
every detail),
and probably strict parsing is what is more consistent with the Uri class
indeed.

Then passing an invalid query string syntax would throw. However, incorrect
UTF-8 sequences
would still be allowed, since neither RFC 1866, nor RFC 3986 require a
syntactically
correct query string to be valid UTF-8.

7. I cannot describe an actual issue, but I find accepting invalid

percent-encoding sequences to be dangerous. It would definitely be
unexpected that %6f=x' would be o=x, but %6g=ywould be%6g=y(which would then become%256g=y`).

Answered above. :)

8. The naming of hasValue() sounds like it would check if there is a

= sign for the given key somewhere, not that it would check that there
is a specific key-value pair. I'm also not sure if it is necessary.
Duplicated keys are comparatively rare and for the use cases where this
is a thing, in_array() on the result of getAll() would just work (or
array_any() for more complex checks).

This method is required by WHATWG URL:
https://url.spec.whatwg.org/#dom-urlsearchparams-has
It's specified as an overloaded method, so I had to introduce a separate
signature for this use-case, as per the policy accepted in
https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#policy_about_new_functions#policy_about_new_functions
.

TBH, I find it useful (more convenient and more efficient than getAll() +
in_array()).
The name itself may be slightly confusing at the first sight, but I think
the
$value parameter which must be passed should clarify its purpose. But in
any case,
I'm open to improving its name.

9. I'm not sure I like the name ->list(). Technically it's also
   redundant with iterator_to_array($queryParams), but having a dedicated
   method probably makes sense nevertheless.

Yes, I'm fine with renaming this method to toList(). It would go well with
toArray()
IMO. It's more ergonomic and more efficient than the alternative, so I agree
that it's useful.

10. Would it make sense to rename parseWhatWg() to

parseUrlSearchParams() or similar to make it clear that this
constructor implements the URLSearchParams specification. I'm asking due
to the section that mentioned how $whatWgUrl->getQuery() is not the
correct value to pass there (due to the questionmark handling). Renaming
this method would also make sense when removing the RFC 3986 parser (as
suggested in (5)).

I think it's a good idea. Maybe people using it would be less likely to be
surprised by
its behavior (how it handles the leading ? character) if the name referred
to
URLSearchParams. However, I think including WHATWG in the method name
would still have its purpose (people with less familiarity with WHATWG URL
would know immediately that this method is related to the WHATWG URL
specification),
so my preferred name would be parseWhatWgUrlSearchParams(), even
if it's somewhat verbose.

11. Bullet point 8 also applies to deleteValue(). Is there an actual

use case for this method?

Yes, it has the same reason:
https://url.spec.whatwg.org/#dom-urlsearchparams-delete

Also, this function may come handy in case of filtering. For example, let's
consider the
"tag=php&tag=web&sort=date" query string: deleteValue("tag", "php") would
remove one
selected filter while keeping the other parameters intact.

12. Is having the withArray() method useful? It does the same as
    QueryParams::fromArray(), no? It feels confusing to have a “replace
    everything” method when one could just create something entirely new.
    Technically one could even call $queryParams->fromArray(), since
    calling static methods on instances is legal.

Currently, the only description about this method is the following:

It replaces the whole query list with the input, but clones the current

instance rather than creating a completely new one.

So it's a more efficient form of instantiating a class based on an array
input than fromArray().
Additionally, it was designed to also keep the original building options
(if they are going to be kept
in the final proposal).

$queryParams = Uri\QueryParams::parseRfc3986("foo=bar", 1000, new
Uri\QueryParamBuildingOptions("true", "false"));
$array = [...];
// Currently preferred solution:
$queryParams = $queryParams->withArray($array);
// The same solution using fromArray():
$queryParams = Uri\QueryParams::fromArray($array,
$queryParams->getBuildingOptions());

P.S. The getBuildingOptions() doesn't exist yet in the proposal, and it
should be added if building options remain in the RFC at last.

13. “doesn't start with a “with” suffix” - that should read “prefix”.

Prefix is “start with”, suffix is “end with”.

Yeah, thanks for noticing it. It was an unfortunate typo, and I had
already fixed it in my draft version when you were reading it.

14. The “Backward Incompatible Changes” section should mention that

adding a new class is technically backwards incompatible, but an allowed
break according to policy and that the Uri namespace is reserved for
ext/uri since 8.5.

Fixed.

One more thing which I'm probably going to change: it turned out that
WHATWG URL specifies
the sort() method (https://url.spec.whatwg.org/#dom-urlsearchparams-sort)
which must use UTF-16 based
comparisons (using 2 bytes). It is really not obvious from the WHATWG URL,
only the WHATWG Infra
specification defines it: https://infra.spec.whatwg.org/#code-unit-less-than

In order to restore compatibility with this requirement, I'd define two
sorting algorithms:

• sortByBytes(): the currently specified one which would use bytes as the
  base of comparison
• sortByUtf16(): the one comparing UTF-16 sequences at once which would be
  compatible with WHATWG URL

Again, thank you for the review!

Máté