Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121091
Message-ID: <b8665e13-d317-49a0-b740-b082ec6bd2c9@gmail.com>
Date: Mon, 18 Sep 2023 20:00:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Stephen Reay <php-lists@koalephant.com>
Cc: PHP Internals <internals@lists.php.net>
References: <c7a7f77b-8187-23f7-7c02-c92f1b538039@gmail.com>
 <c774ef73-802b-42eb-9181-9a080713c390@gmail.com>
 <CAAwdEzCQps8+cFf86-6e_7gCXtdhG7DjLTyQ3TS-=3+kW-Pcqg@mail.gmail.com>
 <29eb53a7-9aef-4e48-999e-97574f27a9df@gmail.com>
 <4C6FD028-4E34-4578-AF04-EDAC120E3E94@koalephant.com>
Content-Language: en-US
In-Reply-To: <4C6FD028-4E34-4578-AF04-EDAC120E3E94@koalephant.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC] [Discussion] DOM HTML5 parsing and serialization
 support
From: dossche.niels@gmail.com (Niels Dossche)

Hi Stephen

On 18/09/2023 08:46, Stephen Reay wrote:
> 
> 
>> On 17 Sep 2023, at 18:28, Niels Dossche <dossche.niels@gmail.com> wrote:
>>
>> Hi Alexandru
>>
>> On 9/17/23 11:59, Alexandru Pătrănescu wrote:
>>> On Sat, Sep 16, 2023, 02:17 Niels Dossche <dossche.niels@gmail.com> wrote:
>>>
>>>>
>>>> We'll add a common abstract base class DOM\Document (name taken from the
>>>> DOM spec & Javascript world).
>>>> DOM\Document contains the properties and abstract methods common to both
>>>> HTML and XML documents.
>>>>
>>>>
>>> Hi,
>>>
>>> Yes looks a lot better.
>>> Great work overall! And thank you for taking on this effort.
>>>
>>> I would have a small suggestion: to make the abstract class an interface.
>>> This will allow even more flexibility on how things can be build further,
>>> suggesting composition over inheritance.
>>> In user land we cannot have interfaces with properties (yet) but in php
>>> internal interfaces we have example with interface UnitEnum that has name
>>> property, extendes by BackedEnum that adds value property.
>>>
>>
>> Right, we discussed the use of an interface internally too.
>> Indeed as you suggest, we chose an abstract class over an interface because of the property limitation.
>> Looking at UnitEnum & BackedEnum (https://github.com/php/php-src/blob/bae30682b896b26f177f83648bd58c77ba3480a8/Zend/zend_enum.stub.php <https://github.com/php/php-src/blob/bae30682b896b26f177f83648bd58c77ba3480a8/Zend/zend_enum.stub.php>) I don't see the properties defined on the interface. In practice, all enums get the properties of course, just not via the interface.
>> So as we cannot represent the properties on the interfaces (yet), we'll stick with the abstract class for now.
>>
>>
>>> Thank you,
>>> Alex
>>>
>>
>> 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,
> 
> Can you expand on the reasoning for two of the decisions in your proposal? I'm not sure I really see the reason/benefit :
> 
> 1. fromX() methods are on the individual classes, rather than the parent, which as I understand it, you're using as a poor-mans interface with properties. I'd have thought that at the very least the parent should declare those methods as abstract
> 

At least the fromEmptyDocument signature differs between HTMLDocument & XMLDocument, so it's not possible to declare that method on the parent.
It's possible to define the other two on the parent. However, I choose against that because:
1) They have different behaviour if called on XML vs HTML, because they return their respective new instance. At least the other methods have the same behaviour between XML & HTML. Furthermore it would be weird to have 2 out of 3 factory methods on the parent, but the other one not.
2) It makes some sense at least to call e.g. createComment() while not knowing the concrete type of the document (e.g. when supporting both XML & HTML documents), so having that method in the parent makes sense. For example:
function addCopyright(DOM\Document $doc) {
  $doc->append($doc->createComment("(c) foo 2023"));
}
However, you're not going to call the factory methods on a variable that can be either XML or HTML document, as you wouldn't know a priori what you're getting back.

> 
> 2. Why "fromEmptyDocument()" rather than just allowing `new DOM\{XML,HTML}Document` via a public constructor?

While technically possible, I find it confusing to have both a normal constructor and factory methods.
When people see factory methods, they'll know - from experience - that every construction needs to go via factories.
Otherwise, people are going to call the constructor and search for a loadHTML/loadHTMLFile method like they do with DOMDocument.

> 
> The only mention of 'constructor' I see in the email or the RFC is the line below:
> 
>> the properties set by DOMDocument's constructor are overridden by its load methods, which is surprising
> 
> But I don't really see how making the constructor private and forcing use of a static method changes this aspect, at all - it just introduces a different way to create an instance.
> 

The problem with DOMDocument's approach is the following:
$dom = new DOMDocument(encoding: "bla");
$dom->loadXML(...); // Oops encoding gone!

With the new classes, you load the document at construction time (or no document at all).
Therefore, this confusion/API misuse is prevented by the API design.
You can see it as having 3 different constructors that disallow loading another document on the same instance afterwards.

> 
> Otherwise, it's great to see some activity on DOM handling classes.
> 
> 
> Cheers
> 
> 
> Stephen 
> 

Cheers
Niels