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é
