Pre-RFC: Fixing spec bugs in the DOM extension

Hi

In my opinion, having them become proper classes instead of aliases has my preference: either we fix everything in one go now while we have the opportunity, or never.

As I've already told you in private, I'm in favor of using this opportunity.

Let me know what you think, especially regarding the type issues.

Will the classes be made final if they are no longer aliases? That
should (hopefully) make similar changes somewhat easier in the future.

Best regards
Tim Düsterhus

Hi internals

The DOM extension in PHP is used to parse, query and manipulate XML/HTML
documents. The DOM extension is based on the DOM specification.
Originally this was the DOM Core Level 3 specification, but nowadays, that
specification has evolved into the current "Living Specification"
maintained by WHATWG.

Unfortunately, there are many bugs in PHP's DOM extension. Most of those
bugs are related to namespace and attribute handling. This leads to people
trying to work around those bugs by relying on more bugs, or on
undocumented side-effects of incorrect behaviour, leading to even more
issues in the end. Furthermore, some of these bugs may have security
implications [1].

Some of these bugs are caused because the method or property was
implemented incorrectly back in the day, or because the original
specification used to be unclear. A smaller part of this is because the
specification has made breaking changes when HTML 5 first came along and
the specification creators had to unify what browsers implemented into a
single specification that everyone agreed on.

It's not possible to "just fix" these bugs because people actually rely
on these bugs. They are also often unaware that what they're doing is
actually incorrect or causes the internal document state to be
inconsistent. We therefore have to fix this in a backwards-compatible way:
i.e. a hard requirement is that all code written for the current DOM
extension keeps working without requiring changes.
In short: the main problem is that 20 years of buggy behaviour means that
the bugs have become ingrained into the system.

Some people have implemented userland DOM libraries on top of the existing
DOM extension. However, even userland solutions can't fully work around
issues caused by PHP's DOM extension. The real solution is to provide a
BC-preserving fix at PHP's side.

Roughly 1.5 months ago I merged my HTML 5 RFC [2] into the PHP 8.4
development branch. This RFC introduced new document classes:
DOM\HTMLDocument and DOM\XMLDocument. The idea here was to preserve
backwards compatibility: if the user wants to keep using HTML 4, they can
keep using the DOMDocument class. Also, when the user wants to work with
HTML 5 and are currently using workarounds, they can migrate on their own
pace (without deprecations or anything) to the new classes. New code can
use DOM{HTML,XML}Document from the start without touching the old classes.

The HTML 5 RFC has left us with an interesting opportunity to also
introduce the spec bugfixes in a BC-preserving way. The idea is that when
the new DOM{HTML,XML}Document classes are used, then the DOM extension
will follow the DOM specification and therefore get rid of bugs. When you
are using the DOMDocument class, the old implementations will be used. This
means that backwards compatibility is kept.

For the past 2.5 weeks I've been working on getting all spec bugs that I
know of fixed. The full list of bugs that this proposal fixes can be found
here:
https://github.com/nielsdos/php-src/blob/dom-spec-compliance-pub/bugs.md.
I also found some discussion [3] from some years ago where C. Scott shared
a list of problems they encountered at Wikimedia [4]. All behavioural
issues are fixed in my PR [5], although my PR could always use more
testing. Currently I have tested that existing DOM code does not break (I
have tested veewee's XML library, Mensbeam library, some SimpleSAML
libraries). I have added tests to test the new spec-compliant behaviour. I
also ported some of the WHATWG's WPT DOM tests (DOM spec-compliance
testsuite) to PHP and those that I've ported all pass [6].

Implementation PR can be found here:
https://github.com/php/php-src/pull/13031

Note that this is not a new extension, but an improvement to the existing
DOM extension. As for "why not an entirely new extension?", please see the
reasoning in my HTML 5 RFC. All interactions with SimpleXML, XSL, XPath etc
will remain possible like you are used to. Implementation-wise, a lot of
code internally is shared between the spec-compliant and old
implementations.

I intend to put this up for RFC. There is however one last detail that
needs to be cleared up: what about "type issues"?
To give an example of a "type issue": there is a string DOMNode::$prefix
property. DOM spec tells us that this should be nullable: when there is no
prefix for a node, the prefix should return NULL. However, because the
property is a string, this currently returns an empty string instead in
PHP. Not a big deal maybe, but there's many of these subtle
inconsistencies: null vs false return value, arguments that should accept
?string instead of string, etc.
Sadly, it's not possible to fix the typing issues for properties and
methods for DOMNode, DOMElement, ... because of BC: properties and methods
can be overridden.
Or is it?

