Skip to content
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

Backport: Templates perf: resolve patterns server side #6673

Closed
wants to merge 11 commits into from
70 changes: 70 additions & 0 deletions src/wp-includes/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -1344,6 +1344,76 @@ function traverse_and_serialize_block( $block, $pre_callback = null, $post_callb
);
}

/**
* Replaces patterns in a block tree with their content.
*
* @since 6.6.0
*
* @param array $blocks An array blocks.
*
* @return array An array of blocks with patterns replaced by their content.
Comment on lines +1352 to +1354

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I know, there’s no need to add an empty line between the @param and @return tags. Also, I noticed the description for the @inner_content function argument is missing. Could that be added?

Suggested change
* @param array $blocks An array blocks.
*
* @return array An array of blocks with patterns replaced by their content.
* @param array $blocks An array blocks.
* @param array $inner_content An optional array of inner content.
* @return array An array of blocks with patterns replaced by their content.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is meant to be a private arg, not sure if there's a better way https://github.com/WordPress/wordpress-develop/pull/6673/files#r1623854851

Copy link

@anton-vlasenko anton-vlasenko Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, there is no concept of a private argument in PHP. I'm also unsure how common it is to omit a function argument from a DocBlock if it is not intended for use.

However, there might be a solution to eliminate the need for the $inner_content function argument:

/**
 * Replaces patterns in a block tree with their content.
 *
 * @since 6.6.0
 *
 * @param array $blocks An array of blocks or a parent block.
 * @return array An array of blocks with patterns replaced by their content.
 */
function resolve_pattern_blocks( $blocks ) {
	// Keep track of seen references to avoid infinite loops.
	static $seen_refs = array();
	$i                = 0;
	$is_parent_block = ! empty( $blocks['innerBlocks'] );

	if ( $is_parent_block ) {
		// If it is a single block, $blocks must be reinitialized before it can be used.
		$block         = $blocks;
		$blocks        = $block['innerBlocks'];
		$inner_content = $block['innerContent'];
	}
	while ( $i < count( $blocks ) ) {
		if ( 'core/pattern' !== $blocks[ $i ]['blockName'] ) {
			if ( $is_parent_block ) {
				$blocks[ $i ] = resolve_pattern_blocks( $blocks[ $i ] );
			}
			++$i;
			continue;
		}
		$attrs = $blocks[ $i ]['attrs'];

		if ( empty( $attrs['slug'] ) ) {
			++$i;
			continue;
		}

		$slug = $attrs['slug'];

		if ( isset( $seen_refs[ $slug ] ) ) {
			// Skip recursive patterns.
			array_splice( $blocks, $i, 1 );
			continue;
		}

		$registry = WP_Block_Patterns_Registry::get_instance();
		$pattern  = $registry->get_registered( $slug );

		// Skip unknown patterns.
		if ( ! $pattern ) {
			++$i;
			continue;
		}

		$blocks_to_insert   = parse_blocks( $pattern['content'] );
		$seen_refs[ $slug ] = true;
		$blocks_to_insert   = resolve_pattern_blocks( $blocks_to_insert );
		unset( $seen_refs[ $slug ] );
		array_splice( $blocks, $i, 1, $blocks_to_insert );

		// If we have inner content, we need to insert nulls in the
		// inner content array, otherwise serialize_blocks will skip
		// blocks.
		if ( !empty( $inner_content ) ) {
			$null_indices  = array_keys( $inner_content, null, true );
			$content_index = $null_indices[ $i ];
			$nulls         = array_fill( 0, count( $blocks_to_insert ), null );
			array_splice( $inner_content, $content_index, 1, $nulls );
		}

		// Skip inserted blocks.
		$i += count( $blocks_to_insert );
	}

	if ( $is_parent_block ) {
		$block['innerBlocks']  = $blocks;
		$block['innerContent'] = $inner_content;
		return $block;
	}

	return $blocks;
}

Probably not optimal, but it passes the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I find it a bit more confusing. Also, why is it now sometimes returning a blocks array and sometimes one block?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I find it a bit more confusing.

I feel the same way.

why is it now sometimes returning a blocks array and sometimes one block?

I made this change to eliminate the $inner_content parameter, aiming to streamline the function's interface.
I've tried to refactor this function into two separate functions, but it didn't go well either. This likely indicates that the current structure might be the most efficient approach.

