Hi
Some more bits that I found easy enough to reply to without giving the
RFC another full read right now. I hope that my answer to (4) unblocks
your work on the RFC. The other points that I didn't reply to, I'll do
at another time. I'll let you know in the emails when I think I said
something to everything that I think is worth replying to.
On 2026-10-16 14:53, Máté Kocsis wrote:
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()`.
[…]
So the real question is whether QueryParams really cannot be abused via
long strings, and whether $maxParamCount alone could mitigate those
risks?
Answered in https://news-web.php.net/php.internals/133001.
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?
For more structured handling within a PHP program, I feel that nowadays
the general sentiment is to “wrap it into a class”. In that class you
could then easily add a method that returns an array with the desired
representation, quite similar to `jsonSerializable()`. Converting all
boolean values to the same desired representation is also not
particularly complicated with something like `array_map(iif(?, 'true',
'incorrect'), $array)` where:
function iif(bool $cond, mixed $if_true, mixed $if_false) {
return $cond ? $if_true : $if_false;
}
As you know I'm a big fan of functionality that composes well, where
each individual feature does just one job - and does it well. And I
think with partial function application doing the mapping yourself can
be done fairly succinctly and does not require reimplementing the logic
for the query parameter implementation (which would also require
documentation and users to learn about this).
[…]
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.
Yes, that proposal sounds reasonable from giving it a quick thought. In
fact for boolean values the proposed signature is preferable to
`string|null`, because `string|null` makes `false` become the empty
string, and including `int` makes false become `0`. I have some small
concerns about `float` due to precision errors, but that should probably
be fine if we use the `serialize_precision`, since that would be
consistent with the rest of PHP. Including the `BackedEnum` there makes
sense to me for folks that like `BackedEnum`. And for `UnitEnum`, a
custom mapper is easy enough.
However, I'm still unsure in case of fromArray() what would be the
correct
solution that is
also as ergonomic as possible.
See above, I think. This can easily be done with a combination of
array_map() / array_filter() combined with PFA. The nice thing is that
the API will land 8.7 at the earliest, so users already had a year to
get used to PFA :-)
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.
`toList()` makes sense to me. Folks will likely consider the
tuple-format for each entry a little weird, but that should be easy
enough to explain. And the distinction to `toArray()` returning a nested
array is clear enough.
Best regards
Tim Düsterhus
