Re: [RFC] [Discussion] DOM HTML5 parsing and serialization support

On Tue, Sep 19, 2023, at 7:30 AM, Tim Düsterhus wrote: > 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 I agree with Tim here. If you have named constructors that are all "equally common," then it's non-obvious which one should be the "unnamed default." I know that fromX() requires an X, fromY() requires a Y, but what does __construct() require? I cannot tell from the call site. If __construct() is just an alias of on of the fromX() methods, then what purpose does it serve other than confusion? In addition, although it's probably not super relevant here, static factory methods are chainable in a way that new is not. No extra parens are necessary. For classes I'm creating at hoc (rather than services that are always managed via DI), I often prefer a named constructor just for convenience. I don't see it as a Java-ism at all. Java is (in)famous for excessive layers of factory classes, which is something different than what is going on here. (How fair that reputation is in 2023, I don't know.) This is just "named constructors," which has been a reasonably common pattern in PHP land for a long time. --Larry Garfield