Skip to content
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

Replace regex with tag processor for duotone class render #49212

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Mar 20, 2023

What?

Updates duotone to use WP_HTML_Tag_Processor instead of preg_replace.

Why?

WP_HTML_Tag_Processor is a more robust solution for adding a class in PHP.

Fixes #49169

How?

Replaces regex code with WP_HTML_Tag_Processor for adding the duotone filter class.

Testing Instructions

  1. Open a post with duotone filters applied.
  2. See that the filters still render.

@ajlende ajlende added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 20, 2023
@ajlende ajlende self-assigned this Mar 20, 2023
@ajlende ajlende requested a review from spacedmonkey as a code owner March 20, 2023 19:08
@github-actions
Copy link

github-actions bot commented Mar 20, 2023

Flaky tests detected in 9c30a2c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4472838415
📝 Reported issues:

@ajlende ajlende requested review from scruffian and jeryj March 20, 2023 21:01
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

I tested this with some posts with Duotone applied and it seemed to work. Code looks good based on what I know about this class.

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +364 to +366
if ( $tags->next_tag() ) {
$tags->add_class( $filter_id );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like add_class gets a free pass and the if statement isn't necessary/desired here.

Suggested change
if ( $tags->next_tag() ) {
$tags->add_class( $filter_id );
}
$tags->add_class( $filter_id );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The $tags->next_tag() returns a boolean if it finds a tag or not, so the if statement is checking to make sure that we have an element to add the class to, not that the class already exists on the element. The only reason I think it might return false is if you have an empty string or malformed HTML.

For reference, I based this off of how it was used in #46625.

@ajlende ajlende merged commit ebdb90c into trunk Mar 21, 2023
@ajlende ajlende deleted the fix/duotone-use-tag-processor branch March 21, 2023 21:11
@github-actions github-actions bot added this to the Gutenberg 15.5 milestone Mar 21, 2023
@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jun 5, 2023
@ramonjd ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use WP_HTML_Tag_Processor for duotone classes
5 participants