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

Hi Stephen On 20/09/2023 12:02, Stephen Reay wrote: > > >> On 20 Sep 2023, at 03:03, Niels Dossche <dossche.niels@gmail.com> wrote: >> >> Hi Stephen >> >> On 19/09/2023 09:58, Stephen Reay wrote: >>> >>> >>>> On 19 Sep 2023, at 14:30, Tim Düsterhus <tim@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 >> <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. > You'd still have to know the signature, and what options it accepts and affect the parser to be able to use it in a meaningful way. Anyway, you can still do this use-case without a method_exists check or something alike by using union types for example. You'll likely still want to put some kind of limitations on, so limiting via type seems sensible. > > 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. > I will list the reasons more clearly in the RFC. > > > Cheers > > > Stephen  Kind regards Niels