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

Appearance settings

[DomCrawler] Make masterminds/html5 as a required dependency of DomCrawler #44170

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

tgalopin
Copy link
Contributor

Q A
Branch? 6.1 (branch not created yet, targeting 6.0 for now)
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #43341
License MIT
Doc PR -

Following the discussion on #43341 and how we will add a new HtmlSanitizer component to Symfony (#44144), it makes a lot of sense to require masterminds/html5 by default on the DomCrawler component. This PR does this and removes unnecessary checks for the library presence in the code.

src/Symfony/Component/DomCrawler/Crawler.php Outdated Show resolved Hide resolved
@derrabus derrabus modified the milestones: 6.0, 6.1 Nov 20, 2021
@ro0NL
Copy link
Contributor

ro0NL commented Nov 21, 2021

I'm not sure how HtmlSanitizer vs. DomCrawler correlates 🤔 nevertheless i think this makes the implementation more robust 👍

@tgalopin
Copy link
Contributor Author

@ro0NL they correlate because the current implementation is basically an opt-in based on the mere presence of the parser. Having html-sanitizer as a component requiring the parser would opt-in people unexpectedly. This pr solves this.

@tgalopin tgalopin force-pushed the require-masterminds-html5 branch from 3ef6037 to 4dab9df Compare November 21, 2021 13:29
@fabpot
Copy link
Member

fabpot commented Dec 11, 2021

Can you target 6.1 now that exists?

@fabpot
Copy link
Member

fabpot commented Mar 26, 2022

@tgalopin Can you rebase this PR on 6.1? Is it ready to be merged then?

@nicolas-grekas nicolas-grekas changed the base branch from 6.0 to 6.1 April 12, 2022 15:14
@nicolas-grekas nicolas-grekas force-pushed the require-masterminds-html5 branch from 4dab9df to 14aacda Compare April 12, 2022 15:15
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I completed the rebase)

@nicolas-grekas
Copy link
Member

Thank you @tgalopin.

@Bilge
Copy link
Contributor

Bilge commented Jan 11, 2023

Looks like this is responsible for the grim regression outlined in #48950.

kesselb added a commit to kesselb/symfony-docs that referenced this pull request Feb 16, 2023
kesselb added a commit to kesselb/symfony-docs that referenced this pull request Feb 16, 2023
javiereguiluz pushed a commit to javiereguiluz/symfony-docs that referenced this pull request Feb 17, 2023
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Feb 17, 2023
…esselb)

This PR was merged into the 6.2 branch.

Discussion
----------

docs(DomCrawler): remove hint about html5-php library

html5-php is mandatory after symfony/symfony#44170

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/releases for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `6.x` for features of unreleased versions).

-->

Commits
-------

1be7e33 docs(DomCrawler): remove hint about html5-php library
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Make masterminds/html5 a required dependency of dom-crawler
7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.