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

Hey Christian

Thank you for going through my proposal.

Am 02-Sep-2023 21:41:50 +0200 schrieb dossche.niels@gmail.com:

Hello internals

I'm opening the discussion for my RFC "DOM HTML5 parsing and serialization support".
https://wiki.php.net/rfc/domdocument_html5_parser

Kind regards
Niels

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi Niels,

thank you for your proposal and your work on this.

This proposal introduces the DOM\HTML5Document class that extends the
DOMDocument class. The reason we introduce a new class instead of replacing
the methods of the existing class is to ensure full backwards compatibility.

Although I do not dislike your idea with a new class for HTML5 parsing I have one question. Why not make the decision, which parser to use, dependent from the doctype declaration at the start of the html document?
There's three major reasons to not depend solely on the doctype:

1. Not all documents have a doctype. So in that case, do we have to pick the old parser or the HTML5 parser?
2. People create DOM documents from scratch, without loading some code or file first.
   In that case, we can't choose upfront if the document is an HTML5 document or a legacy document.
   I believe that by using a class, and thus having a clear choice upfront, this minimizes surprises.
3. Having a separate class allows us to use the type system to restrict some users to only allow HTML5 documents.

Best regards
Christian

Kind regards
Niels

Hi Dennis

Thanks for the proposal Niels,

I’ve dealt with my own grief working through issues in DOMDocument and wanting it to work but finding it inadequate.

HTML5

This would be a great starting point; I would love it if we took the opportunity to fix named character reference decoding, as PHP has (to my knowledge) never respected (at least in HTML5) that they decode differently inside attributes as they do inside markup, considering rules such as the ambiguous ampersand and decode errors.

It’s also been frustrating that DOMDocument parses tags in RCDATA sections where they don’t exist, such as in TITLE or TEXTAREA elements, escapes certain types of invalid comments so that they appear rendered in the saved document, and misses basic semantic rules (e.g. creating a BUTTON element as a child of a BUTTON element instead of closing out the already-open BUTTON).

With this proposal: a real HTML5 parser, these above mentioned problems will fortunately be a problem from the past :)

I’d like to share some what a few of us have been working on inside WordPress, which is to build a conformant streaming HTML5 parser:
 - https://developer.wordpress.org/reference/classes/wp_html_tag_processor/ https://developer.wordpress.org/reference/classes/wp_html_tag_processor/
 - https://make.wordpress.org/core/2023/08/19/progress-report-html-api/ https://make.wordpress.org/core/2023/08/19/progress-report-html-api/

It’s just food for thought right now because adding HTML5 support to DOMDocument would benefit everyone, but we decided we had common need in PHP to work with HTML not in a DOM, but in a streaming fashion, one with very little runtime overhead. My long-term plan has been to get a good grasp for the interface needs and thoroughly test it within the WordPress community and then propose its inclusion into PHP. It’s been incredibly handy so far, and on my laptop runs at around 20 MB/s, which is not great, but good enough for many needs. My naive C port runs on the same laptop at around 80 MB/s and I believe that we can likely triple or quadruple that speed again if any of us working on it knew how to take advantage of SIMD instrinsics.

It tries to accomplish a few goals:
 - be fast enough
 - interpret HTML as an HTML5-compliant browser will
 - find specific locations within an HTML document and then read or modify them
 - pass through any invalid HTML it encounters for the browser to resolve/fix unless modifying the part of the document containing those invalid constructions

I've seen someone link this on Reddit today, it's a really nice project!
It reminds me of Cloudflare's lol-html, which is also a streaming parser used to modify and sanitize documents linearly.
I believe this could be a great addition, it solves a different problem that the ext/dom extension solves. So I think it would be a great complementary addition.

I only bring up this different interface because once we started digging deep into DOMDocument we found that the problems with it were far from superficial; that there is a host of problems and a mismatched interface to our common needs. It has surprised me that PHP, the language of the web, has had such trouble handling HTML, the language of the web, and we wanted to completely resolve this issue once and for all within WordPress so we can clean up decades’ old problems with encoding, decoding, security, and sanitization.

Yes, I was also quite surprised of the lacking support for modern web features, and also the problems with spec compliance.
I only recently got into maintaining ext/dom. So there's still a lot of work to do.
I had already started with adding more DOM APIs in the 8.3 release cycle and plan to continue that effort in 8.4.
Another major project I want to do for 8.4, besides HTML5 support, is fixing the spec compliance issues in an opt-in manner. This would help with security & sanitization problems (HTML5 should help with the encoding&decoding).

Warmly,
Dennis Snell

Kind regards
Niels

I'm opening the discussion for my RFC "DOM HTML5 parsing and serialization support".
https://wiki.php.net/rfc/domdocument_html5_parser https://wiki.php.net/rfc/domdocument_html5_parser

Kind regards
Niels

Hello internals

I'm opening the discussion for my RFC "DOM HTML5 parsing and serialization support".
https://wiki.php.net/rfc/domdocument_html5_parser

