Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#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's profile karolmanijak Owned by: bernhard-reiter's profile 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

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

#1 @karolmanijak
8 months ago

cc: @bernhard-reiter @tomjcafferkey

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

#2 @Bernhard Reiter
8 months ago

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

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

#3 @Bernhard Reiter
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 @Bernhard Reiter
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 😊

@gziolo commented on PR #6225:


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 🙂

#11 @Bernhard Reiter
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#13 @Bernhard Reiter
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 @gziolo
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?

#16 @Bernhard Reiter
8 months ago

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.

#18 follow-up: @swissspidy
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 @Bernhard Reiter
8 months ago

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.

#20 @Bernhard Reiter
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#21 @Bernhard Reiter
8 months ago

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.

Note: See TracTickets for help on using tickets.