Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DomCrawler] Use the native HTML5 parser on PHP 8.4#61475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?7.4
Bug fix?no
New feature?no
Deprecations?no
Issues-
LicenseMIT

This PR keeps theDOM*-based API but uses the native HTML5 parser on PHP 8.4 instead of masterminds/html5.
This works by parsing HTML strings usingDom\HTMLDocument then serializing to XML, and loading again usingDOMDocument::loadXML().

This basically replaces#61356 since it removes any BC breaks.

The drawback compared to a more native approach is the double-parsing that happens.
This could be worked on later by providing a way to leverage the newDom\* API directly.
To be proved worth it before.

@carsonbotcarsonbot added this to the7.4 milestoneAug 20, 2025
@OskarStarkOskarStark changed the title[DomCrawler] Use the native HTM5 parser on PHP 8.4[DomCrawler] Use the native HTML5 parser on PHP 8.4Aug 20, 2025
@nicolas-grekasnicolas-grekasforce-pushed thedom-native-html5 branch 4 times, most recently fromf963618 toa6a0033CompareAugust 21, 2025 06:43
@nicolas-grekasnicolas-grekas merged commit8187625 intosymfony:7.4Aug 21, 2025
10 of 12 checks passed
nicolas-grekas added a commit that referenced this pull requestAug 21, 2025
…nks to the native DOM parser (nicolas-grekas)This PR was merged into the 8.0 branch.Discussion----------[DomCrawler] Always parse according to HTML5 rules thanks to the native DOM parser| Q             | A| ------------- | ---| Branch?       | 8.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITFollows#61475Commits-------0425b2a [DomCrawler] Always parse according to HTML5 rules thanks to the native DOM parser
@nicolas-grekasnicolas-grekas deleted the dom-native-html5 branchAugust 21, 2025 09:08
This was referencedOct 27, 2025
@PhilETaylor
Copy link
Contributor

Just a note/heads up for others that find themselves here...

I upgraded to v7.4.0-BETA1 - ran my unit tests, which worked in 7.3.x - and now they fail :)

Messages like:

Warning: DOMDocument::loadXML(): Failed to parse QName ':class' in Entity, line: 17 in /Users/phil/Sites/symfony-7.4-alpine-reproducer/vendor/symfony/dom-crawler/Crawler.php on line 1116Warning: DOMDocument::loadXML(): error parsing attribute name in Entity, line: 21 in /Users/phil/Sites/symfony-7.4-alpine-reproducer/vendor/symfony/dom-crawler/Crawler.php on line 1116

A reproducer is herehttps://github.com/PhilETaylor/symfony-7.4-alpine-reproducer

I think this is awont fix ...

@stof
Copy link
Member

the fact that Alpine.js makes you write code that reports warnings in spec-compliant HTML parsers looks like an issue for alpine.js instead.

However, it looks like the new code path does not uselibxml_use_internal_errors(true) anymore and so triggers PHP warnings for cases that were not triggering them before.@nicolas-grekas should this be changed ?

@PhilETaylor
Copy link
Contributor

the fact that Alpine.js makes you write code that reports warnings in spec-compliant HTML parsers looks like an issue for alpine.js instead.

Totally agree. Just mentioning it as Im probably the first to see this since upgrading/testing the beta.

and so triggers PHP warnings for cases that were not triggering them before

I only mentioned it as the OP mentioned this should remove b/c breaks, and then my tests broke up upgrade. Im not expecting a fix, unless you think one is needed.

@stof
Copy link
Member

I suggest opening an issue instead of discussing that on a merged PR, to give it more visibility.
As hinted in my previous message, I think something still needs to be fixed.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolas-grekas@PhilETaylor@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp