> On 20 Sep 2023, at 03:03, Niels Dossche <dossche.ni...@gmail.com> wrote: > > 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
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. 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