Kind regards
Niels

Hi internals

I'd like to announce a change to the RFC. The new RFC version is 0.5.1,
the old one was 0.4.0.
The diff can be viewed via the revision history button on the right.

I had a productive discussion with Tim and Arne about the class hierarchy.
Here's a summary of the changes and the rationale.

Until now, the RFC specified that DOM\HTML5Document extends DOMDocument.
However, as we're introducing a new class anyway, we believe we should
take the opportunity to improve the API.
We have the following concerns:
a) It's a bit of an awkward class hierarchy. If we hypothetically
would want to get rid of DOMDocument in the far far future, we can't
easily do that.
b) API is messy. Some methods are useless for HTML5Document. E.g.:
validate(), loadXML(), loadXMLFile(). They can be a source of confusion.
c) The fact that you can pass HTML5Document to methods accepting
DOMDocument may result in unexpected behaviour when the method expects
a particular behaviour. It would be better if developers could "opt-in"
to accepting both DOMDocument and HTML5Document in a method using a
common base class.
d) The properties set by DOMDocument's constructor are overridden by
load methods, which is surprising. That's even mentioned as the second
top comment on https://www.php.net/manual/en/domdocument.loadxml.php.
Furthermore, the XML version argument of the constructor is even
useless for HTML5 documents.

So we propose the following changes to the RFC.

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.
Examples of what it includes/excludes:

• includes: firstElementChild, lastElementChild, ...
• excludes: xmlStandalone, xmlVersion, validate(), ...
  Then we'll have two subclasses: DOM\HTMLDocument (previously we called
  this DOM\HTML5Document) and DOM\XMLDocument. We dropped the 5 from the
  name to be more resilient to version changes and match the DOM spec
  name.
  DOMDocument will also use DOM\Document as a base class to make it
  interchangeable with the new classes.

The above would solve points a, b, and c.
To solve point d, we can use "factory methods":
This means HTMLDocument's constructor will be made private, and instead
we'll have three static methods that create a new instance:

• HTMLDocument::fromHTMLString(string $xml): HTMLDocument;

That should be string $html, yes?

• HTMLDocument::fromHTMLFile(string $filename): HTMLDocument;
• HTMLDocument::fromEmptyDocument(string $encoding="UTF-8"):
  HTMLDocument;

Or to put it in PHP code:

namespace DOM {
	// The base abstract document class
	abstract class Document extends DOM\Node implements DOM\ParentNode {
		/* all properties and methods that are common and sensible for both 
XML & HTML documents */
	}
  
	class XMLDocument extends Document {
		/* insert specific XML methods and properties (e.g. xmlVersion, 
validate(), ...) here */

		private function __construct() {}
  	
		public static function fromEmptyDocument(string $version = "1.0", 
string $encoding = "UTF-8");
		public static function fromFile(string $path);
		public static function fromString(string $source);
	}
  
	class HTMLDocument extends Document {
		/* insert specific Html methods and properties here */

		private function __construct() {}
  	
		public static function fromEmptyDocument(string $encoding = "UTF-8");
		public static function fromFile(string $path);
		public static function fromString(string $source);
	}
}

class DOMDocument extends DOM\Document {
	/* Keep methods, properties, and constructor the same as they are now */
}

We're only adding XMLDocument for completeness and API parity. It's a
drop-in replacement for DOMDocument, and behaves the exact same.
The difference is that the API is on par with HTMLDocument, and the
construction is designed to be more misuse-resistant.
DOMDocument will NOT change, and remains for the foreseeable future.

We also have to change the $ownerDocument field in DOMNode to have type
?DOM\Document instead of ?DOMDocument.
Problem is that this breaks BC (but only a minor break):
https://3v4l.org/El7Ve.
Overriding properties is kind of useless, but if someone does it, then
the compiler will complain loudly during compilation and it should be
easy to fix.

Of course, these changes means that the discussion period will run a
bit longer than originally foreseen.

Kind regards
Niels

This all makes sense to me, and I like this as a way forward. Nice work!

--Larry Garfield

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.

Thank you,
Alex

Hi

[…]

Thank you for the fruitful discussion. This updated API is much better.

When reading the updated RFC yesterday, I initially wanted to complain
about the "The options argument" section still existing, but the
$options argument nowhere to be seen. But I see that you now added it.

One thing I now note that could also be improved, now that we make some
larger improvements to the API:

The $options argument still takes an integer and expects the developer
to use the bitwise-or operator to combine multiple constants to set the
appropriate flags. Despite this being a commonly used pattern in the
standard library, I think it provides for a bad developer experience and
makes it harder to extend the list of options for non-boolean options. I
would never design an API like this when creating a userland library.

I have two suggestions that might or might not be better:

1. Make the options explicit arguments with a default value and expect
   the user to leverage named arguments to override specific arguments, e.g.

   HtmlDocument::fromString(
   string $string,
   bool $ignoreErrors = false, // LIBXML_NOERROR
   bool $reducedMemoryUsage = false, // LIBXML_COMPACT
   bool $impliedTags = true, // LIBXML_HTML_NOIMPLIED
   );

