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

Blocks: Add post-block callback arg to traverse_and_serialize_block(), change signature #5257

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 20, 2023

Per #5247 (comment), I realized that sibling block insertion wasn't likely going to work the way I had planned. As a consequence, I had to come up with a new way to make it work.

Basically, we:

  • Change the signature of the existing callback such that:
    • the function arguments are now a reference to the current block (so it can be modified inline, which is important e.g. for theme attribute insertion), the parent block, and the previous block (instead of the block index and chunk index)
    • the return value is now a string that will be prepended to the result of the inner block traversal and serialization.
  • Add a second callback argument to traverse_and_serialize_block, which is called after the current inner block is traversed and serialized.
    • Its function arguments are a reference to the current block, the parent block, and the next block.

Its usage is demonstrated in #5261: The callback arguments are now sufficient to insert the serialized hooked blocks as strings, rather than via insert_inner_block, prepend_inner block, or append_inner_block.

Trac ticket: https://core.trac.wordpress.org/ticket/59412


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ockham ockham self-assigned this Sep 20, 2023
@ockham
Copy link
Contributor Author

ockham commented Sep 20, 2023

Rough diff on top of #5247 to demo usage:

diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
index 0efa18ea7a..153888ed51 100644
--- a/src/wp-includes/blocks.php
+++ b/src/wp-includes/blocks.php
@@ -760,8 +760,10 @@ function get_hooked_blocks( $name ) {
 	return $hooked_blocks;
 }
 
-function insert_hooked_blocks( $block, $parent, $block_index, $chunk_index ) {
+// FIXME: Better name.
+function insert_hooked_blocks( &$block, $parent = null ) {
 	$hooked_blocks = get_hooked_blocks( $block['blockName'] );
+	$markup = '';
 	foreach ( $hooked_blocks as $hooked_block_type => $relative_position ) {
 		$hooked_block = array(
 			'blockName'    => $hooked_block_type,
@@ -770,17 +772,40 @@ function insert_hooked_blocks( $block, $parent, $block_index, $chunk_index ) {
 			'innerContent' => array(),
 			'innerBlocks'  => array(),
 		);
+
+		$hooked_block_markup = serialize_block( $hooked_block );
+
 		if ( 'before' === $relative_position ) {
-			insert_inner_block( $parent, $block_index, $chunk_index, $hooked_block );
-		} elseif ( 'after' === $relative_position ) {
-			insert_inner_block( $parent, $block_index + 1, $chunk_index, $hooked_block );
+			$markup .= $hooked_block_markup;
 		} elseif ( 'first_child' === $relative_position ) {
-			prepend_inner_block( $block, $hooked_block );
+			// TODO.
 		} elseif ( 'last_child' === $relative_position ) {
-			append_inner_block( $block, $hooked_block );
+			// TODO.
 		}
 	}
-	return $block;
+	return $markup;
+}
+
+// FIXME: Better name.
+function post_insert_hooked_blocks( &$block, $parent = null ) {
+	$hooked_blocks = get_hooked_blocks( $block['blockName'] );
+	$markup = '';
+	foreach ( $hooked_blocks as $hooked_block_type => $relative_position ) {
+		$hooked_block = array(
+			'blockName'    => $hooked_block_type,
+			'attrs'        => array(),
+			'innerHTML'    => '',
+			'innerContent' => array(),
+			'innerBlocks'  => array(),
+		);
+
+		$hooked_block_markup = serialize_block( $hooked_block );
+
+		if ( 'after' === $relative_position ) {
+			$markup .= $hooked_block_markup;
+		}
+	}
+	return $markup;
 }
 
 /**
diff --git a/src/wp-includes/class-wp-block-patterns-registry.php b/src/wp-includes/class-wp-block-patterns-registry.php
index a83b35185b..adf2fa76bf 100644
--- a/src/wp-includes/class-wp-block-patterns-registry.php
+++ b/src/wp-includes/class-wp-block-patterns-registry.php
@@ -165,7 +165,10 @@ final class WP_Block_Patterns_Registry {
 			return null;
 		}
 
-		return $this->registered_patterns[ $pattern_name ];
+		$pattern            = $this->registered_patterns[ $pattern_name ];
+		$blocks             = parse_blocks( $pattern['content'] );
+		$pattern['content'] = traverse_and_serialize_blocks( $blocks, 'insert_hooked_blocks', 'post_insert_hooked_blocks' );
+		return $pattern;
 	}
 
 	/**

@ockham
Copy link
Contributor Author

ockham commented Sep 20, 2023

Demonstration of how to use it over at #5261.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@ockham ockham changed the title Hooked block insertion Blocks: Add post-block callback arg to traverse_and_serialize_block(), change signature Sep 20, 2023
@ockham ockham force-pushed the update/traverse-and-serialize-signature branch 2 times, most recently from 667830c to 3cb36ac Compare September 20, 2023 17:04
@ockham ockham marked this pull request as ready for review September 20, 2023 17:13
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks great. I left some nitpicks, mostly about the PHPDoc comments that are quite complex with the callbacks. I was even looking at other definitions of params that are callbacks, but I didn't find any interesting pattern to reuse here to make it nicer to present in the dev docs.

There is only one remaining question I raise, whether the top-level list of blocks, should also get the previous and the next element passed to the callbacks.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Show resolved Hide resolved
src/wp-includes/blocks.php Show resolved Hide resolved
During work on #59399, it was discovered that ''sibling'' block insertion wasn't likely going to work the way it was planned, which required devising an alternative solution. This new solution requires some changes to `traverse_and_serialize_block(s)`:

- Change the signature of the existing callback such that:
  - the return value is a string that will be prepended to the result of the inner block traversal and serialization;
  - the function arguments are: a ''reference'' to the current block (so it can be modified inline, which is important e.g. for `theme` attribute insertion), the parent block, and the previous block (instead of the block index and chunk index).
- Add a second callback argument to `traverse_and_serialize_block(s)`, which is called ''after'' the block is traversed and serialized.
  - Its function arguments are a reference to the current block, the parent block, and the next block.

Props gziolo.
Fixes #59412. See #59313.
@ockham ockham force-pushed the update/traverse-and-serialize-signature branch from ff183b9 to 4b89583 Compare September 21, 2023 08:31
@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

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

@ockham ockham closed this Sep 21, 2023
@ockham ockham deleted the update/traverse-and-serialize-signature branch September 21, 2023 08:33
$post_callback,
array( &$inner_block, $block, $next )
);
}
Copy link
Member

Choose a reason for hiding this comment

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

funny enough this kind of thing is where I've been exploring the use of a generator function as a context manager.

foreach ( traverse_and_serialize_block( $original_block ) as &$block ) {
	// do anything here
}

this kind of structure in PHP gets around some of the more awkward properties of passing objects/arrays as function arguments, or of the need to explicitly use closed values.

guess it probably doesn't handle the recursion that well though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants