Re: [RFC] [Discussion] Followup Improvements for ext/uri

Hi Larry, - I really, really hate the "set" prefix on all the methods. It's a > builder object, surely the "set" is implied? I like that the "set" prefix makes all setters grouped together. For example, should we have to add some extra methods (like authority()), the build() method would appear between authority() and fragment() in IDE autocomplete lists without the set() prefix. > - It really feels like there's an interface to extract here from the > Url/UriBuilder classes. There's literally only one type-specific method > (build()). > It's the very same thing that we discussed for a very long time last time... What would be the purpose of the interface? To make the two builders interchangeable? But they produce fundamentally different URIs. Even if we don't include build(), then we still have some differences between the two implementations: - components are different: even though most components match, RFC 3986 and WHATWG URL still have a difference; notably, the userinfo component is only acknowledged by RFC 3986, on the other hand, the username and password components are only modifiable in case of WHATWG URL - validation rules are different: each implementation has their own validation rules for each component. I'll need to clarify this in the RFC, but some purely syntax based validations should be performed during the setter/wither calls (e.g. scheme cannot contain "%"), but the ones which rely on the "global state" (e.g. the host is required when the userinfo is set) should be performed by the build() method in order to avoid the temporal coupling I mentioned in the RFC. > - UriQueryParams::hasWithValue(), could that be just hasValue()? You > still need to specify the key anyway, and that's self-evident from the > signature. > Yes, I was already considering updating this name, but I was sure that someone (with 99% confidence of you) will point this out and suggest a better one. I agree that hasValue() is probably the right choice, although hasNameAndValue() would be the most technically correct name... - There's a count() method, so shouldn't Ur{i|l]QueryParams implement > Countable? > Yes, it can. I thought that implementing Countable on its own (without IteratorAggregate) was less useful, so I omitted it. Ignace suggested another approach that would allow implementing IteratorAggregate: if it happens then I'm totally fine with also implementing Countable. > - As above, there really is an interface lurking in UriQueryParams... > I have the same comment as for the builder with one small caveat: as far as I know the implementations, the biggest differences between UriQueryíParams and UrlQueryParams are how they parse the input, and how they percent-encode them during recomposition. The rest is fairly similar for now at least. I even had a brief moment when I thought that merging the two implementations into one is a good idea, but I came to the conclusion that it isn't so that the two classes have the possibility to evolve separately, if needed. So I'd follow the path of the original URI/URL debate and would not try to make the two implementations interoperable. They are only interoperable on the surface. :) > - Why both Uri getRawQueryParams() and getQueryParams()? It looks like > they would return the same value, no? (If not, that should be explained.) > This is actually already briefly explained in the RFC (but thanks to Ignace how also described this part): The difference between Uri\Rfc3986\Uri::getRawQueryParams() and > Uri\Rfc3986\Uri::getQueryParams() is that the former one passes the “raw” > (non-normalized) query string as an input when instantiating Uri\Rfc3986\Uri\UriQueryParams. > - The sort() method... should it take an optional user callback, or do we > lock people in to lexical ordering? > Only WHATWG URL specifies its behavior, and it uses basic alphanumeric sorting. Even though there's nothing that could stop us from implementing fancier sorting ways, I think it's already fine as-is. Sort() can be used to guarantee that the query components are in deterministic order, and I think that's all that we need. > - It would be quite convenient of set() and append() returned $this, > allowing them to be chained. > That's fine for me. WHATWG URL specifies their return type as void, so I went with this, but there's nothing wrong with returning $this. > - The fromArray() logic is... totally weird and unexpected and I hate it. > :-) Why can't you support repeated query parameters using nested arrays > rather than gumming up all calls with a wonky format? > Do you mean something like ["foo" => [0, 1, 2, 3]])"? I think it is indeed possible to implement what you suggested. Whenever the basic structure of the proposal settles a little bit, I'll update the implementation, and I'll try to find out a sensible behavior for arrays/objects. - It's not clear how one would start a new query from scratch, with the > private constructor. There doesn't seem to be a justification for the > private. I can't see why new UriQueryParams()->set('foo', 'bar') is a bad thing. > Yes, starting from scratch is only possible by using UriQueryParams::fromArray([]) or UriQueryParams::parse(""). But I don't have any fundamental issue with adding support for the empty constructor variant. > - Url::isSpecial() Could we come up with a better name here? "Special" > could mean anything unless you know the RFC; it feels like "real escape > string" all over again. > The "special URL" is indeed the technicus terminus that WHATWG URL uses. The RFC explains the concept briefly: The WHATWG URL specification defines some special schemes (http, https, > ftp, file, ws, wss), which have distinct parsing and serialization rules. I don't have any issues with the current name, but the only alternative I could imagine is Uri\WhatWg\Url::isSpecialScheme(). Regards, Máté