This would provide for improved discoverability and the options checking
would be implicit and easily available in IDEs.

2. Make the option parameter an array that takes an array of flags. This
   could either be an associative array (like used with unserialize,
   setcookie or password_hash) or a list of self-descriptive options. The
   latter would allow for a clean(er) migration to tagged-unions if/when
   they make it into PHP.

Example usage:

 HtmlDocument::fromString(
   '<html></html>',
   [
     LIBXML_NOERROR,
     LIBXML_COMPACT,
     LIBXML_HTML_NOIMPLIED,
   ],
 );

With tagged-unions it could look like this in a future version, note the
HtmlVersion option that takes a parameter.

 HtmlDocument::fromString(
   '<html></html>',
   [
     Dom\LoadingOption::IgnoreError,
     Dom\LoadingOption::ReducedMemoryUsage,
     Dom\LoadingOption::NoImpliedTags,
     Dom\LoadingOption::HtmlVersion(5),
   ],
 );

Even if not yet reaping the full benefits of the "list of options", I
think it more naturally splits across multiple lines compared to
'bitwise-or'ing and it can also be more useful in stack traces for error
handlers that are able to handle arrays, because there won't be some
non-descript integer you have to decode manually.

What do you think?

Best regards
Tim Düsterhus

I'm opening the discussion for my RFC "DOM HTML5 parsing and
serialization support".
https://wiki.php.net/rfc/domdocument_html5_parser

Some minor changes after a discussion with Tim:

• The old class names are now written with a leading backslash to
  emphasize they are in the global namespace. Hopefully this will resolve
  confusion around them.

• Fixed unclear wording, i.e. "type hint" -> "type declaration"

• The alias for DOMException will be DOM\DOMException. Because that's the
  official name per DOM spec, and otherwise importing it and using it
  would be confusing with the global namespace <php>Exception</php> class
  (see also
  https://github.com/php/php-src/pull/9071#issuecomment-1193162754 for
  precedent).

I have just read the RFC, and I find it both well written and compelling
to accept. +1 from me.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support
Host of PHP Internals News: https://phpinternals.news

mastodon: @derickr@phpc.social @xdebug@phpc.social
twitter: @derickr and @xdebug

Hi Niels,

Hi internals

Discussion seems to have died down.
Today, it's been 14 days since the last major change was done to the RFC (i.e. the class hierarchy update).
And it's also been close to 4 weeks since I first announced the RFC it on the mailing list.
I'd like to start the vote on Monday (20:00 PM GMT+2) and I intend to let it run for 2 weeks.
Any final complaints should be raised now.

Not much to complain but a question - not sure if it was discussed before.

Naming: XMLDocument::fromEmpty vs. HTMLDocument::createEmpty in the
PHP code section.

For both, XMLDocument::fromEmpty and HTMLDocument::createEmpty there
is an argument available to define the encoding but none of the other
createFrom* methods have this argument.

As far as I understand, in the these other cases the encoding gets
detected from the content of the passed source but what happens is the
source does not contain any information about the encoding?. E.g. you
load an XML/HTML document over HTTP, the encoding is defined via HTTP
header but the content itself doesn't contain it.

Kind regards
Niels

Best,
Marc

Hello internals

I'm opening the discussion for my RFC "DOM HTML5 parsing and serialization support".
https://wiki.php.net/rfc/domdocument_html5_parser

Kind regards
Niels

Hi internals

Discussion seems to have died down.
Today, it's been 14 days since the last major change was done to the
RFC (i.e. the class hierarchy update).
And it's also been close to 4 weeks since I first announced the RFC it
on the mailing list.
I'd like to start the vote on Monday (20:00 PM GMT+2) and I intend to
let it run for 2 weeks.
Any final complaints should be raised now.

Kind regards
Niels

From the RFC:

\DOMDocument will also use DOM\Document as a base class to make it interchangeable with the new classes. We're only adding XMLDocument for completeness and API parity. It's a drop-in replacement for \DOMDocument, and behaves the exact same. The difference is that the API is on par with HTMLDocument, and the construction is designed to be more misuse-resistant. \DOMDocument will NOT change, and remains for the foreseeable future.

Would it make sense then for one of \DOMDocument and DOM\XMLDocument to extend the other, then? So that, eg, we can type against DOM\XMLDocument and then support both old and new classes? Or are the construction et al differences enough that is not viable?

Similarly, the constants would lose their DOM_ prefix in the namespace version, e.g. DOM\INDEX_SIZE_ERR will be an alias for DOM_INDEX_SIZE_ERR. For constants that begin with XML_ I propose to keep the prefix.

Unclear to me: Would the XML constants also be aliased into the namespace verbatim, or left globally?

Did you consider making the new classes throw exceptions rather than forcing people to remember to call another "was there an error" global function like it's still 1996? :-)

Otherwise looks good to me. Thanks!

--Larry Garfield