Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121106
Message-ID: <4F202884-AB40-49A2-BB5B-CF7915942E63@koalephant.com>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_5B7A19A4-3D26-41C8-B196-51F662869017"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6\))
Date: Wed, 20 Sep 2023 17:02:33 +0700
In-Reply-To: <e5e6b75c-3bd3-4b4d-b23a-1f9fa2ff7981@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
To: Niels Dossche <dossche.niels@gmail.com>
References: <c7a7f77b-8187-23f7-7c02-c92f1b538039@gmail.com>
 <c774ef73-802b-42eb-9181-9a080713c390@gmail.com>
 <CAAwdEzCQps8+cFf86-6e_7gCXtdhG7DjLTyQ3TS-=3+kW-Pcqg@mail.gmail.com>
 <29eb53a7-9aef-4e48-999e-97574f27a9df@gmail.com>
 <4C6FD028-4E34-4578-AF04-EDAC120E3E94@koalephant.com>
 <b8665e13-d317-49a0-b740-b082ec6bd2c9@gmail.com>
 <872B9392-D91A-45BC-8956-3E53B16723A6@koalephant.com>
 <39d49239-c87c-2c95-c293-f2952b5e3307@bastelstu.be>
 <C34FE3AE-B08B-44A8-A0E2-007726107C6B@koalephant.com>
 <e5e6b75c-3bd3-4b4d-b23a-1f9fa2ff7981@gmail.com>
Subject: Re: [PHP-DEV] [RFC] [Discussion] DOM HTML5 parsing and serialization
 support
From: php-lists@koalephant.com (Stephen Reay)

--Apple-Mail=_5B7A19A4-3D26-41C8-B196-51F662869017
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8



> On 20 Sep 2023, at 03:03, Niels Dossche <dossche.niels@gmail.com> =
wrote:
>=20
> Hi Stephen
>=20
> On 19/09/2023 09:58, Stephen Reay wrote:
>>=20
>>=20
>>> On 19 Sep 2023, at 14:30, Tim D=C3=BCsterhus <tim@bastelstu.be> =
wrote:
>>>=20
>>> Hi
>>>=20
>>> On 9/19/23 08:35, Stephen Reay wrote:
>>>> Regarding the private constructor: I understand the issue with the =
*old* class being confusing - but your new class doesn't have that =
issue, because there are no "loadXXX" methods: as you said, if you're =
loading an existing document, you're forced to do it via the static =
factory methods. With that change alone, there's zero risk of confusion =
in allowing direct constructor calls, because once the object is =
instantiated there is no subsequent way to load a document and =
inadvertently change the encoding.
>>>> Having a regular constructor and then one or more factory methods =
for specific cases is already a concept in PHP (i.e. =
DateTime[Immutable]'s  `createFromXXX` as well as a constructor), and =
IMO needing to call a factory method to get an "empty" document seems =
out of place in PHP - it seems a bit like a Java-ism - using a factory, =
where one just isn't required.
>>>=20
>>> I was one of the persons who discussed this updated API with Niels =
in private and looking up the discussion it was me who proposed making =
the constructor private and just providing named constructors.
>>>=20
>>> =46rom the perspective of the user of the API, I like the symmetry =
between all the named constructors:
>>>=20
>>> Whenever I want to create a new document, I use one of the fromXyz() =
methods. And when I use those, I get exactly what it says on the tin.
>>>=20
>>> Making the regular constructor available for use would effectively =
make whatever that constructor does a special case / the default case. =
This makes sense for DateTimeImmutable, because the __construct() =
variant is likely much more often used than the various createFromXyz() =
variants. For the HtmlDocument I find it less obvious that creating an =
empty document would be the default case compared to loading an existing =
document from a file.
>>>=20
>>> We should probably rename the named constructors to include the =
"create" prefix for consistency with DTI though.
>>>=20
>>> Best regards
>>> Tim D=C3=BCsterhus
>>>=20
>>=20
>> Hi Tim,
>>=20
>>=20
>> Thanks for providing context on where this idea came from.
>>=20
>>=20
>> The constructor + specialised factory methods pattern is not exactly =
new in PHP (e.g. it took about 3 minutes to find multiple Symphony =
classes doing this), and disabling the public constructor for purely =
cosmetic reasons sounds like a very weird, and ironic choice to me, when =
the stated goal is to make the API *less surprising* than the previous =
one.
>>=20
>=20
> Besides the points that have been mentioned by Tim and Larry, there's =
also the expectation of the programmer that migrates to the new classes =
to take into account.
> They're used to calling the constructor on the old class and calling a =
load method afterwards.
> As there is still a constructor, but no load method, this might be =
confusing for them.
> So in my opinion, disabling it makes it less surprising than the =
previous one.
> Also, just because Symfony does this doesn't mean it's automatically =
something we should follow.
>=20
>>=20
>> Cheers
>>=20
>> Stephen=20
>=20
> Kind regards
> Niels
>=20
> --=20
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php

Hi Niels, Larry

To clarify my earlier message about having the factory methods on the =
faux-interface - when I said meta programming that was probably the =
wrong term to use, so I apologise for the confusion.=20
It's true that the *developer* needs to know the expected type of the =
string being parsed. It's not true that the code directly invoking the =
'fromString' method necessarily needs to know - passing classnames as a =
string and calling static methods on the string is also an established =
concept in PHP, which works really well when the method being called is =
defined on a single top-level class or interface.
A real world example I had in mind, is a Request (e.g. a curl request, =
or some other http-fetching) class that can decode responses into native =
PHP objects, with options for the callee to define the object type =
that's wanted; with just the two provided classes there's already double =
the checks required to know if the specified class name is one the =
library knows how to use. If a hypothetical class to handle SVG, RSS, =
ODF (or any other specialised use of XML) class uses the faux-interface =
from this RFC as it's base, there's no way for the Request class to know =
that it can instantiate an instance from a string, without falling back =
to method_exists() checks.


The only reason I mentioned the DateTime or Symphony classes at all, is =
to highlight that people have been using classes with a regular =
constructor, and one or more static factory methods, for years, and if =
there has been any confusion about it thus far, it's not something I've =
ever heard complaints about, nor seen efforts to address.


Calling arbitrarily named static methods "named constructors" doesn't =
really make a convincing argument when someone suddenly realises that =
the names should be something else halfway through the same =
conversation. That doesn't mean there's something wrong specifically =
with having static factory methods when they're needed, but that's my =
whole point: the *default* way to instantiate an object in PHP is =
through its constructor, and classes that *don't* provide a public =
constructor are *much* less common, usually with an obvious reason why =
(e.g. they're a singleton, or they're objects migrated from resource =
handles).


I've had enough of these conversations to realise that you've made a =
decision and you're sticking to it, regardless of what I might write =
(well except for changing the completely arbitrary names, I guess), so I =
won't continue trying to make my point to you.

Please at least do one thing though: make it *clear* in the RFC that the =
public constructor is removed because ... well I don't know, pick one of =
the reasons you've thrown up I guess. But right now the only reason even =
vaguely mentioned in the RFC is that it prevents the "changed encoding" =
issue, but it doesn't do that, the lack of a load method achieves that, =
and IMO the lack of a regular constructor is a significant enough change =
from most PHP classes, that people should be aware of that when reading =
and voting on your proposal.



Cheers


Stephen=20=

--Apple-Mail=_5B7A19A4-3D26-41C8-B196-51F662869017--