Skip to content

HTML API: Close all elements at the end of a document. #7085

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

Closed
dmsnell wants to merge 4 commits into from
Closed

HTML API: Close all elements at the end of a document. #7085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c847d8
HTML API: Close all elements at the end of a document.
dmsnell Jul 24, 2024
7397897
Merge branch 'trunk' into html-api/fix-close-all-elements
dmsnell Jul 25, 2024
380e379
Add test to ensure elements close.
dmsnell Jul 26, 2024
c6532f7
Merge branch 'trunk' into html-api/fix-close-all-elements
dmsnell Jul 29, 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
17 changes: 11 additions & 6 deletions 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 @@ -609,25 +609,30 @@ public function next_token(): bool {
* until there are events or until there are no more
* tokens works in the meantime and isn't obviously wrong.
*/
while ( empty( $this->element_queue ) && $this->step() ) {
continue;
if ( empty( $this->element_queue ) && $this->step() ) {
return $this->next_token();
}

// Process the next event on the queue.
$this->current_element = array_shift( $this->element_queue );
if ( ! isset( $this->current_element ) ) {
return false;
// There are no tokens left, so close all remaining open elements.
while ( $this->state->stack_of_open_elements->pop() ) {
continue;
}
Comment on lines +620 to +622
Copy link
Copy Markdown
Member

@sirreal sirreal Jul 29, 2024

Choose a reason for hiding this comment

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

Minor, but something I've wondered about. the continue doesn't do anything here. Is there a reason to include it?

Suggested change
while ( $this->state->stack_of_open_elements->pop() ) {
continue;
}
while ( $this->state->stack_of_open_elements->pop() ) {}

Copy link
Copy Markdown
Member

@gziolo gziolo Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you would have to put a code comment inside to document the intent otherwise.

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell Jul 29, 2024

Choose a reason for hiding this comment

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

originally I had a comment inside these loops but WPCS thought it was bad so required the introduction of continue, which surprisingly, introduces actual executable code and slows down the runtime

Copy link
Copy Markdown
Member

@gziolo gziolo Jul 29, 2024

Choose a reason for hiding this comment

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

How strict WPCS is for while ( $this->state->stack_of_open_elements->pop() ) {} in a single line?

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell Jul 29, 2024

Choose a reason for hiding this comment

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

hmm. I might not have tried that, but I'm personally not a fan because of unexplained empty body. we can prep a PR and see if it complains. do you consider that better?

Copy link
Copy Markdown
Member

@gziolo gziolo Jul 29, 2024

Choose a reason for hiding this comment

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

It was only thought exercise as it might go against a linting rule if the new line after an opening curly bracket is enforced.

Copy link
Copy Markdown
Member

@gziolo gziolo Jul 29, 2024

Choose a reason for hiding this comment

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

All good with the code that landed 👍


return empty( $this->element_queue ) ? false : $this->next_token();
}

$is_pop = WP_HTML_Stack_Event::POP === $this->current_element->operation;

/*
* The root node only exists in the fragment parser, and closing it
* indicates that the parse is complete. Stop before popping if from
* indicates that the parse is complete. Stop before popping it from
* the breadcrumbs.
*/
if ( 'root-node' === $this->current_element->token->bookmark_name ) {
return ! $is_pop && $this->next_token();
return $this->next_token();
}

// Adjust the breadcrumbs for this event.
Expand All @@ -638,7 +643,7 @@ public function next_token(): bool {
}

// Avoid sending close events for elements which don't expect a closing.
if ( $is_pop && ! static::expects_closer( $this->current_element->token ) ) {
if ( $is_pop && ! $this->expects_closer( $this->current_element->token ) ) {
return $this->next_token();
}

Expand Down
41 changes: 41 additions & 0 deletions tests/phpunit/tests/html-api/wpHtmlProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,47 @@ public static function data_html_with_target_element_and_depth_of_next_node_in_b
);
}

/**
* Ensures that elements which are unopened at the end of a document are implicitly closed.
*
* @ticket 61576
*/
public function test_closes_unclosed_elements() {
$processor = WP_HTML_Processor::create_fragment( '<div><p><span>' );

$this->assertTrue(
$processor->next_tag( 'SPAN' ),
'Could not find SPAN element: check test setup.'
);

// This is the end of the document, but there should be three closing events.
$processor->next_token();
$this->assertSame(
'SPAN',
$processor->get_tag(),
'Should have found implicit SPAN closing tag.'
);

$processor->next_token();
$this->assertSame(
'P',
$processor->get_tag(),
'Should have found implicit P closing tag.'
);

$processor->next_token();
$this->assertSame(
'DIV',
$processor->get_tag(),
'Should have found implicit DIV closing tag.'
);

$this->assertFalse(
$processor->next_token(),
"Should have failed to find any more tokens but found a '{$processor->get_token_name()}'"
);
}

/**
* Ensures that subclasses can be created from ::create_fragment method.
*
Expand Down