Hi
On 2026-03-01 23:31, Máté Kocsis wrote:
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.
I've given the RFC a fresh read from scratch after your email
https://news-web.php.net/php.internals/132810. I'm replying to the top
of the thread to reduce the nesting, and because I'm not replying to
anyone or anything in particular. Some collected thoughts roughly in
order they appear in the RFC:
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()`.
2. The `$maxParamCount` on the other hand makes more sense to me, but
even for that the number of “entries” will be at most the input length
(for `&&&`).
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.
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.
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.
6. The Uri::getQueryParams() method on the other hand makes sense to me.
It can use RFC 1866 as an implementation detail after performing the
correct percent decoding.
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=y` would be `%6g=y`
(which would then become `%256g=y`).
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).
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.
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)).
11. Bullet point 8 also applies to `deleteValue()`. Is there an actual
use case for this method?
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.
13. “doesn't start with a “with” suffix” - that should read “prefix”.
Prefix is “start with”, suffix is “end with”.
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.
That's it. It looks like a long list of items, but most of them are
minor concerns and suggestion. Overall I'm super happy with how the RFC
turned out and I believe that giving it the proper attention as a
dedicated RFC was the right choice.
Best regards
Tim Düsterhus
