Skip to content

Tag Processor: Support RCData and Script tag closers#4164

Closed
adamziel wants to merge 7 commits intoWordPress:trunkfrom
adamziel:tag-processor-stop-on-rcdata-tag-closers
Closed

Tag Processor: Support RCData and Script tag closers#4164
adamziel wants to merge 7 commits intoWordPress:trunkfrom
adamziel:tag-processor-stop-on-rcdata-tag-closers

Conversation

@adamziel
Copy link
Copy Markdown
Contributor

@adamziel adamziel commented Mar 2, 2023 •
edited by hellofromtonya
Loading

Description

With this PR, Tag Processor can navigate to </script>, </textarea>, and </title> tag closers:

$p = new WP_HTML_Tag_Processor('<script>ABC</script><p>');
$p->next_tag();
$p->next_tag( array( 'tag_closers' => 'visit' ) );
$p->get_tag(); // 'script'

Without this commit, Tag Processor skips over them:

$p = new WP_HTML_Tag_Processor('<script>ABC</script><p>');
$p->next_tag();
$p->next_tag( array( 'tag_closers' => 'visit' ) );
$p->get_tag(); // 'p'

Tag closers are supported by Tag Processor so it only makes sense to consistently support all of them.

cc @ockham @dmsnell @hellofromtonya @gziolo

Trac ticket: https://core.trac.wordpress.org/ticket/57852

adamziel added 2 commits March 2, 2023 15:58
@adamziel
Stop on RCData and Script tag closers
9ee8943 
With this commit, Tag Processor can navigate to
</script>, </textarea>, and </title> tag closers.

Without this commit, the Tag Processor skips over them.

Tag closers are supported by Tag Processor so it only
makes sense to consistently support all of them.
@adamziel
Fix a typo
47be8d6
dmsnell
dmsnell approved these changes Mar 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

A very good addition here, @adamziel

If I had one thing to suggest is that maybe we don't need a separate test for this, that we could put the additional tags in existing tag closer tests.

Also I don't find the abstraction of the script name useful in the tests. I would probably prefer to see <script></script> rather than script that then gets interpolated as such within the test. that's just my style preference though.

@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 2, 2023

@adamziel https://core.trac.wordpress.org/ticket/56299 is closed. I opened a new Trac ticket for this enhancement.

Question: Has this already been released in Gutenberg? If no, then likely this needs to move to 6.3.

hellofromtonya
hellofromtonya reviewed Mar 2, 2023
View reviewed changes
@adamziel @hellofromtonya
Update tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
04c3552 
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Mar 2, 2023 •
edited
Loading

@hellofromtonya it haven't been released in Gutenberg yet, no. Let's punt to 6.3 then. Or 6.2.1? I'll loop in @ntsekouras and @Mamaduka just in case

hellofromtonya
hellofromtonya reviewed Mar 2, 2023
View reviewed changes
@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 2, 2023

Let's punt to 6.3 then. Or 6.2.1? I'll loop in @ntsekouras and @Mamaduka just in case

Enhancements need to go into a major. I'll move it to 6.3. Thank you @adamziel!

@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 2, 2023

Also wondering @adamziel @dmsnell, 6.3 is set to release in August 2023. Instead of continuing development in Core on the HTML API, might be better to continue its refinement and enhanced support / capability back to Gutenberg. Then backport them to Core when ready. That way, each can gain faster testing and feedback cycles. What do you think?

@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Mar 2, 2023

Enhancements need to go into a major.

@hellofromtonya oh this is a bugfix, not an enhancement. The Tag Processor should have never omitted these tag closers in the first place.

@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 2, 2023 •
edited
Loading

oh this is a bugfix, not an enhancement. The Tag Processor should have never omitted these tag closers in the first place.

@adamziel thank you for clarifying!

Is this bug fix essential for 6.2? If yes, it could be committed into 6.2, but will trigger another beta release. Or as you noted, it could go into 6.2.1.

@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Mar 2, 2023

@hellofromtonya all good, 6.2.1 should be fine. I'll reply to your other comment later on

hellofromtonya
hellofromtonya reviewed Mar 2, 2023
View reviewed changes
@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Mar 3, 2023 •
edited
Loading

Instead of continuing development in Core on the HTML API, might be better to continue its refinement and enhanced support / capability back to Gutenberg. Then backport them to Core when ready. That way, each can gain faster testing and feedback cycles. What do you think?

There are a few reasons these Tag Processor PRs are targeting core now:

  • It eliminates the risk of a large core PR too late in the release cycle – all the changes will already be there
  • Backporting changes to Core isn't fast or simple, but backporting changes to Gutenberg is
  • Not updating Gutenberg files means no conflicts to resolve
  • The unit tests have been moved to core and it's a hassle to run or update them in the Gutenberg repo. I suppose Gutenberg could maintain a separate copy of the unit tests as well, but that means additional backporting and reconcilliation work

cc @dmsnell @ockham

@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Mar 3, 2023 •
edited
Loading

@hellofromtonya @dmsnell I went for a separate test case without a data provider to test both cases (<script></script> and </script>). I couldn't find a clean way of fitting these checks into the existing test cases that were mentioned in a discussion.

hellofromtonya
hellofromtonya reviewed Mar 6, 2023
View reviewed changes
hellofromtonya
hellofromtonya approved these changes Mar 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

  • Confirming this is a bugfix, fixing that the processor shouild stop on RCData nad Script tag closers ✅
  • Has good happy and unhappy path test coverage ✅

Ready for commit 👍

@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 6, 2023

Confirmed:

  • Tests fail without the fixes ❌
  • Tests pass with the fixes ✅
  • Manually testing:
    • Able to reproduce the issue without the fix ❌
    • After applying the fix, works as expected ✅

Test Report https://core.trac.wordpress.org/ticket/57852#comment:16

@hellofromtonya
Copy link
Copy Markdown
Contributor

hellofromtonya commented Mar 6, 2023

Committed via https://core.trac.wordpress.org/changeset/55469.

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

Reviewers

@dmsnell dmsnell dmsnell approved these changes

@hellofromtonya hellofromtonya hellofromtonya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adamziel @hellofromtonya @dmsnell