Hi Larry,
I do have concerns about the class design, though. Given the improvements
> to the language, the accessor methods offer zero benefit at all.
> Public-read properties (readonly or otherwise) would be faster and offer no
> less of a guarantee. If you want to allow someone to extend the class and
> provide some custom logic, use aviz instead of readonly and extenders can
> use hooks instead of the methods. The getters don't offer any value
> anymore.
>
Yes, I knew you wouldn't like my traditional style with private properties
+ getters... :) So let me try to answer your suggestions: first of all, I
believe the readonly class modifier serves its purpose, and I definitely
want to keep it because it can ensure that all URI instances are immutable.
That's why I cannot use property hooks, since they are incompatible with
readonly. So only the possibility of using asymmetric visibility remains:
however, since extenders still cannot hook them, this idea should also be
rejected. Otherwise, I would consider using readonly with public read,
although I believe traditional methods are better suited for overriding
(easier syntax, decades of experience) than property hooks (my 2cents).
> It took me a while to realize that, I think, the fromWhatWg() method is
> using an in/out parameter for error handling. That is an insta-no on my
> part. in/out reference parameters make sense in C, maybe C++, and
> basically nowhere else. I view them as a code smell everywhere they're
> used in PHP. Better alternatives include exceptions or union returns.
>
Yes, originally the RFC used a reference parameter to return the error
during parsing. I knew it was controversial, but that's what was a
consistent choice with other internal functions/methods.
After your feedback, I changed this behavior to an union type return type:
public static function parse(string $uri, ?string $baseUrl = null):
static|array {}
So that in case of failure, an array of Uri\WhatWgError objects are
returned. This practice is not really idiomatic with PHP, so personally I'm
not sure I like it, but neither did I particularly like passing a parameter
by reference...
> It looks like you've removed the with*() methods. Why? That means it
> cannot be used as a builder mechanism, which is plenty valuable. (Though
> could be an issue with query as a string vs array.)
>
>
As I answered to Dennis, they were reclaimed in the meanwhile.
The WhatWgError looks to me like it's begging to be an Enum.
>
It's probably not that visible at the first glance, but Uri\WhatWgError has
2 properties: an error code, and a position, so it's not feasible to make
it an enum. I'd however create a separate Uri\WhatWgErrorCode enum
containing all the error codes, so that the class constants could be
removed from Uri\WhatWgError, but I felt it's overengineering so I decided
not to do this.
Regards,
Máté