Currently, as a result of the HTML 5 RFC, the new DOM{HTML,XML}Document
classes keep using the DOMNode, DOMElement, ... classes.
For consistency, the DOMNode etc class were aliased to the DOM namespace,
i.e. DOM\Node is an alias for DOMNode, DOM\Element an alias for DOMElement
etc.
Being an alias, this means that fixing types for DOM\Node is not possible
because it's really just another name for DOMNode, so changing it for
DOM\Node means changing it for DOMNode.
Unless we no longer alias the classes but make them proper classes
instead. This means we can fix the typing for DOM\Node while keeping
DOMNode untouched, preserving BC. The downside is that it becomes more
difficult for interoperability. One of the reasons the HTML 5 RFC
introduced aliases instead of proper classes is so that code taking a
DOMNode as an argument could also be passed a DOM\Node. However, if we make
it a proper class instead, such code has to either transition fully to the
new DOM classes or use a type union, e.g. DOMNode|DOM\Node.
In my opinion, having them become proper classes instead of aliases has my
preference: either we fix everything in one go now while we have the
opportunity, or never.

Let me know what you think, especially regarding the type issues.

Kind regards
Niels

[1] https://github.com/php/php-src/issues/8388
[2] https://wiki.php.net/rfc/domdocument_html5_parser
[3] https://externals.io/message/104687
[4] https://www.mediawiki.org/wiki/Parsoid/PHP/Help_wanted
[5] https://github.com/php/php-src/pull/13031
[6] https://github.com/nielsdos/wpt/tree/master/dom/php-out (yes, this is
a dirty port)

Thank you for the work!

I agree that making them proper classes instead of aliases is the better
proposition here.
I'm not fully informed about the DOM spec, and I don't know if the current
class/interface hierarchy is in the best shape, but maybe we should also
consider having a look a this?

About making those new classes finals, this would require reconsidering the
class hierarchy anyway, as nearly everything inherits from DOMNode, and
other classes (namely Comment/Text/CData nodes) extend other classes.
However, I would not necessarily be against it, especially if we add the
required interfaces, as the current mechanism of registering a custom class
is not very powerful and rather cumbersome to use as the constructor is
never called.
As such, I'm not sure if I would support adding the current mechanism to
customize the node classes returned by the extension. Indeed, the current
mechanism doesn't play nicely at all with static analysis and this is
something I stopped trying to integrate when writing my DocBook renderer
project. [1]

Best regards,

Gina P. Banyard

[1] https://gitlab.com/Girgias/docbook-renderer

Hi internals

The DOM extension in PHP is used to parse, query and manipulate
XML/HTML documents. The DOM extension is based on the DOM specification.
Originally this was the DOM Core Level 3 specification, but nowadays,
that specification has evolved into the current "Living Specification"
maintained by WHATWG.

Unfortunately, there are many bugs in PHP's DOM extension. Most of
those bugs are related to namespace and attribute handling. This leads
to people trying to work around those bugs by relying on more bugs, or
on undocumented side-effects of incorrect behaviour, leading to even
more issues in the end. Furthermore, some of these bugs may have
security implications [1].

Some of these bugs are caused because the method or property was
implemented incorrectly back in the day, or because the original
specification used to be unclear. A smaller part of this is because the
specification has made breaking changes when HTML 5 first came along
and the specification creators had to unify what browsers implemented
into a single specification that everyone agreed on.

It's not possible to "just fix" these bugs because people actually
rely on these bugs. They are also often unaware that what they're
doing is actually incorrect or causes the internal document state to be
inconsistent. We therefore have to fix this in a backwards-compatible
way: i.e. a hard requirement is that all code written for the current
DOM extension keeps working without requiring changes.
In short: the main problem is that 20 years of buggy behaviour means
that the bugs have become ingrained into the system.

Some people have implemented userland DOM libraries on top of the
existing DOM extension. However, even userland solutions can't fully
work around issues caused by PHP's DOM extension. The real solution is
to provide a BC-preserving fix at PHP's side.

Roughly 1.5 months ago I merged my HTML 5 RFC [2] into the PHP 8.4
development branch. This RFC introduced new document classes:
DOM\HTMLDocument and DOM\XMLDocument. The idea here was to preserve
backwards compatibility: if the user wants to keep using HTML 4, they
can keep using the DOMDocument class. Also, when the user wants to work
with HTML 5 and are currently using workarounds, they can migrate on
their own pace (without deprecations or anything) to the new classes.
New code can use DOM{HTML,XML}Document from the start without touching
the old classes.