*/
function resolve_pattern_blocks( $blocks, &$inner_content = null ) {
// Keep track of seen references to avoid infinite loops.
static $seen_refs = array();
$i = 0;
while ( $i < count( $blocks ) ) {
if ( 'core/pattern' === $blocks[ $i ]['blockName'] ) {
$attrs = $blocks[ $i ]['attrs'];

if ( empty( $attrs['slug'] ) ) {
++$i;
continue;
}

$slug = $attrs['slug'];

if ( isset( $seen_refs[ $slug ] ) ) {
// Skip recursive patterns.
array_splice( $blocks, $i, 1 );
continue;
}

$registry = WP_Block_Patterns_Registry::get_instance();
$pattern = $registry->get_registered( $slug );

// Skip unknown patterns.
if ( ! $pattern ) {
++$i;
continue;
}

$blocks_to_insert = parse_blocks( $pattern['content'] );
$seen_refs[ $slug ] = true;
$blocks_to_insert = resolve_pattern_blocks( $blocks_to_insert );
unset( $seen_refs[ $slug ] );
array_splice( $blocks, $i, 1, $blocks_to_insert );

// If we have inner content, we need to insert nulls in the
// inner content array, otherwise serialize_blocks will skip
// blocks.
if ( $inner_content ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @inner_content is expected to be an array, it might be a good idea to explicitly check to ensure it is an array to avoid PHP warnings. Is there a guarantee that $blocks[$i]['innerBlocks'] is always an array?

Suggested change
if ( $inner_content ) {
if ( is_array( $inner_content ) && $inner_content ) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always an array, yes, there's also an empty check below. Also this argument is meant to be private, not sure if there's a better way to do it.

$null_indices = array_keys( $inner_content, null, true );
$content_index = $null_indices[ $i ];
$nulls = array_fill( 0, count( $blocks_to_insert ), null );
array_splice( $inner_content, $content_index, 1, $nulls );
}

// Skip inserted blocks.
$i += count( $blocks_to_insert );
} else {
if ( ! empty( $blocks[ $i ]['innerBlocks'] ) ) {
$blocks[ $i ]['innerBlocks'] = resolve_pattern_blocks(
$blocks[ $i ]['innerBlocks'],
$blocks[ $i ]['innerContent']
);
}
++$i;
}
}
return $blocks;
}

/**
* Given an array of parsed block trees, applies callbacks before and after serializing them and
* returns their concatenated output.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ protected function migrate_pattern_categories( $pattern ) {
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function prepare_item_for_response( $item, $request ) {
// Resolve pattern blocks so they don't need to be resolved client-side
// in the editor, improving performance.
$blocks = parse_blocks( $item['content'] );
$blocks = resolve_pattern_blocks( $blocks );
$item['content'] = serialize_blocks( $blocks );

$fields = $this->get_fields_for_response( $request );
$keys = array(
'name' => 'name',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,12 @@ protected function prepare_item_for_database( $request ) {
* @return WP_REST_Response Response object.
*/
public function prepare_item_for_response( $item, $request ) {
// Resolve pattern blocks so they don't need to be resolved client-side
// in the editor, improving performance.
$blocks = parse_blocks( $item->content );
$blocks = resolve_pattern_blocks( $blocks );
$item->content = serialize_blocks( $blocks );

// Restores the more descriptive, specific name for use within this method.
$template = $item;

Expand Down
72 changes: 72 additions & 0 deletions tests/phpunit/tests/blocks/resolvePatternBlocks.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php
/**
* Tests for resolve_pattern_blocks.
*
* @package WordPress
* @subpackage Blocks
*
* @since 6.6.0
*
* @group blocks
* @covers resolve_pattern_blocks
*/
class Tests_Blocks_ResolvePatternBlocks extends WP_UnitTestCase {
public function set_up() {
parent::set_up();

register_block_pattern(
'core/test',
array(
'title' => 'Test',
'content' => '<!-- wp:paragraph -->Hello<!-- /wp:paragraph --><!-- wp:paragraph -->World<!-- /wp:paragraph -->',
'description' => 'Test pattern.',
)
);
register_block_pattern(
'core/recursive',
array(
'title' => 'Recursive',
'content' => '<!-- wp:paragraph -->Recursive<!-- /wp:paragraph --><!-- wp:pattern {"slug":"core/recursive"} /-->',
'description' => 'Recursive pattern.',
)
);
}

public function tear_down() {
unregister_block_pattern( 'core/test' );
unregister_block_pattern( 'core/recursive' );

parent::tear_down();
}

/**
* @dataProvider data_should_resolve_pattern_blocks_as_expected
*
* @ticket 61228
*
* @param string $blocks A string representing blocks that need resolving.
* @param string $expected Expected result.
*/
public function test_should_resolve_pattern_blocks_as_expected( $blocks, $expected ) {
$actual = resolve_pattern_blocks( parse_blocks( $blocks ) );
$this->assertSame( $expected, serialize_blocks( $actual ) );
}

/**
* Data provider.
*
* @return array
*/
public function data_should_resolve_pattern_blocks_as_expected() {
return array(
// Works without attributes, leaves the block as is.
'pattern with no slug attribute' => array( '<!-- wp:pattern /-->', '<!-- wp:pattern /-->' ),
// Resolves the pattern.
'test pattern' => array( '<!-- wp:pattern {"slug":"core/test"} /-->', '<!-- wp:paragraph -->Hello<!-- /wp:paragraph --><!-- wp:paragraph -->World<!-- /wp:paragraph -->' ),
// Skips recursive patterns.
'recursive pattern' => array( '<!-- wp:pattern {"slug":"core/recursive"} /-->', '<!-- wp:paragraph -->Recursive<!-- /wp:paragraph -->' ),
// Resolves the pattern within a block.
'pattern within a block' => array( '<!-- wp:group --><!-- wp:paragraph -->Before<!-- /wp:paragraph --><!-- wp:pattern {"slug":"core/test"} /--><!-- wp:paragraph -->After<!-- /wp:paragraph --><!-- /wp:group -->', '<!-- wp:group --><!-- wp:paragraph -->Before<!-- /wp:paragraph --><!-- wp:paragraph -->Hello<!-- /wp:paragraph --><!-- wp:paragraph -->World<!-- /wp:paragraph --><!-- wp:paragraph -->After<!-- /wp:paragraph --><!-- /wp:group -->' ),
);
}
}
Loading