Hi
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.
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.
From the perspective of the user of the API, I like the symmetry
between all the named constructors:
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.
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.
We should probably rename the named constructors to include the "create"
prefix for consistency with DTI though.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
