ENH Use masterminds/html5 for HTMLValue #10647
Conversation
emteknetnz
force-pushed
the
pulls/5/dom-crawler
branch
from
df04994 to
a42e31e
Compare
emteknetnz
changed the title
emteknetnz
force-pushed
the
pulls/5/dom-crawler
branch
from
a42e31e to
444ec50
Compare
emteknetnz
force-pushed
the
pulls/5/dom-crawler
branch
2 times, most recently
from
36b687f to
dc42a5e
Compare
src/View/Parsers/HTMLValue.php
Outdated
|
|
||
| // remove stray closing tags which are not required | ||
| foreach (HTML::config()->get('void_elements') as $tag) { | ||
| $res = str_replace("</$tag>", '', $res); |
There was a problem hiding this comment.
I would expect the HTML5 parser to take care of this. When I comment it out locally, all the unit test still pass just fine.
If it is needed, then we should include a test case to cover it. Also, we might have a case sensitivity problem. So we probably should use str_ireplace instead.
| '<html><html><body><falsetag "attribute=""attribute""">' | ||
| => '<falsetag></falsetag>', | ||
| '<body<body<body>/bodu>/body>' | ||
| => '/bodu>/body>' |
There was a problem hiding this comment.
These two test case still look relevant and should be copied over to HTMLValueTest
| $invalid = [ | ||
| '<p><div><a href="test-link"></p></div>', | ||
| '<html><div><a href="test-link"></a></a></html_>', | ||
| '""\'\'\'"""\'""<<<>/</<htmlbody><a href="test-link"<<>' |
There was a problem hiding this comment.
This test case is not in HTMLValueTest
There was a problem hiding this comment.
Parser cannot parse it, I think it's fine to leave this last one out and not expect the parser to parse because it is wildly invalid HTML
| ); | ||
| } | ||
|
|
||
| public function testAttributeEscaping() |
There was a problem hiding this comment.
This test case is not in HTMLValueTest.
emteknetnz
force-pushed
the
pulls/5/dom-crawler
branch
from
dc42a5e to
d7ddb00
Compare
Issue #10528
I simply used masterminds/html5 directly rather than symfony/dom-crawler (which itself uses masterminds/html5) because it seemed to work out better out of the box with our existing class and unit-tests. We don't need any of the additional API in symfony/dom-crawler in order to maintain our current functionality.
There is minimal behaviour difference between this parser and silverstripe/html5 so be no issues when upgrading. There were only small differences when parsing invalid HTML, usually it seems to remove the invalid HTML rather than trying to create an empty tag.
I feel pretty confident that we can simply upgrade the parser here and not worry about any build task to do content migration. I can't really forsee any legitimate content the first time someone saves a page in CMS 5 after upgrading from CMS 4