-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Navigation Block: Fix erroneous escaping of ampersands (etc) #59561
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/blocks/block-navigation-block-hooks-test.php |
$post = get_post( self::$navigation_post ); | ||
|
||
gutenberg_block_core_navigation_update_ignore_hooked_blocks_meta( $post ); | ||
$this->assertSame( self::$original_markup . '<!-- wp:tests/my-block /-->', $post->post_content ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently failing, but apparently not because of #59516:
1) Block_Navigation_Block_Hooks_Test::test_block_core_navigation_update_ignore_hooked_blocks_meta
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<!-- wp:navigation-link {"label":"News & About","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type"} /--><!-- wp:tests/my-block /-->'
+'<!-- wp:navigation-link {"label":"News \u0026amp; About","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type"} /-->'
(Note that &
is being escaped to \u0026amp;
, which seems correct. Most importantly, the leading slash is retained.)
Okay, I've now managed to fix the assertions, and to add a new one that actually fails and captures the problem in #59516. This should hopefully make it easier to track down the problem. |
(Oops, fat-fingered the Close button, and then "Delete branch" 😅 ) |
I also just reverted 467aade (i.e. #59021) locally and ran the tests again. This makes the first two assertions pass (and only the post meta related one fail, as expected), confirming that the unit test covers the bug 👍 |
Here are some tests I ran against https://github.com/WordPress/wordpress-develop: diff --git a/tests/phpunit/tests/post/getTheContent.php b/tests/phpunit/tests/post/getTheContent.php
index 91f0b621f3..d9030080db 100644
--- a/tests/phpunit/tests/post/getTheContent.php
+++ b/tests/phpunit/tests/post/getTheContent.php
@@ -84,4 +84,24 @@ class Tests_Post_GetTheContent extends WP_UnitTestCase {
$this->assertSame( 'Foo', get_the_content() );
}
+
+ public function test_the_content_block_attribute_escaping_1() {
+ $unescaped = '<!-- wp:navigation-link {"label":"News & About","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type"} /-->';
+ $escaped = str_replace( '&', '\u0026amp;', $unescaped );
+
+ $post = self::factory()->post->create_and_get( array( 'post_content' => $unescaped ) );
+
+ $actual = get_the_content( null, true, $post );
+ $this->assertSame( $escaped, $actual );
+ }
+
+ public function test_the_content_block_attribute_escaping_2() {
+ $unescaped = '<!-- wp:navigation-link {"label":"News & About","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type"} /-->';
+ $escaped = str_replace( '&', '\u0026amp;', $unescaped );
+
+ $post = self::factory()->post->create_and_get( array( 'post_content' => $escaped ) );
+
+ $actual = get_the_content( null, true, $post );
+ $this->assertSame( $escaped, $actual );
+ }
} (Note that This results in:
I think this means that we need to run This is backed up by a finding that @tjcafferkey just relayed to me: https://github.com/WordPress/wordpress-develop/blob/7002bce8abe6f6f1858bdd2f02cd1168ef50e4d8/src/wp-includes/post.php#L4532 |
Heads-up @getdave @youknowriad -- this is a WIP fix for https://core.trac.wordpress.org/ticket/60671, which is IMO problematic enough that we should get it into RC1. I think we're pretty close to resolving it (within the next 1-2 hours 🤞); it will then require a package sync. Does that work for y'all? |
That seems very tight, because we'll need one hour more to do the package release commit approximately and the commit freeze is approaching. Worst case, it lands in RC2. |
Gotcha, thank you! I'll keep you posted on our progress. |
I think I have an idea how to solve this. I don't think there's a function in WP to _un_escape stuff like (We'll still call |
The fix should be essentially working now. I'll have to update the unit test to reflect the changes (plus the PR title and description). |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @kylekelly. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change tests well for me, thanks @ockham! 🙏🏻
One TODO for later I think we can do to make this more clear is documenting the differences between the purposes of the incoming $post
(the stdClass
) which is being prepared to be written to the DB vs the get_post( $post->ID )
(the WP_Post
class) which is being retrieved from the DB. The latter mainly (I say mainly because its also used to retrieve meta) being used to provide context to the Block Hooks API
Note to self: Unit tests aside, we should verify that this also fixes https://core.trac.wordpress.org/ticket/60671. |
Why does using the |
See the diff. The reason we needed it in the first place was that we needed to persist the post content to the DB, after having inserted hooked blocks. We originally chose the However, since the duplicated |
Fix erroneous escaping of ampersands to `u0026amp;`. This is done by using the [`rest_pre_insert_{$this->post_type}`](https://developer.wordpress.org/reference/hooks/rest_pre_insert_this-post_type/) _filter_ rather than the `rest_insert_wp_navigation` _action_. This avoids calling `wp_update_post` twice, which was the original reason of the issue, as it removed the backslash from the already-encoded entity. Unlinked contributors: kylekelly. Co-authored-by: ockham <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: swissspidy <[email protected]>
This fix is critical to ensure correct behaviour of the Navigation block in WP 6.5. Without this blocks would start outputting erroneous characters on the front of the site which would constitute a bug in 6.5 but also break backwards compatbility with previous releases. I was also identified during the RC 1 phase. For these reasons I believe this PR is suitable for inclusion in WP 6.5 during the RC phase. |
Fix erroneous escaping of ampersands to `u0026amp;`. This is done by using the [`rest_pre_insert_{$this->post_type}`](https://developer.wordpress.org/reference/hooks/rest_pre_insert_this-post_type/) _filter_ rather than the `rest_insert_wp_navigation` _action_. This avoids calling `wp_update_post` twice, which was the original reason of the issue, as it removed the backslash from the already-encoded entity. Unlinked contributors: kylekelly. Co-authored-by: ockham <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: swissspidy <[email protected]>
I just cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release: b455d76 |
Fix erroneous escaping of ampersands to `u0026amp;`. This is done by using the [`rest_pre_insert_{$this->post_type}`](https://developer.wordpress.org/reference/hooks/rest_pre_insert_this-post_type/) _filter_ rather than the `rest_insert_wp_navigation` _action_. This avoids calling `wp_update_post` twice, which was the original reason of the issue, as it removed the backslash from the already-encoded entity. Unlinked contributors: kylekelly. Co-authored-by: ockham <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: swissspidy <[email protected]>
What?
Fix erroneous escaping of ampersands to
u0026amp;
.Why?
To fix #59516. Since these "entities" are missing a leading backslash, it them to show up verbatim on the frontend!
How?
By using the
rest_pre_insert_{$this->post_type}
filter rather than therest_insert_wp_navigation
action. This avoids callingwp_update_post
twice, which was the original reason of the issue, as it removed the backslash from the already-encoded entity.(Note that the fact that encoded entities are sent over the wire in the first place is due to a quirk in the Navigation block: #41063.)
Testing Instructions
(See CI, or run
npm run test:php
locally.)Related
https://core.trac.wordpress.org/ticket/60671 seems to be related