If you hook some blocks with Block Hooks API, slashes are stripped from template part block attributes

[karolmanijak]

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

1. 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 );

2. Go to Editor -> Patterns -> General -> Post Meta
3. Remove the content and create any paragraph with some content e.g. "test"
4. Go to Inspector Controls of paragraph -> Advanced
5. Add ADDITIONAL CSS CLASS(ES): "somebem--class" - this is to add some data to attributes
6. Add BLOCK NAME: "mimicking bem -- class" - this is to add some data to attributes
7. Switch to "Template Part" tab in Inspector Controls and check the number of revisions
8. 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

9. 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.

[karolmanijak]

cc: @bernhard-reiter @tomjcafferkey

As an example, the bug occurs when adding WooCommerce's Mini Cart block through Block Hooks API.

[Bernhard Reiter]

Thank you for reporting, @karolmanijak. I'll look into this shortly.

Possibly related: ​https://github.com/WordPress/gutenberg/issues/59516

[Bernhard Reiter]

PR to pin-point (and fix) the issue: ​https://github.com/WordPress/gutenberg/pull/59561

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

[Bernhard Reiter]

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 should be ready for review 😊

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.

Is there test coverage? Can we add tests.

I'll add some 👍

@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]

In 57790:

Block Hooks: Use new Templates Controller filter instead of action.

This changeset adds a new rest_pre_insert_{$this->post_type} filter in the WP_REST_Templates_Controller, where it is applied to the return value of the prepare_item_for_database method. (This is consistent with the WP_REST_Post_Controller, where that filter has existed before.)

The new filter is then used to inject hooked blocks into the template (or template part) content received via the endpoint, prior to persisting it to the database.

This supersedes the previous mechanism, which was using the rest_after_insert_{$this->post_type} action, from which it performed an additional wp_update_post call to update the template (part) content with the hooked blocks injected. The new technique eschews that additional call and the resulting extra revision it created, as well as a problem with regard to duplicated escaping and sanitization, which had caused some special characters to be garbled.

Props tomjcafferkey, gziolo, swissspidy, karolmanijak.
Fixes #60671.

Committed to Core in https://core.trac.wordpress.org/changeset/57790.

[Bernhard Reiter]

Reopening to request approval from another Core Committer to backport to the 6.5 branch.

​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

[gziolo]

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]

In 57799:

Block Hooks: Remove filter global reset from test teardown.

Resetting the $wp_current_filter global during test teardown is unnecessary, as it is taken care of by the unit test's base class.

This changeset removes the reset accordingly.

Follow-up [57790].
Props swissspidy, timothyblynjacobs.
See #60671.

Committed to Core in https://core.trac.wordpress.org/changeset/57799.

[swissspidy]

@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 :)

[Bernhard Reiter]

Replying to swissspidy:

[57790] and [57799] look good to me for 6.5.

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.

[Bernhard Reiter]

In 57802:

Block Hooks: Use new Templates Controller filter instead of action.

This changeset adds a new rest_pre_insert_{$this->post_type} filter in the WP_REST_Templates_Controller, where it is applied to the return value of the prepare_item_for_database method. (This is consistent with the WP_REST_Post_Controller, where that filter has existed before.)

The new filter is then used to inject hooked blocks into the template (or template part) content received via the endpoint, prior to persisting it to the database.

This supersedes the previous mechanism, which was using the rest_after_insert_{$this->post_type} action, from which it performed an additional wp_update_post call to update the template (part) content with the hooked blocks injected. The new technique eschews that additional call and the resulting extra revision it created, as well as a problem with regard to duplicated escaping and sanitization, which had caused some special characters to be garbled.

Reviewed by swissspidy.
Merges [57790] to the to the 6.5 branch.

Props tomjcafferkey, gziolo, swissspidy, karolmanijak.
Fixes #60671.

[Bernhard Reiter]

In 57803:

Block Hooks: Remove filter global reset from test teardown.

Resetting the $wp_current_filter global during test teardown is unnecessary, as it is taken care of by the unit test's base class.

This changeset removes the reset accordingly.

Follow-up [57790].

Reviewed by swissspidy.
Merges [57799] to the to the 6.5 branch.

Props swissspidy, timothyblynjacobs.
See #60671.