Blocks: Fix incorrect placement for hooked blocks in the parent conta…

…iner (#54349) * Blocks: Fix incorrect placement for hooked blocks in the parent container * Improve the way chunk index gets detected * Add unit tests covering hooking blocks at first and last child positions * Update lib/experimental/block-hooks.php Co-authored-by: Bernie Reiter <[email protected]> * Small tweaks after code review and rebasing with trunk. Props to @ockham for spotting issues. * Add tests covering the edge case where the container block does not have inner blocks * Improve test coverage for injecting hooked blocks --------- Co-authored-by: Bernie Reiter <[email protected]>
WordPress · Sep 14, 2023 · cd7370d · cd7370d
1 parent 999b49c
commit cd7370d
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 5 deletions.
diff --git a/lib/compat/wordpress-6.4/block-hooks.php b/lib/compat/wordpress-6.4/block-hooks.php
@@ -158,14 +158,28 @@ function gutenberg_insert_hooked_block( $inserted_block, $relative_position, $an
 				array_unshift( $block['innerBlocks'], $inserted_block );
 				// Since WP_Block::render() iterates over `inner_content` (rather than `inner_blocks`)
 				// when rendering blocks, we also need to prepend a value (`null`, to mark a block
-				// location) to that array.
-				array_unshift( $block['innerContent'], null );
+				// location) to that array after HTML content for the inner blocks wrapper.
+				$chunk_index = 0;
+				for ( $index = $chunk_index; $index < count( $block['innerContent'] ); $index++ ) {
+					if ( is_null( $block['innerContent'][ $index ] ) ) {
+						$chunk_index = $index;
+						break;
+					}
+				}
+				array_splice( $block['innerContent'], $chunk_index, 0, array( null ) );
 			} elseif ( 'last_child' === $relative_position ) {
 				array_push( $block['innerBlocks'], $inserted_block );
 				// Since WP_Block::render() iterates over `inner_content` (rather than `inner_blocks`)
-				// when rendering blocks, we also need to prepend a value (`null`, to mark a block
-				// location) to that array.
-				array_push( $block['innerContent'], null );
+				// when rendering blocks, we also need to correctly append a value (`null`, to mark a block
+				// location) to that array before the remaining HTML content for the inner blocks wrapper.
+				$chunk_index = count( $block['innerContent'] );
+				for ( $index = count( $block['innerContent'] ); $index > 0; $index-- ) {
+					if ( is_null( $block['innerContent'][ $index - 1 ] ) ) {
+						$chunk_index = $index;
+						break;
+					}
+				}
+				array_splice( $block['innerContent'], $chunk_index, 0, array( null ) );
 			}
 			return $block;
 		}

diff --git a/phpunit/fixtures/hooked-block/block.json b/phpunit/fixtures/hooked-block/block.json
@@ -0,0 +1,7 @@
+{
+	"name": "tests/hooked-block",
+	"blockHooks": {
+		"tests/group-first-child": "firstChild",
+		"tests/group-last-child": "lastChild"
+	}
+}
diff --git a/phpunit/tests/blocks/renderHookedBlocks.php b/phpunit/tests/blocks/renderHookedBlocks.php
@@ -0,0 +1,160 @@
+<?php
+
+use PHP_CodeSniffer\Generators\HTML;
+
+/**
+ * Tests for hooked blocks rendering.
+ *
+ * @package WordPress
+ * @subpackage Blocks
+ * @since 6.4.0
+ *
+ * @group blocks
+ */
+class Tests_Blocks_RenderHookedBlocks extends WP_UnitTestCase {
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_first_child_position() {
+		$content = <<<HTML
+<!-- wp:tests/group-first-child {"layout":{"type":"constrained"}} -->
+	<div class="wp-block-group">
+		<!-- wp:paragraph -->
+			<p>Foo</p>
+		<!-- /wp:paragraph -->
+	</div>
+<!-- /wp:tests/group-first-child -->
+HTML;
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		$expected_result = <<<HTML
+<!-- wp:tests/group-first-child {"layout":{"type":"constrained"}} -->
+	<div class="wp-block-group">
+		<!-- wp:tests/hooked-block /--><!-- wp:paragraph -->
+			<p>Foo</p>
+		<!-- /wp:paragraph -->
+	</div>
+<!-- /wp:tests/group-first-child -->
+HTML;
+		$this->assertSame( $expected_result, $result );
+	}
+
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_first_child_position_no_inner_content() {
+		$content = '<!-- wp:tests/group-first-child /-->';
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		$expected_result = '<!-- wp:tests/group-first-child --><!-- wp:tests/hooked-block /--><!-- /wp:tests/group-first-child -->';
+		$this->assertSame( $expected_result, $result );
+	}
+
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_first_child_position_no_child_blocks() {
+		$content = <<<HTML
+<!-- wp:tests/group-first-child -->
+	<div class="wp-block-group"></div>
+<!-- /wp:tests/group-first-child -->
+HTML;
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		// @todo In the perfect world, the hooked block would be injected inside the `div` tag.
+		$expected_result = <<<HTML
+<!-- wp:tests/group-first-child --><!-- wp:tests/hooked-block /-->
+	<div class="wp-block-group"></div>
+<!-- /wp:tests/group-first-child -->
+HTML;
+		$this->assertSame( $expected_result, $result );
+	}
+
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_last_child_position() {
+		$content = <<<HTML
+<!-- wp:tests/group-last-child {"layout":{"type":"constrained"}} -->
+	<div class="wp-block-group">
+		<!-- wp:paragraph -->
+			<p>Foo</p>
+		<!-- /wp:paragraph -->
+	</div>
+<!-- /wp:tests/group-last-child -->
+HTML;
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		$expected_result = <<<HTML
+<!-- wp:tests/group-last-child {"layout":{"type":"constrained"}} -->
+	<div class="wp-block-group">
+		<!-- wp:paragraph -->
+			<p>Foo</p>
+		<!-- /wp:paragraph --><!-- wp:tests/hooked-block /-->
+	</div>
+<!-- /wp:tests/group-last-child -->
+HTML;
+		$this->assertSame( $expected_result, $result );
+	}
+
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_last_child_position_no_inner_content() {
+		$content = '<!-- wp:tests/group-last-child /-->';
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		$expected_result = '<!-- wp:tests/group-last-child --><!-- wp:tests/hooked-block /--><!-- /wp:tests/group-last-child -->';
+		$this->assertSame( $expected_result, $result );
+	}
+
+	/**
+	 * @ticket 59313
+	 */
+	public function test_inject_hooked_block_at_last_child_position_no_child_blocks() {
+		$content = <<<HTML
+<!-- wp:tests/group-last-child -->
+	<div class="wp-block-group"></div>
+<!-- /wp:tests/group-last-child -->
+HTML;
+
+		$block_type = register_block_type( GUTENBERG_DIR_TESTFIXTURES . '/hooked-block/' );
+		$blocks     = parse_blocks( $content );
+		$result     = gutenberg_serialize_blocks( $blocks );
+
+		unregister_block_type( $block_type->name );
+
+		// @todo In the perfect world, the hooked block would be injected inside the `div` tag.
+		$expected_result = <<<HTML
+<!-- wp:tests/group-last-child -->
+	<div class="wp-block-group"></div>
+<!-- wp:tests/hooked-block /--><!-- /wp:tests/group-last-child -->
+HTML;
+		$this->assertSame( $expected_result, $result );
+	}
+}