Hi Stephen On 20/09/2023 12:02, Stephen Reay wrote: > > >> 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 >> <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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
