Hi,
> 1. Query setters and types of their parameter. Quote from the builder > example: > > > > ``` > > ->setQuery*(*"a=1&b=2"*])* > > ->setQueryParams*([*"a" => 1, "b" => 2*])* *// Has the same effect as > the setQuery() call above* > > ``` > > > > I’d expect the `setQueryParams` to set the particular params that I > specified instead of overriding all the query (which I’d expect `setQuery` > to do). > I'm happy that you raised this concern because I wouldn't ever have thought the behavior was unclear. My expectation with setters is that they completely override the related property. If they were intended to add/append the query params then something like addQueryParams() should be used instead. And I think this question has just highlighted why it's a bad idea to remove the set prefix from the Builder methods: because it would also remove some additional context about what the method exactly does (which is apparently not even entirely clear with the set prefix included, but without the set prefix, one would have zero clue if it's an append or set operation). > Maybe it would be possible for `setQuery` to accept string, array and > instances of `*QueryParams`? And `setQueryParams` could either be left out > or override just the selected params? As far as I see, there is no > equivalent `withQueryParams` so there’s no precedent on how such a method > should work, right? > Good call, I've already updated the RFC so that setQueryParams() accepts a UriQueryParams/UrlQueryParams instance. It was just an oversight on my part. But I don't really like to use union types (mainly because the method behavior needs extra explanation), that's why I chose to add two distinct methods for the query handling. > Btw I wasn’t able to find docs on the existing Uri classes (had to look it > up in the prev RFC). Is the search broken or are the docs just not there > yet? > Yes, unfortunately, the docs are not there yet :( I usually prioritize my work in favor of php-src, and since I apparently chose a massive undertaking yet again, I have very limited time for anything else. But I'll try to find time to add the missing stuff to the documentation. > 2. Regarding the interface extraction, is there any difference between the > internal states of `Uri\WhatWg\UrlQueryParams` and > `Uri\Rfc3986\UriQueryParams` objects? I would naively expect not only a > common interface, but a common class. A `QueryParams` that could be > supplied to any of the withers/setters and that would be able to > `->toRfc3986QueryString()` or `->toWhatWgQueryString()` on use not on > instantiation. > I have already been thinking a lot about this question for a while, and I agree that the two QueryParams implementations are very similar: the only difference between them is how they parse the query string into query params and how they recompose the query params to a query string. So I would be happy to be able to unify the two implementations, that would be a huge simplification. However, there are two reasons why I'm very hesitant to do so: - We should be absolutely sure that an unified implementation cannot cause parsing confusion vulnerability. E.g. when the query params are parsed according to one specification, and then they are recomposed according to the other one. For example, one difference (this is also mentioned in the RFC) is that WHATWG URL removes the leading "?" character during parsing, while RFC 3986 leaves it as-is. These differences must be considered very carefully. - If we have two dedicated classes, then they can evolve separately with specification-specific behavior. If it turns out that we want to add support for some specification-specific feature, then a specification-specific class is better suited for the purpose. I can't really come up with many examples, but maybe additional percent-encoding capabilities could be needed (e.g. getFirstPercentEncoded()). > Similarly with the builders I fail to see why do I need to decide on > `Uri\Rfc3986\UriBuilder` and `Uri\WhatWg\UrlBuilder` at the start instead > of having `Uri\Builder` that could be consumed via `->buildRfc3986Url()` or > `->buildWhatWgUri(). Is that UserInfo thing the whole dealbreaker? To me > all the other parts like specifying host, port and so on seem spec-agnostic > until serialization. > Yes, the userinfo is just one of the deal breakers. Not too long ago, I modified the RFC text, and added more info about how validation exactly works. According to my plans, the individual setters would also make some basic validation (formatting of the component), while the build() methods would make sure that some global rules are also satisfied (there's a bit more info in the RFC about this). And this is only possible to implement if there are two different builders. Máté
