From 81abd373a767a254e6c3528aece9384cba015163 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 10 Feb 2025 17:51:46 -0800 Subject: [PATCH 1/5] Remove cursor move counting from OD_HTML_Tag_Processor --- .../class-od-html-tag-processor.php | 24 -------- .../optimization-detective/optimization.php | 5 +- .../test-class-od-html-tag-processor.php | 55 ------------------- 3 files changed, 1 insertion(+), 83 deletions(-) diff --git a/plugins/optimization-detective/class-od-html-tag-processor.php b/plugins/optimization-detective/class-od-html-tag-processor.php index de9cffd163..a8f6d82934 100644 --- a/plugins/optimization-detective/class-od-html-tag-processor.php +++ b/plugins/optimization-detective/class-od-html-tag-processor.php @@ -233,16 +233,6 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor { */ private $reached_end_of_document = false; - /** - * Count for the number of times that the cursor was moved. - * - * @since 0.6.0 - * @var non-negative-int - * @see self::next_token() - * @see self::seek() - */ - private $cursor_move_count = 0; - /** * Finds the next tag. * @@ -320,7 +310,6 @@ public function expects_closer( ?string $tag_name = null ): bool { public function next_token(): bool { $this->current_stored_xpath = null; // Clear cache. $this->current_xpath = null; // Clear cache. - ++$this->cursor_move_count; if ( ! parent::next_token() ) { $this->open_stack_tags = array(); $this->open_stack_attributes = array(); @@ -419,19 +408,6 @@ public function next_token(): bool { return true; } - /** - * Gets the number of times the cursor has moved. - * - * @since 0.6.0 - * @see self::next_token() - * @see self::seek() - * - * @return non-negative-int Count of times the cursor has moved. - */ - public function get_cursor_move_count(): int { - return $this->cursor_move_count; - } - /** * Updates or creates a new attribute on the currently matched tag with the passed value. * diff --git a/plugins/optimization-detective/optimization.php b/plugins/optimization-detective/optimization.php index a793a67594..cceb8f6328 100644 --- a/plugins/optimization-detective/optimization.php +++ b/plugins/optimization-detective/optimization.php @@ -363,7 +363,6 @@ function od_optimize_template_output_buffer( string $buffer ): string { $processor->set_bookmark( $current_tag_bookmark ); // TODO: Should we break if this returns false? foreach ( $visitors as $visitor ) { - $cursor_move_count = $processor->get_cursor_move_count(); $visitor_return_value = $visitor( $tag_visitor_context ); if ( true === $visitor_return_value ) { $tracked_in_url_metrics = true; @@ -371,9 +370,7 @@ function od_optimize_template_output_buffer( string $buffer ): string { // If the visitor traversed HTML tags, we need to go back to this tag so that in the next iteration any // relevant tag visitors may apply, in addition to properly setting the data-od-xpath on this tag below. - if ( $cursor_move_count !== $processor->get_cursor_move_count() ) { - $processor->seek( $current_tag_bookmark ); // TODO: Should this break out of the optimization loop if it returns false? - } + $processor->seek( $current_tag_bookmark ); // TODO: Should this break out of the optimization loop if it returns false? } $processor->release_bookmark( $current_tag_bookmark ); diff --git a/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php b/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php index 0c6953c654..055f89d240 100644 --- a/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php +++ b/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php @@ -682,14 +682,9 @@ public function test_bookmarking_and_seeking(): void { ); $actual_figure_contents = array(); - $last_cursor_move_count = $processor->get_cursor_move_count(); - $this->assertSame( 0, $last_cursor_move_count ); $bookmarks = array(); while ( $processor->next_open_tag() ) { - $this_cursor_move_count = $processor->get_cursor_move_count(); - $this->assertGreaterThan( $last_cursor_move_count, $this_cursor_move_count ); - $last_cursor_move_count = $this_cursor_move_count; if ( 'FIGURE' === $processor->get_tag() && @@ -773,56 +768,6 @@ public function test_bookmarking_and_seeking(): void { // TODO: Try adding too many bookmarks. } - /** - * Test get_cursor_move_count(). - * - * @covers ::get_cursor_move_count - */ - public function test_get_cursor_move_count(): void { - $processor = new OD_HTML_Tag_Processor( - trim( - ' - - - - - ' - ) - ); - $this->assertSame( 0, $processor->get_cursor_move_count() ); - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'HTML', $processor->get_tag() ); - $this->assertTrue( $processor->set_bookmark( 'document_root' ) ); - $this->assertSame( 1, $processor->get_cursor_move_count() ); - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'HEAD', $processor->get_tag() ); - $this->assertSame( 3, $processor->get_cursor_move_count() ); // Note that next_token() call #2 was for the whitespace between and . - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'HEAD', $processor->get_tag() ); - $this->assertTrue( $processor->is_tag_closer() ); - $this->assertSame( 4, $processor->get_cursor_move_count() ); - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'BODY', $processor->get_tag() ); - $this->assertSame( 6, $processor->get_cursor_move_count() ); // Note that next_token() call #5 was for the whitespace between and . - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'BODY', $processor->get_tag() ); - $this->assertTrue( $processor->is_tag_closer() ); - $this->assertSame( 7, $processor->get_cursor_move_count() ); - $this->assertTrue( $processor->next_tag() ); - $this->assertSame( 'HTML', $processor->get_tag() ); - $this->assertTrue( $processor->is_tag_closer() ); - $this->assertSame( 9, $processor->get_cursor_move_count() ); // Note that next_token() call #8 was for the whitespace between and . - $this->assertFalse( $processor->next_tag() ); - $this->assertSame( 10, $processor->get_cursor_move_count() ); - $this->assertFalse( $processor->next_tag() ); - $this->assertSame( 11, $processor->get_cursor_move_count() ); - $this->assertTrue( $processor->seek( 'document_root' ) ); - $this->assertSame( 12, $processor->get_cursor_move_count() ); - $this->setExpectedIncorrectUsage( 'WP_HTML_Tag_Processor::seek' ); - $this->assertFalse( $processor->seek( 'does_not_exist' ) ); - $this->assertSame( 12, $processor->get_cursor_move_count() ); // The bookmark does not exist so no change. - } - /** * Export an array as a PHP literal to use as a snapshot. * From e7a984059dc4cfd153219d7735f63ca4c561a6e2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 11 Feb 2025 15:24:26 -0800 Subject: [PATCH 2/5] Add backport of fix in Core-62085 Restore deprecated method without using it --- .../class-od-html-tag-processor.php | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/plugins/optimization-detective/class-od-html-tag-processor.php b/plugins/optimization-detective/class-od-html-tag-processor.php index a8f6d82934..1c9aa97e05 100644 --- a/plugins/optimization-detective/class-od-html-tag-processor.php +++ b/plugins/optimization-detective/class-od-html-tag-processor.php @@ -233,6 +233,31 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor { */ private $reached_end_of_document = false; + /** + * Count for the number of times that the cursor was moved. + * + * The use of this has been replaced with {@see self::$cursor_at_bookmark}. + * + * @since 0.6.0 + * @var non-negative-int + * @see self::next_token() + */ + private $cursor_move_count = 0; + + /** + * The bookmark that the cursor is currently known to be at. + * + * This is used as a backport for the WP 6.8 fix in {@link https://core.trac.wordpress.org/ticket/62085} which + * no-ops seek() calls in which the cursor is already at the provided bookmark. + * + * @since n.e.x.t + * @var string|null + * @see self::next_token() + * @see self::seek() + * @see self::set_bookmark() + */ + private $cursor_at_bookmark = null; + /** * Finds the next tag. * @@ -310,6 +335,8 @@ public function expects_closer( ?string $tag_name = null ): bool { public function next_token(): bool { $this->current_stored_xpath = null; // Clear cache. $this->current_xpath = null; // Clear cache. + $this->cursor_at_bookmark = null; + ++$this->cursor_move_count; if ( ! parent::next_token() ) { $this->open_stack_tags = array(); $this->open_stack_attributes = array(); @@ -408,6 +435,20 @@ public function next_token(): bool { return true; } + /** + * Gets the number of times the cursor has moved. + * + * @since 0.6.0 + * @deprecated n.e.x.t The use of this has been replaced with {@see self::$cursor_at_bookmark}. + * @codeCoverageIgnore + * + * @return non-negative-int Count of times the cursor has moved. + */ + public function get_cursor_move_count(): int { + _deprecated_function( __METHOD__, 'optimization-detective n.e.x.t' ); + return $this->cursor_move_count; + } + /** * Updates or creates a new attribute on the currently matched tag with the passed value. * @@ -485,8 +526,14 @@ public function get_current_depth(): int { * @return bool Whether the internal cursor was successfully moved to the bookmark's location. */ public function seek( $bookmark_name ): bool { + // This is only needed prior to WP 6.8 per . + if ( $bookmark_name === $this->cursor_at_bookmark ) { + return true; + } + $result = parent::seek( $bookmark_name ); if ( $result ) { + $this->cursor_at_bookmark = $bookmark_name; $this->open_stack_tags = $this->bookmarked_open_stacks[ $bookmark_name ]['tags']; $this->open_stack_attributes = $this->bookmarked_open_stacks[ $bookmark_name ]['attributes']; $this->open_stack_indices = $this->bookmarked_open_stacks[ $bookmark_name ]['indices']; @@ -506,6 +553,8 @@ public function seek( $bookmark_name ): bool { public function set_bookmark( $name ): bool { $result = parent::set_bookmark( $name ); if ( $result ) { + $this->cursor_at_bookmark = $name; // Only needed prior to WP 6.8 per . + $this->bookmarked_open_stacks[ $name ] = array( 'tags' => $this->open_stack_tags, 'attributes' => $this->open_stack_attributes, @@ -534,6 +583,10 @@ public function release_bookmark( $name ): bool { return false; } unset( $this->bookmarked_open_stacks[ $name ] ); + if ( $this->cursor_at_bookmark === $name ) { + // Only needed prior to WP 6.8 per . + $this->cursor_at_bookmark = null; + } return parent::release_bookmark( $name ); } From 6d979f7eba463228f8159fb4a48ba128e07fac96 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 11 Feb 2025 15:39:10 -0800 Subject: [PATCH 3/5] Update snapshot with new different attribute order --- .../tests/test-cases/nested-figure-embed/expected.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html index 398b40f1c6..c667b0e3cc 100644 --- a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html +++ b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html @@ -20,7 +20,7 @@
- +
From db0df7bd5e621a8267d45e53a2b754a42337eb6a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 11 Feb 2025 15:58:06 -0800 Subject: [PATCH 4/5] Add check for whether the seek no-op fix is present --- .../nested-figure-embed/expected.html | 2 +- .../class-od-html-tag-processor.php | 23 ++++++++++++++++++- .../test-class-od-html-tag-processor.php | 1 + 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html index c667b0e3cc..398b40f1c6 100644 --- a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html +++ b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/expected.html @@ -20,7 +20,7 @@
- +
diff --git a/plugins/optimization-detective/class-od-html-tag-processor.php b/plugins/optimization-detective/class-od-html-tag-processor.php index 1c9aa97e05..ae4ae01f63 100644 --- a/plugins/optimization-detective/class-od-html-tag-processor.php +++ b/plugins/optimization-detective/class-od-html-tag-processor.php @@ -258,6 +258,27 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor { */ private $cursor_at_bookmark = null; + /** + * Whether current WP version has the no-op check in seek() for when the cursor is already at the provided bookmark. + * + * @since n.e.x.t + * @see self::seek() + * @var bool + */ + private $has_seek_noop; + + /** + * Constructor. + * + * @since n.e.x.t + * + * @param string $html HTML to process. + */ + public function __construct( string $html ) { + parent::__construct( $html ); + $this->has_seek_noop = version_compare( (string) strtok( get_bloginfo( 'version' ), '-' ), '6.8', '>=' ); + } + /** * Finds the next tag. * @@ -527,7 +548,7 @@ public function get_current_depth(): int { */ public function seek( $bookmark_name ): bool { // This is only needed prior to WP 6.8 per . - if ( $bookmark_name === $this->cursor_at_bookmark ) { + if ( ! $this->has_seek_noop && $bookmark_name === $this->cursor_at_bookmark ) { return true; } diff --git a/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php b/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php index 055f89d240..db29077ac1 100644 --- a/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php +++ b/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php @@ -419,6 +419,7 @@ public function data_provider_sample_documents(): array { /** * Test next_tag(), next_token(), get_xpath(), expects_closer(). * + * @covers ::__construct * @covers ::next_open_tag * @covers ::next_tag * @covers ::next_token From ea047d6ace87711506058e3244dd3fc8c22a4848 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 18 Feb 2025 14:32:46 +0800 Subject: [PATCH 5/5] Add TODO --- .../tests/test-cases/nested-figure-embed/set-up.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/set-up.php b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/set-up.php index 59e3337af5..c209530f45 100644 --- a/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/set-up.php +++ b/plugins/embed-optimizer/tests/test-cases/nested-figure-embed/set-up.php @@ -59,7 +59,7 @@ static function ( OD_Tag_Visitor_Context $context ) use ( $test_case ): bool { 'href' => $poster, ) ); - $processor->set_attribute( 'preload', 'auto' ); + $processor->set_attribute( 'preload', 'auto' ); // TODO: For some reason the order here is different with data-od-xpath. } else { $processor->set_attribute( 'preload', 'none' ); }