Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124002
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-538-g1508afaa2-fm-20240616.001-g1508afaa
Precedence: bulk
MIME-Version: 1.0
Message-ID: <dc040b64-028d-49ff-a177-23e9f054fdec@app.fastmail.com>
In-Reply-To: 
 <CAH5C8xUb1O20ZDrOQNC=ckFxHUUWSK7sw_njQQzFBd0qgQqoww@mail.gmail.com>
References: 
 <CAH5C8xUb1O20ZDrOQNC=ckFxHUUWSK7sw_njQQzFBd0qgQqoww@mail.gmail.com>
Date: Fri, 28 Jun 2024 22:14:19 +0000
To: "php internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Add WHATWG compliant URL parsing API
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: larry@garfieldtech.com ("Larry Garfield")

On Fri, Jun 28, 2024, at 8:06 PM, M=C3=A1t=C3=A9 Kocsis wrote:
> Hi Everyone,
>
> I've been working on a new RFC for a while now, and time has come to=20
> present it to a wider audience.
>
> Last year, I learnt that PHP doesn't have built-in support for parsing=20
> URLs according to any well established standards (RFC 1738 or the=20
> WHATWG URL living standard), since the parse_url() function is=20
> optimized for performance instead of correctness.
>
> In order to improve compatibility with external tools consuming URLs=20
> (like browsers), my new RFC would add a WHATWG compliant URL parser=20
> functionality to the standard library. The API itself is not final by=20
> any means, the RFC only represents how I imagined it first.
>
> You can find the RFC at the following link:=20
> https://wiki.php.net/rfc/url_parsing_api
>
> Regards,
> M=C3=A1t=C3=A9

I am all for proper data modeling of all the things, so I support this e=
ffort.

Comments:

* There's no need for UrlComponent to be backed.

* I don't understand why UrlParser is a static class.  We just had a who=
le big debate about that. :-)

There's a couple of ways I could see it working, and I'm not sure which =
I prefer:

1. Better if we envision the parser getting options or configuration in =
the future.
$url =3D new UrlParser()->parseUrl(): Url;

2. The named-constructor pattern is quite common.
$url =3D Url::parseFromString()
$url =3D Url::parseToArray();

* I... do not understand the point of having public properties AND gette=
rs/withers.  A readonly class with withers, OK, a bit clunky to implemen=
t but it would be your problem in C, not mine, so I don't care. :-)  But=
 why getters AND public properties?  If going that far, why not finish u=
p clone-with and then we don't need the withers, either? :-)

* Making all the parameters to Url required except port makes little sen=
se to me.  User/pass is more likely to be omitted 99% of the time than p=
ort.  In practice, most components are optional, in which case it would =
be inaccurate to not make them nullable.  Empty string wouldn't be quite=
 the same, as that is still a value and code that knows to skip empty st=
ring when doing something is basically the same as code that knows to sk=
ip nulls.  We should assume people are going to instantiate this class t=
hemselves often, not just get it from the parser, so it should be design=
ed to support that.

* I would not make Url final.  "OMG but then people can extend it!"  Exa=
ctly.  I can absolutely see a case for an HttpUrl subclass that enforces=
 scheme as http/https, or an FtpUrl that enforces a scheme of ftp, etc. =
 Or even an InternalUrl that assumes the host is one particular company,=
 or something.  (If this sounds like scope creep, it's because I am conf=
ident that people will want to creep this direction and we should plan a=
head for it.)

* If the intent of the withers is to mimic PSR-7, I don't think it does =
so effectively.  Without the interface, it couldn't be a drop-in replace=
ment for UriInterface anyway.  And we cannot extend it to add the interf=
ace if it's final.  Widening the parameters in PSR-7 interfaces to suppo=
rt both wouldn't work, as that would be a hard-BC break for any existing=
 implementations.  So I don't really see what the goal is here.

* If we ever get "data classes", this would be a good candidate. :-)

* Crazy idea:

new UriParser(HttpUrl::class)->parse(string);

To allow a more restrictive set of rules.  Or even just to cast the obje=
ct to that child class.

--Larry Garfield