[DomCrawler] Add DomCrawler to leverage PHP 8.4's HTML5-compliant DOM parser
#61356
+706
−240
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
65a181e to
2e9f204
Compare
2e9f204 to
8b72b64
Compare
|
Looks like psalm doesn't undertand my annotations. Any proposals to make them understandable to the tool? |
|
I'll have a deeper look at this PR soon, but the BC break seems definitely the best option. As you said, I also expect that very few projects will be impacted. Thank you for taking over this topic! |
nicolas-grekas
added a commit
that referenced
this pull request
Aug 12, 2025
… PHP 8.4+ (nicolas-grekas) This PR was merged into the 7.4 branch. Discussion ---------- [HtmlSanitizer] Use the native HTML5 parser when using PHP 8.4+ | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | yes | Issues | #53666 | License | MIT Together with #61356, this PR allows removing any dependency on masterminds/html5 in favor of the native HTML5 capabilities of PHP 8.4 on Symfony 8 In order to do so, this we: * Use the native HTML5 parser when using PHP 8.4+ * Deprecate `MastermindsParser`; use `NativeParser` instead * [BC BREAK] `ParserInterface::parse()` can now return `\Dom\Node|\DOMNode|null` instead of just `\DOMNode|null` * Add argument `$context` to `ParserInterface::parse()` Note that `DomVisitor` is internal so no BC breaks there. And `StringSanitizer::htmlLower()` can leverage `strtolower()` since PHP 8.2 thanks to https://wiki.php.net/rfc/strtolower-ascii Commits ------- d0f98ad [HtmlSanitizer] Use the native HTML5 parser when using PHP 8.4+
alexandre-daubois
approved these changes
Aug 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces #54383
It takes the BC breaking approach by widening some property and method types to accept / return classes from both
Dom\*andDOM*sets.This widening is a hard BC break for child classes the re-declare the corresponding properties/methods.
I expect this situation to be rather uncommon, so I don't think this will be a big-bang BC break.
Fortunately, this change in signature will be easy to spot since the PHP engine will fail early.
In addition, the
Crawlerclass is made generic: once an instance is created, it's going to stick to one set, eitherDom\*orDOM*classes.To further limit the impact,
Crawler::__construct()allows building onlyCrawler<DOMNode>instances and aDomCrawlerclass is introduced to createCrawler<Dom\Node>ones.As a reminder, the goal of this work is to be able to remove the dependency on
masterminds/html5in Symfony 8.It is NOT to force moving to the new
Dom\*API.Still, in order to achieve this goal and because we want HTML5-parsing to be the default,
BrowserKitwill leverage the newDomCrawlerclass when running on PHP 8.4+.The new
Dom\*API has some behavioral changes, all legit and documented at https://wiki.php.net/rfc/opt_in_dom_spec_complianceThe
Crawlerimplementation ensures some fundamental behaviors remain, such as node names being returned in lowercase, empty values/texts being returned as the empty string instead of null, or void tags to not have closing tags. I don't think we'd gain much by propagating these changes brought by theDom\*API to theCrawlerone. It'd actually make adopting the newDom\*implementation harder for little to no benefits.The BC break is a decision we have to make. I think it's the best possible approach because in practice, the impact will be limited. The alternative taken in #54383 is to create two independent type hierarchies. This will create a situation where suddenly, all the existing crawler-related code will need to be updated. Better not when there's a less costly path - even though it'll break some edge-cases.