#57852 closed defect (bug) (fixed)
HTML API: Support RCData and Script tag closers
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
This ticket was mentioned in PR #4164 on WordPress/wordpress-develop by @zieladam.
9 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:
9 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:
9 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:
9 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!
#7
@
9 months ago
- Owner set to hellofromTonya
- Status changed from new to reviewing
Self-assigning for commit review.
@hellofromTonya commented on PR #4164:
9 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:
9 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:
9 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:
9 months ago
#11
@hellofromtonya all good, 6.2.1 should be fine. I'll reply to your other comment later on
#12
@
9 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.
9 months ago
@zieladam commented on PR #4164:
9 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:
9 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
@
9 months ago
- Keywords has-testing-info added
Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/4164
Steps to Reproduce or Test
- Create a must-use plugins directory in
wp-content/mu-plugins/
. - Add a new
test.php
file in that directory. - 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; });
- 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 ofSCRIPT
.
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:
9 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
@
9 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.
@hellofromTonya commented on PR #4164:
9 months ago
#21
Committed via https://core.trac.wordpress.org/changeset/55469.
Changing to an enhancement of the HTML API.