Hi
On 3/1/26 23:31, Máté Kocsis wrote:
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:
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?
2. 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
3. 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.
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?
5. Within the "Array API" section.
This also uses Uri\Rfc3986\UriQueryParams in the examples.
6. 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
