Tag Processor: Fix a problem backing up too far after updating HTML #46598
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
fd9a0a2
to
7fe4a56
Compare
anton-vlasenko
force-pushed
the
fix/tag-processor-back-too-far
branch
from
f78757f
to
49178f0
Compare
anton-vlasenko
requested changes
Dec 16, 2022
There was a problem hiding this comment.
Thanks for the PR, @dmsnell!
It fixes lots of the tests.
But some PHP 5.6 tests still don't pass.
E.g.:
WP_HTML_Tag_Processor_Test::test_updating_attributes_works_even_in_malformed_html_2 with data set "Invalid entity inside attribute value" ('<img src="https://s0.wp.com/i.../span>', '<img foo="bar" class="firstTa.../span>')
[66](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:67)
Failed asserting that two strings are identical.
[67](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:68)
--- Expected
[68](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:69)
+++ Actual
[69](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:70)
@@ @@
[70](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:71)
-<img foo="bar" class="firstTag" src="https://s0.wp.com/i/atat.png" title="&; First <title> is ¬it;" TITLE="second title" title="An Imperial &imperial; AT-AT"><span class="secondTag">test</span>
[71](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:72)
+<img class="firstTag" foo="bar" src="https://s0.wp.com/i/atat.png" title="&; First <title> is ¬it;" TITLE="second title" title="An Imperial &imperial; AT-AT"><span class="secondTag">test</span>
[72](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:73)
[73](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:74)
/var/www/html/wp-content/plugins/gutenberg/phpunit/html/wp-html-tag-processor-test.php:1117
[74](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:75)
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:51
[75](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:76)
[76](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:77)
2) WP_HTML_Tag_Processor_Test::test_updating_attributes_works_even_in_malformed_html_2 with data set "Single and double quotes in attribute value" ('<p title="Demonstrating how t.../span>', '<p foo="bar" class="firstTag".../span>')
[77](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:78)
Failed asserting that two strings are identical.
[78](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:79)
--- Expected
[79](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:80)
+++ Actual
[80](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:81)
@@ @@
[81](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:82)
-<p foo="bar" class="firstTag" title="Demonstrating how to use single quote (') and double quote (")"><span class="secondTag">test</span>
[82](https://github.com/WordPress/gutenberg/actions/runs/3711263900/jobs/6291503554#step:14:83)
+<p class="firstTag" foo="bar" title="Demonstrating how to use single quote (') and double quote (")"><span class="secondTag">test</span>
|
I had to rebase this PR, because Github started to display incorrect "changed files" when I rebased https://github.com/WordPress/gutenberg/tree/fix/parity-with-cores-php-ci-jobs to the latest trunk. |
|
PHP 5.6 tests do not indicate a real problem – they just complain about the order of the generated attributes: These updates are sorted as follows: private function apply_attributes_updates() {
// ...
usort( $this->attribute_updates, array( self::class, 'sort_start_ascending' ) );
foreach ( $this->attribute_updates as $diff ) {
// ...
}
// ...
}
private static function sort_start_ascending( $a, $b ) {
return $a->start - $b->start;
}In the failed tests Ideally the tests wouldn't fail in this scenario. How about we provide an alternative acceptable output string where these attributes are listed in PHP 5.6 order @dmsnell ? |
@adamziel I wonder why it passes the tests then? The code responsible for attributes' sorting hasn't changed since then.
Doing so would make the tests pass, but it doesn't explain why the sorting doesn't work as it used to. |
Ah, I get it. We didn't compare similar values in the callback function before #46018.
Yes. We also have to ensure that these alternative strings are only used when running the tests on PHP 5.x. |
|
Thanks for working on this, both of you.
This PR won't merge to
If I'm reading this right we're talking about providing alternative strings against which to compare the results based on differences in the sort algorithm across PHP versions. That's something that would quiet the tests but definitely increase the maintenance burden and confuse the people who come after us, so I'd rather we either fix the tests so that they aren't clamping down on behaviors we don't care to assert (for example, we don't care where the attributes appear) or we take the same "avoid the problem" approach but do it inside the tag processor. I'll propose an update soon which does one of these. It's noteworthy that PHP 8.0 changes the default behavior of the sort functions by making them stable, whereas in previous versions the sort is unstable. Simply changing the comparison string is not only risky, but it's only going to move us a couple steps forward before we would run into this same problem. I'm thinking we could sort the updates by start and then by alphabetical order of the replacement. That shouldn't product a conflict because we shouldn't have multiple copies of the same attribute. This is all adjusting runtime behavior in order to appease deficient tests, and while I hate doing that, as a practical thing we discussed using |
dmsnell
changed the base branch from
fix/parity-with-cores-php-ci-jobs
to
trunk
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
f180522
to
f138315
Compare
|
Everything was passing in the tests in fix/parity-with-cores-php-ci-jobs, @anton-vlasenko, so I've rebased against It's a good update for |
dmsnell
added a commit
that referenced
this pull request
Dec 16, 2022
…46598) A defect introduced in #46018 led to the tag processor backing up one index too far after flushing its queued changes on a document. For most operations this didn't cause any harm because when immediately moving forward after an update, the `next_tag()` returned to the same spot: it was backing up to one position before the current tag instead of at the start of the current tag. Unfortunately, when the current tag was the first in the document this would lead the processor to rewind to position `-1`, right before the start of the document, and lead to errors with `strpos()` when it received out-of-bounds indices. In this fix we're correcting the adjustment for the HTML tag's `<` and documenting the math in the file so that it's clearer why it's there and providing guidance should another fix be necessary. As supporting work to this patch we're making the text replacement sort stable, inside the tag processor, for when determining the order in which to apply text replacements. This isn't necessary for the runtime but is a nuissance for testing because different PHP versions produce different unstable sort orderings and this prevents that from causing the unit tests to fail in one version but pass in another. Props to @anton-vlasenko for finding this bug. Enforce sort stability when flushing out text replacements
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
f138315
to
5b7ef38
Compare
anton-vlasenko
requested changes
Dec 19, 2022
There was a problem hiding this comment.
@dmsnell
When I run the tests locally (PHP 8.1), I get these errors (please see below).
Maybe it makes sense to add alternative strings, just as @adamziel proposed.
I'm not sure why Github's CI jobs don't fail. I'm investigating it.
P.S. I'm going to reinstall my local WP copy to see if it's somehow related.
I've asked around and it seems that this issue sometimes happens: sort works differently on different PHP installations.
So if the tests pass here, I guess that is fine.
Please disregard this message, @dmsnell
1) Block_Fixture_Test::test_kses_doesnt_change_fixtures with data set #69 ('<!-- wp:file {"href":"http://... -->\n', 'core__file__pdf-preview.seria...d.html')
Failed to match core__file__pdf-preview.serialized.html
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
'<!-- wp:file {"href":"http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf","displayPreview":true,"previewHeight":370} -->
-<div class="wp-block-file"><object class="wp-block-file__embed" data="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" type="application/pdf" style="width:100%;height:370px" aria-label="yolo"></object><a id="wp-block-file--media-_clientId_0" href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf">yolo</a><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" class="wp-block-file__button wp-element-button" download aria-describedby="wp-block-file--media-_clientId_0">Download</a></div>
+<div class="wp-block-file"><object></object><a id="wp-block-file--media-_clientId_0" href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf">yolo</a><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" class="wp-block-file__button wp-element-button" download aria-describedby="wp-block-file--media-_clientId_0">Download</a></div>
<!-- /wp:file -->
'
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/class-block-fixture-test.php:31
2) Block_Fixture_Test::test_kses_doesnt_change_fixtures with data set #70 ('<!-- wp:file {"href":"http://... -->\n', 'core__file__pdf-preview__depr...d.html')
Failed to match core__file__pdf-preview__deprecated-1.serialized.html
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
'<!-- wp:file {"href":"http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf","displayPreview":true,"previewHeight":370} -->
-<div class="wp-block-file"><object class="wp-block-file__embed" data="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" type="application/pdf" style="width:100%;height:370px" aria-label="yolo"></object><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf">yolo</a><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" class="wp-block-file__button wp-element-button" download>Download</a></div>
+<div class="wp-block-file"><object></object><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf">yolo</a><a href="http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf" class="wp-block-file__button wp-element-button" download>Download</a></div>
<!-- /wp:file -->
'
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/class-block-fixture-test.php:31
3) Gutenberg_REST_Global_Styles_Controller_Test::test_get_theme_items
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+ 0 => Array &1 (
+ 'version' => 2
+ 'settings' => Array &2 (
+ 'color' => Array &3 (
+ 'palette' => Array &4 (
+ 'theme' => Array &5 (
+ 0 => Array &6 (
+ 'slug' => 'foreground'
+ 'color' => '#3F67C6'
+ 'name' => 'Foreground'
+ )
+ )
+ )
+ )
+ )
+ 'styles' => Array &7 (
+ 'blocks' => Array &8 (
+ 'core/post-title' => Array &9 (
+ 'typography' => Array &10 (
+ 'fontWeight' => '700'
+ )
+ )
+ )
+ )
+ 'title' => 'variation'
+ )
+)
/Users/anton/html/wordpress.test/tests/phpunit/includes/abstract-testcase.php:987
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/class-gutenberg-rest-global-styles-controller-test.php:116
4) WP_Webfonts_Provider_Local_Test::test_get_css with data set "truetype format" (array(array('local', 'Open Sans', 'italic', 'bold', 'http://example.org/assets/fon...ht.ttf')), '@font-face{font-family:"Open ...pe');}')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'@font-face{font-family:"Open Sans";font-style:italic;font-weight:bold;src:local("Open Sans"), url('http://example.org/assets/fonts/OpenSans-Italic-VariableFont_wdth,wght.ttf') format('truetype');}'
+'@font-face{font-family:"Open Sans";font-style:italic;font-weight:bold;src:local("Open Sans"), url('/assets/fonts/OpenSans-Italic-VariableFont_wdth,wght.ttf') format('truetype');}'
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/class-wp-webfonts-local-provider-test.php:91
5) WP_Webfonts_Provider_Local_Test::test_get_css with data set "woff2 format" (array(array('local', 'Source Serif Pro', 'normal', '200 900', 'normal', 'http://example.org/assets/fon....woff2'), array('local', 'Source Serif Pro', 'italic', '200 900', 'normal', 'http://example.org/assets/fon....woff2')), '@font-face{font-family:"Sourc...f2');}')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'@font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-stretch:normal;src:local("Source Serif Pro"), url('http://example.org/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');}@font-face{font-family:"Source Serif Pro";font-style:italic;font-weight:200 900;font-stretch:normal;src:local("Source Serif Pro"), url('http://example.org/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2') format('woff2');}'
+'@font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-stretch:normal;src:local("Source Serif Pro"), url('/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');}@font-face{font-family:"Source Serif Pro";font-style:italic;font-weight:200 900;font-stretch:normal;src:local("Source Serif Pro"), url('/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2') format('woff2');}'
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/class-wp-webfonts-local-provider-test.php:91
6) WP_Style_Engine_Test::test_wp_style_engine_get_styles with data set "inline_valid_typography_style" (array(array('clamp(2em, 2vw, 4em)', 'Roboto,Oxygen-Sans,Ubuntu,sans-serif', 'italic', '800', '1.3', '2', 'underline', 'uppercase', '2')), null, array('font-size:clamp(2em, 2vw, 4em...ing:2;', array('clamp(2em, 2vw, 4em)', 'Roboto,Oxygen-Sans,Ubuntu,sans-serif', 'italic', '800', '1.3', '2', 'underline', 'uppercase', '2')))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
Array &0 (
- 'css' => 'font-size:clamp(2em, 2vw, 4em);font-family:Roboto,Oxygen-Sans,Ubuntu,sans-serif;font-style:italic;font-weight:800;line-height:1.3;column-count:2;text-decoration:underline;text-transform:uppercase;letter-spacing:2;'
+ 'css' => 'font-size:clamp(2em, 2vw, 4em);font-family:Roboto,Oxygen-Sans,Ubuntu,sans-serif;font-style:italic;font-weight:800;line-height:1.3;text-decoration:underline;text-transform:uppercase;letter-spacing:2;'
'declarations' => Array &1 (
'font-size' => 'clamp(2em, 2vw, 4em)'
'font-family' => 'Roboto,Oxygen-Sans,Ubuntu,sans-serif'
@@ @@
'font-style' => 'italic'
'font-weight' => '800'
'line-height' => '1.3'
- 'column-count' => '2'
'text-decoration' => 'underline'
'text-transform' => 'uppercase'
'letter-spacing' => '2'
)
)
/Users/anton/html/wordpress.test/src/wp-content/plugins/gutenberg/phpunit/style-engine/style-engine-test.php:47
| * @return integer | ||
| */ | ||
| private static function sort_start_ascending( $a, $b ) { | ||
| return $a->start - $b->start; | ||
| $by_start = $a->start - $b->start; | ||
| if ( $by_start !== 0 ) { |
There was a problem hiding this comment.
Suggested change
| if ( $by_start !== 0 ) { | |
| if ( 0 !== $by_start ) { |
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#yoda-conditions
| } | ||
|
|
||
| $by_text = isset( $a->text, $b->text ) ? strcmp( $a->text, $b->text ) : 0; | ||
| if ( $by_text !== 0 ) { |
There was a problem hiding this comment.
Suggested change
| if ( $by_text !== 0 ) { | |
| if ( 0 !== $by_text ) { |
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#yoda-conditions
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
5b7ef38
to
76d08aa
Compare
anton-vlasenko
approved these changes
Dec 19, 2022
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
76d08aa
to
07586c8
Compare
|
Rebasing this against |
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
07586c8
to
070e007
Compare
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
070e007
to
5622c03
Compare
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
2371d5b
to
79b08f5
Compare
|
Size Change: +35 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
4 times, most recently
from
9551114
to
b11e861
Compare
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
b11e861
to
6bd6a6a
Compare
anton-vlasenko
approved these changes
Dec 20, 2022
There was a problem hiding this comment.
I confirm this PR passes PHPUnit tests on all Core-supported PHP versions (5.6 - 8.2). So I approve that PR.
But I'd prefer someone else to also approve this PR as I'm not very familiar with the `WP_HTML_Tag_Processor' class. This PR changes the order in which element attributes are sorted.
adamziel
approved these changes
Dec 20, 2022
|
👋 Coming here from #46680, which seems to be fixed by this PR. Thank you for that! 🎉 Can we maybe add the test coverage from that PR here? I.e. diff --git a/phpunit/html/wp-html-tag-processor-test.php b/phpunit/html/wp-html-tag-processor-test.php
index cab1496249..fbf629088c 100644
--- a/phpunit/html/wp-html-tag-processor-test.php
+++ b/phpunit/html/wp-html-tag-processor-test.php
@@ -378,12 +378,14 @@ class WP_HTML_Tag_Processor_Test extends WP_UnitTestCase {
*
* @covers set_attribute
* @covers get_updated_html
+ * @covers get_attribute
*/
public function test_set_attribute_with_a_non_existing_attribute_adds_a_new_attribute_to_the_markup() {
$p = new WP_HTML_Tag_Processor( self::HTML_SIMPLE );
$p->next_tag();
$p->set_attribute( 'test-attribute', 'test-value' );
$this->assertSame( '<div test-attribute="test-value" id="first"><span id="second">Text</span></div>', $p->get_updated_html() );
+ $this->assertSame( 'test-value', $p->get_attribute( 'test-attribute' ) );
}
/**(And maybe add an assertion like that to other |
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
6bd6a6a
to
f68508d
Compare
…46598) A defect introduced in #46018 led to the tag processor backing up one index too far after flushing its queued changes on a document. For most operations this didn't cause any harm because when immediately moving forward after an update, the `next_tag()` returned to the same spot: it was backing up to one position before the current tag instead of at the start of the current tag. Unfortunately, when the current tag was the first in the document this would lead the processor to rewind to position `-1`, right before the start of the document, and lead to errors with `strpos()` when it received out-of-bounds indices. In this fix we're correcting the adjustment for the HTML tag's `<` and documenting the math in the file so that it's clearer why it's there and providing guidance should another fix be necessary. As supporting work to this patch we're making the text replacement sort stable, inside the tag processor, for when determining the order in which to apply text replacements. This isn't necessary for the runtime but is a nuissance for testing because different PHP versions produce different unstable sort orderings and this prevents that from causing the unit tests to fail in one version but pass in another. Props to @anton-vlasenko for finding this bug. Enforce sort stability when flushing out text replacements
dmsnell
force-pushed
the
fix/tag-processor-back-too-far
branch
from
f68508d
to
696d63f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Fixes a bug in the HTML Tag Processor when updating a document when the internal pointer is at the first tag in the document.
Why?
The Tag Processor shouldn't break code relying on it.
How?
A defect introduced in #46018 led to the tag processor backing up one index too far after flushing its queued changes on a document.
For most operations this didn't cause any harm because when immediately moving forward after an update, the
next_tag()returned to the same spot: it was backing up to one position before the current tag instead of at the start of the current tag.Unfortunately, when the current tag was the first in the document this would lead the processor to rewind to position
-1, right before the start of the document, and lead to errors withstrpos()when it received out-of-bounds indices.In this fix we're correcting the adjustment for the HTML tag's
<and documenting the math in the file so that it's clearer why it's there and providing guidance should another fix be necessary.Props to @anton-vlasenko for finding this bug.
Testing Instructions
Run the PHP unit test suite with PHP <= 7.0.
Tests should start failing in
trunkbut should pass in this branch.Temporarily merging in
fix/parity-with-cores-php-ci-jobsin this PR to run such tests in CI.