The HTML 5 RFC has left us with an interesting opportunity to also
introduce the spec bugfixes in a BC-preserving way. The idea is that
when the new DOM{HTML,XML}Document classes are used, then the DOM
extension will follow the DOM specification and therefore get rid of
bugs. When you are using the DOMDocument class, the old implementations
will be used. This means that backwards compatibility is kept.

For the past 2.5 weeks I've been working on getting all spec bugs that
I know of fixed. The full list of bugs that this proposal fixes can be
found here:
https://github.com/nielsdos/php-src/blob/dom-spec-compliance-pub/bugs.md.
I also found some discussion [3] from some years ago where C. Scott
shared a list of problems they encountered at Wikimedia [4]. All
behavioural issues are fixed in my PR [5], although my PR could always
use more testing. Currently I have tested that existing DOM code does
not break (I have tested veewee's XML library, Mensbeam library, some
SimpleSAML libraries). I have added tests to test the new
spec-compliant behaviour. I also ported some of the WHATWG's WPT DOM
tests (DOM spec-compliance testsuite) to PHP and those that I've ported
all pass [6].

Implementation PR can be found here: https://github.com/php/php-src/pull/13031

Note that this is not a new extension, but an improvement to the
existing DOM extension. As for "why not an entirely new extension?",
please see the reasoning in my HTML 5 RFC. All interactions with
SimpleXML, XSL, XPath etc will remain possible like you are used to.
Implementation-wise, a lot of code internally is shared between the
spec-compliant and old implementations.

I intend to put this up for RFC. There is however one last detail that
needs to be cleared up: what about "type issues"?
To give an example of a "type issue": there is a string DOMNode::$prefix property. DOM spec tells us that this should be
nullable: when there is no prefix for a node, the prefix should return
NULL. However, because the property is a string, this currently returns
an empty string instead in PHP. Not a big deal maybe, but there's many
of these subtle inconsistencies: null vs false return value, arguments
that should accept ?string instead of string, etc.
Sadly, it's not possible to fix the typing issues for properties and
methods for DOMNode, DOMElement, ... because of BC: properties and
methods can be overridden.
Or is it?

Currently, as a result of the HTML 5 RFC, the new
DOM{HTML,XML}Document classes keep using the DOMNode, DOMElement, ...
classes.
For consistency, the DOMNode etc class were aliased to the DOM
namespace, i.e. DOM\Node is an alias for DOMNode, DOM\Element an alias
for DOMElement etc.
Being an alias, this means that fixing types for DOM\Node is not
possible because it's really just another name for DOMNode, so changing
it for DOM\Node means changing it for DOMNode.
Unless we no longer alias the classes but make them proper classes
instead. This means we can fix the typing for DOM\Node while keeping
DOMNode untouched, preserving BC. The downside is that it becomes more
difficult for interoperability. One of the reasons the HTML 5 RFC
introduced aliases instead of proper classes is so that code taking a
DOMNode as an argument could also be passed a DOM\Node. However, if we
make it a proper class instead, such code has to either transition
fully to the new DOM classes or use a type union, e.g.
DOMNode|DOM\Node.
In my opinion, having them become proper classes instead of aliases has
my preference: either we fix everything in one go now while we have the
opportunity, or never.

Let me know what you think, especially regarding the type issues.

Kind regards
Niels

[1] https://github.com/php/php-src/issues/8388
[2] https://wiki.php.net/rfc/domdocument_html5_parser
[3] https://externals.io/message/104687
[4] https://www.mediawiki.org/wiki/Parsoid/PHP/Help_wanted
[5] https://github.com/php/php-src/pull/13031
[6] https://github.com/nielsdos/wpt/tree/master/dom/php-out (yes, this
is a dirty port)

I am also on team "yes, let's just do it right." If that means the new classes are only 99% drop ins for the old ones, I'm OK with that. People can switch over when they're ready and do all the clean up at once.

I'm not sure about making things final. I don't know the domain space well enough to have a strong opinion at the moment, but my main concern would be ensuring that it's still extensible in reasonable ways. Eg, if I wanted to add a Web Component element to a page, I want to do that without fugly workarounds. I don't have a strong opinion at this point on what the right way to do that is.

--Larry Garfield