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.

Sign up for GitHub

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

Jump to bottom

HTML API: Fix CDATA section null and whitespace handling #7230

Closed
wants to merge 8 commits into from
Closed

HTML API: Fix CDATA section null and whitespace handling #7230

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f5f3130
Fix CDATA section null and whitespace handling
sirreal Aug 22, 2024
ff41a65
Fix comment about cdata section handling
sirreal Aug 22, 2024
47ccb34
A Yoda violation this is not.
sirreal Aug 22, 2024
8554f21
Update comment and rearrange joda condition
dmsnell Aug 23, 2024
702d3bf
Merge branch 'trunk' into html-api/fix-cdata-whitespace-frameset-hand…
sirreal Aug 27, 2024
381b04b
Merge branch 'trunk' into html-api/fix-cdata-whitespace-frameset-hand…
sirreal Sep 3, 2024
7a92ed3
Remove text subdivision on CDATA nodes
sirreal Sep 3, 2024
a22147e
Merge branch 'trunk' into html-api/fix-cdata-whitespace-frameset-hand…
dmsnell Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/wp-includes/html-api/class-wp-html-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4113,9 +4113,26 @@ private function step_in_foreign_content(): bool {
return true;

/*
* > A comment token
* A CDATA section is effectively a text node and should have similar handling.
*/
case '#cdata-section':
/*
* NULL bytes and whitespace do not change the frameset-ok flag.
*/
$current_token = $this->bookmarks[ $this->state->current_token->bookmark_name ];
sirreal marked this conversation as resolved.
Show resolved Hide resolved
$cdata_content_start = $current_token->start + 9;
$cdata_content_length = $current_token->length - 12;
// phpcs:ignore WordPress.PHP.YodaConditions.NotYoda -- A Yoda violation this is not.
if ( $cdata_content_length !== strspn( $this->html, "\0 \t\n\f\r", $cdata_content_start, $cdata_content_length ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The yoda condition PHPCS flagged here seems to be a false positive. I suspect it's confusing a string literal on the line with a literal comparison, but the string literal is not being compared. It's fine to have it on the other side…

Copy link
Member

Choose a reason for hiding this comment

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

it doesn't define Yoda conditions the way most of us do, I think. by flipping this conditional it's happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect it was finding a literal in the RHS of the comparison and thinking that literal are supposed to go on the left (yoda), but it ignores the fact that the literal is in a function call. But that's just my intuition.

Switching the order is fine if it satisfies the linter 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I believe it's the variable on the left hand side that it doesn't like, unless both are variables, in which case it's happy and doesn't care.

joda condition checks like these aren't about literals, but accidental assignment to a variable.

$this->state->frameset_ok = false;
}

$this->insert_foreign_element( $this->state->current_token, false );
return true;

/*
* > A comment token
*/
case '#comment':
case '#funky_comment':
$this->insert_foreign_element( $this->state->current_token, false );
Expand Down
Loading