Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126047
Precedence: bulk
MIME-Version: 1.0
References: <CAH5C8xUb1O20ZDrOQNC=ckFxHUUWSK7sw_njQQzFBd0qgQqoww@mail.gmail.com>
 <dd61999c-1ebd-4765-9add-cd8065968965@gmail.com> <CAOV5rgYZ_s1of-igLFEK7oqqh6=3HeYv0=JvvKvvjkcp0F6Q9Q@mail.gmail.com>
 <CAH5C8xV67frqOBCvLt73RM7QO86_pu40sHP5h71_kDRbKPtA8Q@mail.gmail.com> <f8e1d610-f8c5-460a-a73d-68dd37ab4636@app.fastmail.com>
In-Reply-To: <f8e1d610-f8c5-460a-a73d-68dd37ab4636@app.fastmail.com>
Date: Sun, 24 Nov 2024 21:40:07 +0100
Message-ID: <CAH5C8xW7sObyaU4=RwQvfdU0ogiWiKr_rDLgGchKkP7K8frAOg@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Add WHATWG compliant URL parsing API
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c676e40627ae9f0a"
From: kocsismate90@gmail.com (=?UTF-8?B?TcOhdMOpIEtvY3Npcw==?=)

--000000000000c676e40627ae9f0a
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 an=
d
> 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 =3D 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=C3=A1t=C3=A9

--000000000000c676e40627ae9f0a
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Larry,</div><div dir=3D"ltr"><br></div=
><div class=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left:1px solid rgb(77,77,77);padding-left:1ex">
I do have concerns about the class design, though.=C2=A0 Given the improvem=
ents to the language, the accessor methods offer zero benefit at all.=C2=A0=
 Public-read properties (readonly or otherwise) would be faster and offer n=
o less of a guarantee.=C2=A0 If you want to allow someone to extend the cla=
ss and provide some custom logic, use aviz instead of readonly and extender=
s can use hooks instead of the methods.=C2=A0 The getters don&#39;t offer a=
ny value anymore.<br></blockquote><div><br></div><div>Yes, I knew you would=
n&#39;t like my traditional style with private properties + getters... :) S=
o let me try to answer your suggestions: first of all, I believe the readon=
ly class modifier serves=C2=A0its purpose, and I definitely want to keep it=
 because it can ensure that all URI instances are=C2=A0immutable. That&#39;=
s why I cannot use property hooks,=C2=A0since they are incompatible with re=
adonly. So only the possibility of using asymmetric visibility remains: how=
ever, since extenders still cannot hook them, this idea should also be reje=
cted. Otherwise, I would consider using readonly with public read, although=
 I believe traditional methods are better suited for overriding (easier syn=
tax, decades of experience) than property hooks (my 2cents).</div><div><br>=
</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;b=
order-left:1px solid rgb(77,77,77);padding-left:1ex">
<br>
It took me a while to realize that, I think, the fromWhatWg() method is usi=
ng an in/out parameter for error handling.=C2=A0 That is an insta-no on my =
part.=C2=A0 in/out reference parameters make sense in C, maybe C++, and bas=
ically nowhere else.=C2=A0 I view them as a code smell everywhere they&#39;=
re used in PHP.=C2=A0 Better alternatives include exceptions or union retur=
ns.<br></blockquote><div><br></div><div>Yes, originally the RFC used a refe=
rence parameter to return the error during parsing. I knew it was controver=
sial, but that&#39;s what was a consistent choice with other internal funct=
ions/methods.</div><div>After your feedback, I changed this behavior to an =
union type return type:</div><div><br></div><div>public static function par=
se(string $uri, ?string $baseUrl =3D null): static|array {}<br></div><div><=
br></div><div>So that in case of failure, an array of Uri\WhatWgError objec=
ts are returned. This practice is not really idiomatic with PHP, so persona=
lly I&#39;m not sure I like it, but neither did I particularly like=C2=A0pa=
ssing a parameter by reference...</div><div><br></div><blockquote class=3D"=
gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(77=
,77,77);padding-left:1ex">
<br>
It looks like you&#39;ve removed the with*() methods.=C2=A0 Why?=C2=A0 That=
 means it cannot be used as a builder mechanism, which is plenty valuable.=
=C2=A0 (Though could be an issue with query as a string vs array.)<br>
<br></blockquote><div><br></div><div>As I answered to Dennis, they were rec=
laimed in the meanwhile.</div><div><br></div><blockquote class=3D"gmail_quo=
te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(77,77,77);p=
adding-left:1ex">
The WhatWgError looks to me like it&#39;s begging to be an Enum.<br></block=
quote><div><br></div><div>It&#39;s probably not that visible at the first g=
lance, but Uri\<span style=3D"color:rgb(191,191,191)">WhatWgError has 2 pro=
perties: an error code, and a position, so it&#39;s not feasible to make it=
 an enum. I&#39;d however create a separate=C2=A0</span><span style=3D"colo=
r:rgb(191,191,191)">Uri\</span><span style=3D"color:rgb(191,191,191)">WhatW=
gErrorCode enum containing all the error codes, so that the class constants=
 could be removed from=C2=A0</span><span style=3D"color:rgb(191,191,191)">U=
ri\</span><span style=3D"color:rgb(191,191,191)">WhatWgError, but I felt it=
&#39;s overengineering=C2=A0so I decided not to do this.</span></div><div>=
=C2=A0</div><div>Regards,</div><div>M=C3=A1t=C3=A9</div></div></div>

--000000000000c676e40627ae9f0a--