#60671 closed defect (bug) (fixed)
If you hook some blocks with Block Hooks API, slashes are stripped from template part block attributes
Reported by: | karolmanijak | Owned by: | Bernhard Reiter |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | major | Version: | 6.5 |
Component: | Revisions | Keywords: | has-patch has-unit-tests dev-reviewed commit |
Focuses: | Cc: |
Description
Description
When you use Block Hooks API and hook any block and then edit template part which includes some escaped characters, then they're incorrectly escaped and two revisions are created.
In a consequence of incorrect escape, the blocks may become corrupted.
Steps to reproduce
- In your functions.php hook some block (I used an [example from docs](https://make.wordpress.org/core/2023/10/15/introducing-block-hooks-for-dynamic-blocks/):
function example_block_hooks( $hooked_blocks, $position, $anchor_block, $context ) { // Template/Template Part hooks. if ( $context instanceof WP_Block_Template ) { // Hooks the "Like" button block before the Post Title in the Single template. if ( 'core/post-title' === $anchor_block && 'before' === $position && 'single' === $context->slug ) { $hooked_blocks[] = 'ockham/like-button'; } // Hooks the Login/Logout link block after the Navigation block if the context of the template part is a header. if ( 'core/group' === $anchor_block && 'last_child' === $position && 'header' === $context->area ) { $hooked_blocks[] = 'core/loginout'; } } // Pattern hooks. if ( is_array( $context ) && isset( $context['slug'] ) ) { // Hooks into the Post Meta pattern in the Twenty Twenty-Four theme. if ( 'core/post-terms' === $anchor_block && 'after' === $position && 'twentytwentyfour/post-meta' === $context['slug'] ) { $hooked_blocks[] = 'ockham/like-button'; } } return $hooked_blocks; } add_filter( 'hooked_block_types', 'example_block_hooks', 10, 4 );
- Go to Editor -> Patterns -> General -> Post Meta
- Remove the content and create any paragraph with some content e.g. "test"
- Go to Inspector Controls of paragraph -> Advanced
- Add ADDITIONAL CSS CLASS(ES): "somebem--class" - this is to add some data to attributes
- Add BLOCK NAME: "mimicking bem -- class" - this is to add some data to attributes
- Switch to "Template Part" tab in Inspector Controls and check the number of revisions
- Save post
Actual:
- the number of revisions raised by 2
- you can check the revisions. Second revision "escapes" the special characters (dashes) and remove the backslash
\
Expected:
- the number of revisions raised by 1
- there's no incorrect escaping
- You can refresh the page and you can see block name is corrupted:
- Actual: "mimicking bem u002du002d class"
- Expected: "mimicking bem -- class"
This is small example but it may be more destructive in real scenarios.
More info
Reproducible in WordPress 6.5 Beta only (couldn't reproduce it in WordPress 6.4.3 even with Gutenberg 17.7 enabled).
Originally raised in https://github.com/woocommerce/woocommerce/issues/45176.
Change History (21)
#2
@
8 months ago
Thank you for reporting, @karolmanijak. I'll look into this shortly.
Possibly related: https://github.com/WordPress/gutenberg/issues/59516
#3
@
8 months ago
- Owner set to Bernhard Reiter
- Status changed from new to assigned
PR to pin-point (and fix) the issue: https://github.com/WordPress/gutenberg/pull/59561
This ticket was mentioned in PR #6225 on WordPress/wordpress-develop by @tomjcafferkey.
8 months ago
#4
- Keywords has-patch added
This change is inspired by this PR https://github.com/WordPress/gutenberg/pull/59561 since they are suffering from the same problem.
By creating a rest_pre_insert_{$this->post_type} filter in theWP_REST_Templates_Controller
rather than using the rest_insert_{$this->post_type}
action. This avoids calling wp_update_post
twice, which was the original reason for the issue, as it removed the backslash from the already-encoded entity.
By creating this filter it also creates consistency with the WP_REST_Posts_Controller
which has the same filter.
Trac ticket: https://core.trac.wordpress.org/ticket/60671
#5
@
8 months ago
- Milestone changed from Awaiting Review to 6.5
Tentatively setting Milestone to 6.5. This issue is very similar in nature to https://github.com/WordPress/gutenberg/issues/59516, except that it affects template parts (rather than Navigation blocks). A fix is underway, but it probably won't be ready in time for the commit freeze before RC1.
This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.
8 months ago
@Bernhard Reiter commented on PR #6225:
8 months ago
#7
This should be ready for review 😊
8 months ago
#8
By creating this filter it also creates consistency with the WP_REST_Posts_Controller which has the same filter.
That makes sense to me. Avoiding two database operations should be one of the goals of this fix. See my https://github.com/WordPress/wordpress-develop/pull/6225#discussion_r1513897139, where I'm unsure if it will work as intended for the logic that injects blocks.
@Bernhard Reiter commented on PR #6225:
8 months ago
#9
Is there test coverage? Can we add tests.
I'll add some 👍
@Bernhard Reiter commented on PR #6225:
8 months ago
#10
@swissspidy I've added some tests, both for the filter in isolation, and when used inside the controller. LMK if this looks good to you, or if you'd like anything else covered 🙂
@Bernhard Reiter commented on PR #6225:
8 months ago
#12
Committed to Core in https://core.trac.wordpress.org/changeset/57790.
#13
@
8 months ago
- Keywords dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening to request approval from another Core Committer to backport to the 6.5 branch.
This ticket was mentioned in PR #6244 on WordPress/wordpress-develop by @Bernhard Reiter.
8 months ago
#14
- Keywords has-unit-tests added
https://github.com/WordPress/wordpress-develop/pull/6225 included a test that involved setting the $wp_current_filter
global, and then removing it during teardown.
It was pointed out to me in Slack by @TimothyBJacobs that this is unnecessary:
Filters should get automatically reset by the parent
tearDown
implementation.
It's not the filter function I'm mocking though but the filter hook name (i.e. via the
$wp_current_filter
global)
I believe that should get reset as well. https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/tests/phpunit/includes/abstract-testcase.php#L382
This PR removes the unnecessary rest accordingly.
Trac ticket: https://core.trac.wordpress.org/ticket/60671
#15
@
8 months ago
Reopening to request approval from another Core Committer to backport to the 6.5 branch.
I tested and approved it on GitHub. @swissspidy, do we need anyone else to look at it before backporting to 6.5
branch?
@Bernhard Reiter commented on PR #6244:
8 months ago
#17
Committed to Core in https://core.trac.wordpress.org/changeset/57799.
#18
follow-up:
↓ 19
@
8 months ago
- Keywords dev-reviewed commit added; dev-feedback removed
@gziolo The rules are described in the handbook:
After a branch has been created for a release, each change (except for build and test suite changes) needs to be reviewed by two permanent committers. The first reviewer should set the keywords “commit dev-feedback”, and the second reviewer should set the keywords “commit dev-reviewed”. If a permanent committer created the patch, only one additional review is necessary.
[57790] and [57799] look good to me for 6.5.
Nit for [57799]: don't see a reason for mentioning the tests/phpunit/includes/abstract-testcase.php
file path in the new comment there. That will just become stale with any refactor, and any IDE will allow you to quickly jump to the definition. But it is what it is :)
#19
in reply to:
↑ 18
@
8 months ago
Replying to swissspidy:
Thank you very much! I'll backport them shortly.
Nit for [57799]: don't see a reason for mentioning the
tests/phpunit/includes/abstract-testcase.php
file path in the new comment there. That will just become stale with any refactor, and any IDE will allow you to quickly jump to the definition. But it is what it is :)
Ah, my bad, apologies! I'll keep a patch ready locally; I can maybe land that at some point if I need to tweak the tests some more.
cc: @bernhard-reiter @tomjcafferkey
As an example, the bug occurs when adding WooCommerce's Mini Cart block through Block Hooks API.