Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 16 months ago

#57852 closed defect (bug) (fixed)

HTML API: Support RCData and Script tag closers

Reported by: zieladam's profile zieladam Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: HTML API Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: Cc:

Description

This proposal is to enhance the Tag Processor for it 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'

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

Change History (22)

#1 @hellofromTonya
17 months ago

  • Type changed from defect (bug) to enhancement

Changing to an enhancement of the HTML API.

This ticket was mentioned in PR #4164 on WordPress/wordpress-develop by @zieladam.


17 months ago
#2

  • Keywords has-patch has-unit-tests added

## Description

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

{{{php
$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:

{{{php
$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

@hellofromTonya commented on PR #4164:


17 months ago
#3

@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.

@zieladam commented on PR #4164:


17 months ago
#4

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

@hellofromTonya commented on PR #4164:


17 months ago
#5

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!

#6 @hellofromTonya
17 months ago

  • Milestone changed from Awaiting Review to 6.3

#7 @hellofromTonya
17 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning for commit review.

@hellofromTonya commented on PR #4164:


17 months ago
#8

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?

@zieladam commented on PR #4164:


17 months ago
#9

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 commented on PR #4164:


17 months ago
#10

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.

@zieladam commented on PR #4164:


17 months ago
#11

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

#12 @hellofromTonya
17 months ago

  • Milestone changed from 6.3 to 6.2
  • Type changed from enhancement to defect (bug)

In talking with @zieladam, this ticket is indeed a bugfix:

The Tag Processor should have never omitted these tag closers in the first place.

I'll move into 6.2.0 as 6.2.1 is not yet opened yet. However, if another beta is triggered, then this can be committed to ship with 6.2.0.

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

@zieladam commented on PR #4164:


17 months ago
#14

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's a few reason 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 are already 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

@zieladam commented on PR #4164:


17 months ago
#15

@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 them into the existing test cases that were mentioned in a discussion.

#16 @hellofromTonya
17 months ago

  • Keywords has-testing-info added

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4164

Steps to Reproduce or Test

  1. Create a must-use plugins directory in wp-content/mu-plugins/.
  2. Add a new test.php file in that directory.
  3. Copy the following code into the test file:
    <?php
    
    add_action( 'init', function() {
            $p = new WP_HTML_Tag_Processor( '<script>ABC</script><p>Some content</p>' );
            $p->next_tag();
            $p->next_tag( array( 'tag_closers' => 'visit' ) );
            $tag = $p->get_tag(); // Should be 'SCRIPT'
            echo "<p style='margin:30px;font-size:50px'>$tag</p>";
            exit;
    });
    
    
  1. Refresh the browser to view the site. 🐞 Bug occurs.

Expected Results

When testing a patch to validate it works as expected:

  • ✅ It should display: SCRIPT

When reproducing a bug:

  • ❌ It'll display: P

Environment

  • Browser: Chrome
  • Localhost: wp-env (Docker)
  • OS: macOS
  • Plugins: none
  • Theme: TT3
  • WordPress: trunk

Actual Results

When reproducing a bug/defect:

  • ❌ Yes, can confirm that it finds P instead of SCRIPT.

When testing the bugfix patch:

  • ✅ Yes, can confirm that when applying the patch, it now finds the closer tag, i.e. SCRIPT.
Automated Tests

When running

npm run test:php -- --group html-api

with the patch, the results are ❌:

..............................................F................  63 / 164 ( 38%)
............................................................... 126 / 164 ( 76%)
......................................                          164 / 164 (100%)

Time: 00:01.991, Memory: 150.50 MB

There was 1 failure:

1) Tests_HtmlApi_wpHtmlTagProcessor::test_next_tag_should_stop_on_rcdata_and_script_tag_closers_when_requested
Did not find the </script> tag closer
Failed asserting that false is true.

This is expected and reproduces the bug.

After applying the patch, the tests pass ✅

@hellofromTonya commented on PR #4164:


17 months ago
#17

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

#18 @hellofromTonya
17 months ago

  • Version set to trunk

PR 4164 is ready for commit.

As there will be an unscheduled Beta 5 tomorrow, I'm preparing the commit now for that beta. This will ship in 6.2.

#19 @hellofromTonya
17 months ago

  • Keywords commit added

#20 @hellofromTonya
17 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55469:

HTML API: Fix finding RCData and Script tag closers.

Fixes finding the following tag closers </script>, </textarea>, and </title> in WP_HTML_Tag_Processor.

Follow-up to [55407], [55203].

Props zieladam, dmsnell, hellofromTonya.
Fixes #57852.
See #57575.

#22 @SergeyBiryukov
16 months ago

  • Component changed from General to HTML API
Note: See TracTickets for help on using tickets.