Hi Stephen On 19/09/2023 09:58, Stephen Reay wrote: > > >> On 19 Sep 2023, at 14:30, Tim Düsterhus <t...@bastelstu.be> 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 >> > > Hi Tim, > > > Thanks for providing context on where this idea came from. > > > 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. >
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. > > Cheers > > Stephen Kind regards Niels -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
