Added masterminds/html5

[bytestream]

Resolves #828

[oliverklee]

Is this valid HTML?

[bytestream]

Depends on the DOCTYPE. It's not valid XHTML, it's valid HTML:

Attributes are placed inside the start tag, and consist of a name and a value, separated by an "=" character. The attribute value can remain unquoted if it doesn't contain ASCII whitespace or any of " ' ` = < or >. Otherwise, it has to be quoted using either single or double quotes. The value, along with the "=" character, can be omitted altogether if the value is the empty string.

<!-- empty attributes -->
<input name=address disabled>
<input name=address disabled="">

<!-- attributes with a value -->
<input name=address maxlength=200>
<input name=address maxlength='200'>
<input name=address maxlength="200">

https://html.spec.whatwg.org/multipage/introduction.html#intro-early-example

[JakeQZ]

I see no reason to change this, the RNIB recommendation is alt="". We can add a separate test for alt. We should not change the tests to allow the code to pass; we should fix the code to pass the tests - unless the tests are demonstrably wrong ;)

[JakeQZ]

I’ve looked into this a bit more.

Earlier versions of the HTML5 spec permitted the empty attribute syntax only for Boolean attributes. Later versions still don’t explicitly state that it is allowed for non-Boolean attributes, but they no longer say it is not (the specific clause, “This syntax is permitted only for boolean attributes,” has been removed).

Given that email clients are some way behind in their support for the specs (if they weren’t there’d be no need for Emogrifier), and many are probably still parsing HTML as HTML 4.01 or earlier, I would be reticent about outputting non-Boolean attributes with the empty attribute syntax, as it may cause issues on some clients.

Masterminds/html5-php actually has rules defining non-Boolean attributes to be output with ="" but these do not kick in when the disable_html_ns option is set. But if that option is not set, an xmlns attribute is set on the root html element and our XPath expressions fail to match elements by type.

It would be possible to extend OutputRules to achieve the desired behaviour (which could also help in other cases, e.g. preventing "\r" in the output on Windows); to do so would also require extending HTML5 to replace the save method to instantiate an extended OutputRules.

I note that the test that has picked this up is unrelated, which suggests some more tests in this area may be pertinent – although in some ways they would really be testing the behaviour of a third-party component (ext-dom or Masterminds\html5-ptp), albeit indirectly.

PS. DOMDocument preserves whether non-Boolean or unrecognized attributes were specified with an explicit empty string or empty attribute syntax, apparently through DOMAttr having a DOMText child or not. Recognized Boolean attributes are always output with the empty attribute syntax.

[bytestream]

Masterminds/html5-php actually has rules defining non-Boolean attributes to be output with ="" but these do not kick in when the disable_html_ns option is set.

How much did you dig into that? Enough to submit a PR to them? Doesn't seem too obvious to me how disable_html_ns affects the code from a quick glance.

[JakeQZ]

OutputRules::nonBooleanAttribute decides this.

If there is no namespace, it always returns false, which means the attribute is considered Boolean and we get the empty attribute syntax.

However, if there is a namespace, as I'm sure you've found, our XPath expressions always fail because, e.g. xmlns:p != p...

[oliverklee]

Hi @bytestream, thanks for the PR! ❤️

Could you please fix the PHPMD warning? Thanks!

[bytestream]

Hi @bytestream, thanks for the PR! ❤️

Could you please fix the PHPMD warning? Thanks!

Thanks for checking! I'll look at this soon

[JakeQZ]

This looks good and substantial. Thanks for putting the effort in. I haven't been through with a fine-toothed comb, the new class I have barely looked at at all.

I have made a few comments.

I also wonder if we need some more test cases - some that are HTML4 or XHTML and some HTML5?

[JakeQZ]

I see no reason to change this, the RNIB recommendation is alt="". We can add a separate test for alt. We should not change the tests to allow the code to pass; we should fix the code to pass the tests - unless the tests are demonstrably wrong ;)

[JakeQZ]

Incidentally, PHP8 promises some DOM improvements: https://wiki.php.net/rfc/dom_living_standard_api

[oliverklee]

I also wonder if we need some more test cases - some that are HTML4 or XHTML and some HTML5?

Yes, please. Particularly, I'd strongly prefer we get the new parser class in as a separate PR first (with good and complete tests).

[JakeQZ]

I'd strongly prefer we get the new parser class in as a separate PR first (with good and complete tests).

We have some tests already in AbstractHtmlProcessorTest for adding missing DOCTYPE, <html>. <head> and <body> though I agree we also need to test the new class for this (and anything else it does that is testable). To avoid duplication, I wonder if those tests (or the common part of them) could be moved to a trait or suchlike?

I also agree with breaking this down into smaller PRs, and the new class seems a good place to start.

[bytestream]

@oliverklee @JakeQZ what needs to happen to get this moving again?

[oliverklee]

what needs to happen to get this moving again?

@bytestream I propose adding the new class (without using it) and unit tests for it in a separate PR, and then (after that PR is merged) we can make this PR about using the new class.

[bytestream]

I've sent it upstream. Lets see if they accept it there, otherwise I'll send another PR here.

[JakeQZ]

Hi @bytestream, thanks for following-up on this.

I’ve committed a couple of changes to address code changes that were required to the tests as part of this PR. The remaining issue pertains to the empty attribute syntax, for which I have created #867 for discussion of the best way forward.

I’m also wondering about testing both the HTML5 code paths and HTML4 code paths (i.e. when the DOCTYPE is explicitly HTML4 or earlier) with the same set of tests (that we already have), as these will diverge with the introduction of Masterminds/html5-php and maybe there’s a neat way of doing that…

[bytestream]

I'll look at sending the new class your way in a separate PR tomorrow. Maintainers of masterminds/html5-php seem to be against the idea of merging my PR on the principle that they've long argued for many years that "the lib is only intended to parse decent html"

[JakeQZ]

"the lib is only intended to parse decent html"

As opposed to well-formed and perfectly valid HTML?

[oliverklee]

Could this go the approach symfony took? Rather than adding html5 as a requirement, add it as a suggestion and only use it if the class exists.

Yes, that sounds feasible, and I would be okay with that.

However, I'd like to change this to a configuration option (or some other deliberate way to trigger this, e.g. by injecting the HTML parser). I wouldn't like Emogrifier automatically change its behavior in a project if some other dependency pulls in masterminds/html5 during an update.

I'd still like the tests to be run for both code paths as much as possible, but would be happy for certain tests to be skipped where they would fail for the masterminds case.

Even better: We can have different tests for both cases, or a flexible way to test this (which might be too complex - I haven't fully though this through).

[JakeQZ]

However, I'd like to change this to a configuration option (or some other deliberate way to trigger this, e.g. by injecting the HTML parser). I wouldn't like Emogrifier automatically change its behavior in a project if some other dependency pulls in masterminds/html5 during an update.

I agree. I think I alluded to that concern in my previous comment, and this suggestion would satisfactorily allay that concern for me.

Even better: We can have different tests for both cases, or a flexible way to test this (which might be too complex - I haven't fully though this through).

I have now remembered what the issue with PHPUnit was that meant that the simple change I referenced above (to run the TestCase twice with different parameters) would not be quite adequate: the dataProviders are executed beforehand and thus the provided data seems rather fixed and can't reflect the requirements for the particular run. It might be worth opening this up to a wider audience, or even asking Sebastian himself if he has any suggestions. Because I think what we basically want to do is run the TestCase multiple times (well, twice) with different parameters, and have those different parameters reflected in the provided data (i.e. we can skip some datasets that we know will fail for a particular configuration).

[bytestream]

@JakeQZ it sounds like https://phpunit.readthedocs.io/en/9.2/annotations.html#requires is what you're after? If it needs to be done for individual cases within a @dataProvider then I would think that's as simple as not returning the case in the data provider function based on some condition.

However, I'd like to change this to a configuration option (or some other deliberate way to trigger this, e.g. by injecting the HTML parser). I wouldn't like Emogrifier automatically change its behavior in a project if some other dependency pulls in masterminds/html5 during an update.

Just an enableHtml5() method?

public function enableHtml5()
{
    if (! class_exists(Html5::class)) { throw new Exception("Add masterminds/html5 as a project dependency"); }

    $this->html5 = true;
}

[bytestream]

Any feedback on the above?

[oliverklee]

ping @JakeQZ

[JakeQZ]

Any feedback on the above?

ping @JakeQZ

Not sure what you are waiting for from me here.

I thought we had agreed for this to be implemented as an option that must be specifically enabled (i.e. not just a class_exists() test), and that the tests would be dualled up, so that the same set of tests would run for both options.

There are a few testing-related bits and bobs we still ought to implement, e.g. in the area of valueless attributes, but I don't think the lack of those is necessarily blocking this – it just could make it slightly more awkward because we would have to disable some tests for the Masterminds case. But we would have to disable some tests for the Masterminds case anyway.

@JakeQZ it sounds like https://phpunit.readthedocs.io/en/9.2/annotations.html#requires is what you're after? If it needs to be done for individual cases within a @dataProvider then I would think that's as simple as not returning the case in the data provider function based on some condition.

Oh, sorry, were you waiting for me to reply to this? I kind of read it as a comment, and thought "that sounds good" - to myself :/

Um, that sounds good. If it works, and solves the dual testing issue with the ability to vary the provided data based on with/without Masterminds (which we need because, AFAICS, we need to drop specific datasets depending on whether Masterminds is used), then, yes please, let's go for it.

I'm prepared to accept that certain tests will fail in the Masterminds case (you could also add tests that would fail conversely), as long as it's clear, and the issues involved are minor if not trivial or non-consequental (as I think most probably are).

Look forward to a PR... :)

[JakeQZ]

Look forward to a PR... :)

PS. You could probably split this into two PRs - the first one being for the dual tests, with the option to enable Masterminds which does nothing in that first PR (other than to allow the tests to run both with and without that option set)...

PPS. I've added #921, which might have some relevance for the testing side of this - whatever they are doing, i.e. the approach they are taking, to combine data providers might also suggest a way of implementing re-running TestCases with different paramenters in the way we want. I haven't looked closely. However, a brief look at @requires suggests it would not be suitable (it can only skip whole tests, not specific elements in a dataset).

[bytestream]

Any comments on what I just pushed? This is the simplest way of doing it.

I've added $html5 as an argument to fromHtml(). The reason for this is that a class level enableHtml5 method does not make sense. The option is only relevant to fromHtml(). If you use fromDomDocument and enableHtml5 then nothing is going to happen and I suspect that will confuse people.

I suspect what you really wanted to see is the below. If this is the approach you want then let me know but this is significantly more work...

AbstractHtmlProcessor:

public function __construct(?HTML5 $parser = null)
{
    $this->html5 = $parser;  
}

AbstractTest:

public function fooTest(): void
{
    $this->instance->doSomething();
}

DomDocumentTest:

public function setUp(): void
{
    $this->instance = new HtmlProcessor();
}

Html5Test:

public function setUp(): void
{
    $this->instance = new HtmlProcessor(
        new HTML5([/* opts */])
    );
}

[JakeQZ]

Any comments on what I just pushed?

I like the solution to the testing issue, by moving it to the Composer / GitHub Action level to run the tests twice. I hadn't thought of that, but it should make it much easier to do.

In terms of interface for usage, I was thinking that this would be a configration level option, and could be implemented similarly to how it is done in PHPWord for these sort of options, rather than an extra parameter. @oliverklee, WDYT?

You are right that HTML5 is not applicable to the fromDomDocument case. This highlights another issue that I have just realized. Since the DOMDocument may be passed between class instances (e.g. CssInliner then CssPruner and/or CssToAttributeCoverter), we would need some way for the HTML5 object instance to be also passed through as well as the DOMDocument. A neat solution, I think, would be to add an AbstractHtmlProcessor::fromHtmlProcessor method that takes care of copying all required object references across, and to recommend that in place of fromDomDocument, e.g. in the examples:

$domDocument = CssInliner::fromHtml($html)->inlineCss($css)->getDomDocument();

HtmlPruner::fromDomDocument($domDocument)->removeElementsWithDisplayNone();
$html = CssToAttributeConverter::fromDomDocument($domDocument)
  ->convertCssToVisualAttributes()->render();

becomes

$htmlProcessor = CssInliner::fromHtml($html)->inlineCss($css);

HtmlPruner::fromHtmlProcessor($htmlProcessor)->removeElementsWithDisplayNone();
$html = CssToAttributeConverter::fromHtmlProcessor($htmlProcessor)
  ->convertCssToVisualAttributes()->render();

Such a change could be implmented as a separate PR ahead of other changes. When it comes to merging to main (hopefully soon!), it would be good to split this up in some reasonable logical way into several separate changesets, so it is more manageable and any issues can be more easily isolated.

[bytestream]

A neat solution, I think, would be to add an AbstractHtmlProcessor::fromHtmlProcessor method that takes care of copying all required object references across, and to recommend that in place of fromDomDocument, e.g. in the examples:

This sounds better than

I was thinking that this would be a configration level option, and could be implemented similarly to how it is done in PHPWord for these sort of options, rather than an extra parameter

because

You are right that HTML5 is not applicable to the fromDomDocument case.

It would mean fromHtmlProcessor allows use of masterminds/html5 and fromDomDocument / fromHtml stay as they are (DOMDocument).

[JakeQZ]

A neat solution, I think, would be to add an AbstractHtmlProcessor::fromHtmlProcessor method that takes care of copying all required object references across, and to recommend that in place of fromDomDocument, e.g. in the examples:

This sounds better than

I was thinking that this would be a configration level option, and could be implemented similarly to how it is done in PHPWord for these sort of options, rather than an extra parameter

I don't follow. The two are independent. There still needs to be a means of deciding whether to use HTML5 in the first place (i.e. when fromHtml is initially invoked).

[bytestream]

As I mentioned I think setHtmlProcessor(HtmlProcessor::HTML5); would be confusing when used with static::fromDomDocument ?

[JakeQZ]

As I mentioned I think setHtmlProcessor(HtmlProcessor::HTML5); would be confusing when used with static::fromDomDocument ?

I see your point, though if only using fromDomDocument you would not call setHtmlProcessor as it would have no effect.

I can also see that having an optional Boolean parameter to fromHtml would allow greater flexibility more easily (should one want to sometimes but not always use HTML5). Is this the only (public) method that would gain a parameter?

[bytestream]

Is this the only (public) method that would gain a parameter?

I think so

[JakeQZ]

Is this the only (public) method that would gain a parameter?

I think so

In that case I would be generally in favour of this approach. (This is proposed as an optional paramter with a default false, which I agree it should be – if in future more options are added, it can be turned into an array or suchlike, with true and false explicitly tested for for backwards compatibility.)

I have thus far only glanced through the changes, and not used a fine-toothed comb.

If there is a way of breaking things down into separate PRs, that would be helpful, though my previous suggestion in this department would be no longer valid (if we are not going to have a global configuration option).

As mentioned, the only thing I think needs to be added is an AbstractHtmlProcessor::fromHtmlProcessor method, and this could be done as a separate PR first (including updates to the README to use this rather than fromDomDocument where applicable) - then with the 2nd PR this would be extended to copy the HTML5 reference as well as the DOMDocument reference.

@oliverklee WDYT?

[oliverklee]

I'm really sorry for being so late for the party. :-(

My thoughts on this:

• I really like the general approach.
• I don't think Emogrifier should be aware of environment variables. Instead, we should have some other way to configure our classes for one of the two modes - maybe via two different subclasss of each class, or maybe by injecting some HTML parsing strategy class. We also can have parametrized tests.
• I dislike having a boolean switch for switching between two (more or less equal) modes. Instead, maybe we can have two HTML parsers: one NativeHtmlParser (which would be a wrapper for the native HTML parsing, and which could be the default if no other parser is passed) and a NewHtmlParser (or some other name) so that we work with both parsers the same way.

What do you think?

[bytestream]

I agree in priciniple but fear the changes you're suggesting require a lot of work.

I don't think Emogrifier should be aware of environment variables

The env var is only really there to minimise the numbers of changes required to get the test suite to run on masterminds/html5

I dislike having a boolean switch for switching between two (more or less equal) modes. Instead, maybe we can have two HTML parsers: one NativeHtmlParser (which would be a wrapper for the native HTML parsing, and which could be the default if no other parser is passed) and a NewHtmlParser (or some other name) so that we work with both parsers the same way.

I may be overcomplicating this but I don't understand the concept of an abstract parser in this case. The entire library is based around DOMDocument and XPath so any future parser would need support for these

[oliverklee]

I will switch the default git branch from master to main in a minute. GitHub then will autoclose all PRs targeted at the removed/renamed branch. Please create a new PR (from the existing PR branch) targeting main.