Repository navigation
[xExtension-ImageProxy] Use the HTML5 parser of PHP 8.4+ when available - #578
NopeCharon wants to merge 3 commits into
Conversation
The extension parsed the article content with `DOMDocument::loadHTML()`, which relies on the HTML 4 parser of libxml2 and alters modern markup when the content is serialised back: * void elements unknown to HTML 4, such as `<source>`, are turned into containers that swallow the following elements, e.g. the `<img>` of a `<picture>` ends up inside `<source>…</source>`; * loose text at the start of the content is wrapped in a `<p>`; * a `<body>` element with attributes is returned with the content, because only a bare `<body>` tag was stripped from the output. On PHP 8.4+, parse the content with `Dom\HTMLDocument::createFromString()`, which implements the standard HTML5 parsing algorithm, and return the `innerHTML` of the body, so no tag needs to be stripped anymore. The content is always UTF-8 in FreshRSS, which is now passed as the encoding instead of converting every non-ASCII character to a numeric entity. The legacy `DOMDocument` code is kept for PHP 8.1 to 8.3. The proxying of `<img>` and `<video>` elements moves to a helper shared by both parsers. `Dom\Element::getAttribute()` returns `?string`, hence the `?? ''`. PHPStan hides the `Dom\*` classes because the minimum supported PHP version is 8.1, so declare the few symbols used in a stub file that is only scanned, like in FreshRSS/FreshRSS#8825. It can be removed once PHP 8.4 is the minimum version. The regression tests gain HTML5-specific cases, run on PHP 8.4+ only, and the existing Unicode URL check now compares decoded URLs, since only libxml2 percent-encodes `src` attributes when serialising. Fixes FreshRSS#494
|
Related: FreshRSS/FreshRSS#6831 |
| } | ||
| if ($img->hasAttribute('src')) { | ||
| $src = $img->getAttribute('src'); | ||
| $src = $img->getAttribute('src') ?? ''; |
There was a problem hiding this comment.
These changes don't make sense. It's all encapsulated in if ($img->hasAttribute('src')) {. Same down below, also for video.
There was a problem hiding this comment.
Right, removed the ?? '' fallbacks and typed the PHPStan stub's getAttribute() as string instead. 3f8236d
| } | ||
|
|
||
| if (class_exists('Dom\HTMLDocument')) { // PHP 8.4+ | ||
| // Standard HTML5 parser, which handles modern markup such as `<picture>` and does not wrap loose text in `<p>` |
| } | ||
|
|
||
| /** @throws FreshRSS_Context_Exception */ | ||
| private function testHtml5Parsing(): void { |
There was a problem hiding this comment.
Is there really a point to testing these things? It'll already be included as part of https://github.com/lexbor/lexbor/tree/e651d6bce83a6601dff37261374f0545e910b1c1/test (the underlying library in PHP)
There was a problem hiding this comment.
Fair, the picture and loose text cases are really the parser's job. I dropped them and kept only the body test, since stripping the body is done in our code.
Drop the redundant null fallbacks after hasAttribute() checks, remove an unneeded comment, and keep only the HTML5 test that covers our own body handling. Merge main and bump the version to 1.1.1. Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
| min: 80100 # PHP 8.1 | ||
| max: 80599 # PHP 8.5 | ||
| scanFiles: | ||
| - tests/stubs/php84-dom.php |
There was a problem hiding this comment.
Such a stub already exists in phar://%rootDir%/phpstan.phar/vendor/jetbrains/phpstorm-stubs/dom/dom_n.stub if needed. But I have a (hopefully) better approach coming
There was a problem hiding this comment.
This fails to check that the code works with PHP < 8.4. I cannot find a single-pass PHPStan approach covering all our needs, but I will push a 2-pass approach
|
I cannot find a single-pass PHPStan approach covering all our needs, but I have pushed a 2-pass approach. Double-check welcome. This might also be used in FreshRSS core |
Summary
On PHP 8.4+, parse the article content with the standard HTML5 parser (
Dom\HTMLDocument::createFromString()). PHP 8.1 to 8.3 keep the existingDOMDocumentcode.The HTML 4 parser of libxml2 currently changes modern markup when the content is serialised back. For example, on
main:main<picture><source srcset="…"><img src="…"></picture><p>after</p><picture><source srcset="…"><img src="…"></source></picture><p>after</p><picture><source srcset="…"><img src="…"></picture><p>after</p>Loose text <br> line<p>Loose text <br> line</p>Loose text <br> line<!doctype html><html><body class="page"><p>Body text</p></body></html><body class="page"><p>Body text</p><p>Body text</p>innerHTMLof the body, so no<body>tag needs to be stripped anymore. FreshRSS content is always UTF-8, so UTF-8 is passed as the encoding instead of converting every non-ASCII character to a numeric entity<img>and<video>moves to a private helper used by both parsers (Dom\Element::getAttribute()returns?string, hence the?? '')Dom\*classes because the minimum PHP version is 8.1. As in feat(favicons): use PHP 8.4+ native HTML5 parser FreshRSS#8825, a small stub file intests/stubs/php84-dom.phpdeclares the symbols used. It is only scanned, never analysed or executed, and can be deleted when PHP 8.4 becomes the minimumsrcwhen serialising (one café.jpgvsone%20caf%C3%A9.jpg, which browsers request the same way)Fixes #494
Validation
php tests/ImageProxyTest.phpon PHP 8.1 (24 assertions), 8.4 and 8.5 (27 assertions). The new HTML5 test fails on PHP 8.4 with the extension frommain(Void elements such as <source> must not swallow the following elements)edge: installed it with the CLI, then ranImageProxyExtension::setImageProxyHook()on an entry, followed bylazyimg()as the views do. On PHP 8.5 the output is correct HTML5. On PHP 8.3 the legacy path gives the same output as before. In both cases the proxiedsrc/posteris moved todata-originaland thedata-xextension-imageproxy-original-*attributes are keptphp-lint,phpcs,phpstan(with../FreshRSS, run on PHP 8.1 and 8.5) andtypos