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=y` would 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é
