Post Template Block: Set block context via filter #50313
Conversation
ockham
requested review from
gziolo,
michalczaplinski,
andrewserong and
ntsekouras
| // Set the block name to one that does not correspond to an existing registered block. | ||
| // This ensures that for the inner instances of the Post Template block, we do not render any block supports. | ||
| $block_instance['blockName'] = 'core/null'; | ||
|
|
||
| $block->name = 'core/null'; |
There was a problem hiding this comment.
| ) | ||
| ) | ||
| )->render( array( 'dynamic' => false ) ); | ||
| $block_content = $block->render( array( 'dynamic' => false ) ); |
There was a problem hiding this comment.
I left a similar comment under the PR that updates the Comment Template block. Have you considered passing the original context $block->context together with the locally modified context? I guess both approaches will work. The only question is why we landed on using the approach with a separate instance of WP_Block in this block. @ntsekouras, any insights we could miss?
There was a problem hiding this comment.
I discovered something on the Comments Template block PR that might be relevant here: https://github.com/WordPress/gutenberg/pull/50279/files#r1184916607
There was a problem hiding this comment.
The only question is why we landed on using the approach with a separate instance of WP_Block in this block. @ntsekouras, any insights we could miss?
I'm not really sure, but maybe even the render_block_context filter wasn't even in code yet. It seems both Query and this filter were introduced in the same WP version(5.5). There is a discussion about the context in the original PR, but nothing regarding the new instance.
There was a problem hiding this comment.
I'm still not sure why the issue with the layout classes is happening, but the culprit is definitely this change. Logging out $block_content shows the classes are already present in the nested block. This happens independently of whether the block in question supports layout or not: I can see the classes locally being added to Post Title.
There was a problem hiding this comment.
My current (and somewhat uninformed) hunch is that prior to this PR, we were setting the block name to core/null for the copy of the block that we created on the fly, whereas now, we're setting it directly on the block itself. Maybe setting the block name back to its original value after rendering could fix this. I'll give that a try.
There was a problem hiding this comment.
Ah, I think I was slightly off. Judging by this comment
// Set the block name to one that does not correspond to an existing registered block.
// This ensures that for the inner instances of the Post Template block, we do not render any block supports....the whole point of setting the block name to core/null is to prevent the application of block supports -- which would lead to the duplication of class names that we're seeing here. So it's basically not working as it's supposed to.
My new hunch is that setting only the ->name isn't enough: Previously, we were setting the parsed block's blockName field, which is used by the WP_Block constructor to set the ->name but also the ->block_type.
So I think it's the latter that's missing here.
There was a problem hiding this comment.
Thanks for the ping and for the explanation surrounding using render_block_context 👍
In testing, I noticed that this PR appears to result in an unexpected classname appearing in children of the li item. Specifically the inner wrapper classname for the post template block appears to be duplicated in the child of the li item, where it shouldn't occur.
On trunk, for example, wp-block-post-template-is-layout-flow appears on the wrapper ul element, and not in the Group block that is a direct child of the Post Template block:
With this PR, wp-block-post-template-is-layout-flow appears to be duplicated, that is, it's appears in the wrapper ul element, and is also appearing in the rendered markup for the Group block that is a direct child of the Post Template block:
From observing that, my guess is that by re-using $block instead of a new WP_Block, additional data is possibly being passed along / process / generated, that shouldn't be?
CC: @tellthemachines since this relates to layout classes.
|
I can reproduce @andrewserong's issue above in the editor - I'm not at all sure what's happening at first glance 😕 but will try to look into it a bit more. |
|
You can achieve a similar goal by correctly propagating the available context to the rendered inner blocks with: diff --git a/packages/block-library/src/post-template/index.php b/packages/block-library/src/post-template/index.php
index 3a3c207cf9..4797faf69a 100644
--- a/packages/block-library/src/post-template/index.php
+++ b/packages/block-library/src/post-template/index.php
@@ -90,9 +90,12 @@ function render_block_core_post_template( $attributes, $content, $block ) {
$block_content = (
new WP_Block(
$block_instance,
- array(
- 'postType' => get_post_type(),
- 'postId' => get_the_ID(),
+ array_merge(
+ $block->context,
+ array(
+ 'postType' => get_post_type(),
+ 'postId' => get_the_ID(),
+ )
)
)
)->render( array( 'dynamic' => false ) );In the case of the Post Template Block it shouldn't make any difference because the only additional context available is the |
|
Glad you have this subject opened. I opened a track ticket recently with some code I've used to propagate the context based on the provides_context attribute. Here's the link to the ticket : https://core.trac.wordpress.org/ticket/58278#ticket And here's the proposed implementation if that helps : |
|
I left some feedback #50279 (comment) based on my explorations on the changes that landed in |
|
As next steps for this PR, I'd like to:
|
Unfortunately, that's not quite equivalent to the solution proposed by this PR's branch; see #50279 (comment), where I explain the difference (for the Comment Template block in that case, but the same applies to the Post Template block). |
In the Post Template block's render callback, instead of creating a new `WP_Block` instance with `postId` and `postType` context set, use the `render_block_context` filter to set that context and render the existing `WP_Block` instance. This approach arguably follows our established patterns with regard to handling block context better. Notably, with the previous approach, we were only setting block context for the Post Template block itself. In this PR, we extend it to apply to all child blocks, including ones that are dynamically inserted, e.g. via the `render_block` filter. This is relevant for Auto-inserting blocks (see #50103). This follows the precedent of the Comment Template block, see #50279.
ockham
force-pushed
the
update/post-template-render-context
branch
from
e22af2d
to
8695bd7
Compare
Done in 8695bd7, which passes on (I also rebased.) Edit: Aside, we might want to include this test in |
|
Flaky tests detected in 8add1e6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5079572629
|
|
My new discovery is that we operate with two different states for the The context that to the current block opted to with public $context = array();The accumulated context exposed by all parent blocks with protected $available_context;I realized that my attempt shared in #50313 (comment) is incorrect because we should be passing Here is the test case that I copied and modified for the Post Template block from WordPress core that covers the block context functionality: It's going to be necessary to apply additional modifications for Post Template and Comment Template blocks to ensure that the available context is correctly passed from parent blocks: diff --git a/phpunit/blocks/render-post-template-test.php b/phpunit/blocks/render-post-template-test.php
index 13b5623cdd..2718126b8f 100644
--- a/phpunit/blocks/render-post-template-test.php
+++ b/phpunit/blocks/render-post-template-test.php
@@ -13,6 +13,13 @@ class Tests_Blocks_RenderPostTemplateBlock extends WP_UnitTestCase {
private static $post;
private static $other_post;
+ /**
+ * Registered block names.
+ *
+ * @var string[]
+ */
+ private $registered_block_names = array();
+
public function set_up() {
parent::set_up();
@@ -39,6 +46,37 @@ class Tests_Blocks_RenderPostTemplateBlock extends WP_UnitTestCase {
);
}
+ /**
+ * Tear down each test method.
+ */
+ public function tear_down() {
+ while ( ! empty( $this->registered_block_names ) ) {
+ $block_name = array_pop( $this->registered_block_names );
+ unregister_block_type( $block_name );
+ }
+
+ parent::tear_down();
+ }
+
+ /**
+ * Registers a block type.
+ *
+ * @param string|WP_Block_Type $name Block type name including namespace, or alternatively a
+ * complete WP_Block_Type instance. In case a WP_Block_Type
+ * is provided, the $args parameter will be ignored.
+ * @param array $args {
+ * Optional. Array of block type arguments. Any arguments may be defined, however the
+ * ones described below are supported by default. Default empty array.
+ *
+ * @type callable $render_callback Callback used to render blocks of this block type.
+ * }
+ */
+ private function register_block_type( $name, $args ) {
+ register_block_type( $name, $args );
+
+ $this->registered_block_names[] = $name;
+ }
+
public function test_rendering_post_template() {
$parsed_blocks = parse_blocks(
'<!-- wp:post-template --><!-- wp:post-title /--><!-- wp:post-excerpt /--><!-- /wp:post-template -->'
@@ -70,4 +108,62 @@ END;
str_replace( array( "\n", "\t" ), '', $markup )
);
}
+
+ public function test_provides_block_context() {
+ $provided_context = array();
+
+ $this->register_block_type(
+ 'gutenberg/test-context-provider',
+ array(
+ 'attributes' => array(
+ 'foo' => array(
+ 'type' => 'string',
+ ),
+ ),
+ 'provides_context' => array(
+ 'gutenberg/foo' => 'foo',
+ ),
+ )
+ );
+
+ $this->register_block_type(
+ 'gutenberg/test-context-consumer',
+ array(
+ 'uses_context' => array(
+ 'gutenberg/foo',
+ ),
+ 'render_callback' => static function( $attributes, $content, $block ) use ( &$provided_context ) {
+ $provided_context[] = $block->context;
+
+ return '';
+ },
+ )
+ );
+
+ $parsed_blocks = parse_blocks(
+ '<!-- wp:gutenberg/test-context-provider {"foo":"bar"} -->' .
+ '<!-- wp:post-template -->' .
+ '<!-- wp:gutenberg/test-context-consumer /-->' .
+ '<!-- /wp:post-template -->' .
+ '<!-- /wp:gutenberg/test-context-provider -->'
+ );
+
+ render_block( $parsed_blocks[0] );
+
+ $this->assertSame(
+ array(
+ array(
+ 'gutenberg/foo' => 'bar',
+ 'postType' => self::$other_post->post_type,
+ 'postId' => self::$other_post->ID,
+ ),
+ array(
+ 'gutenberg/foo' => 'bar',
+ 'postType' => self::$post->post_type,
+ 'postId' => self::$post->ID,
+ ),
+ ),
+ $provided_context
+ );
+ }
}At the moment, the test fails with the following error: 
It confirms that this PR resolves one of the existing issues for passing down |
Ah, nice find! I was trying to find test coverage for the Post Template block in Core and was surprised that I couldn't find any 😅
Thanks a lot for investigating, and for posting the unit test and fix! Happy to land this, and to iterate on the additional fixes separately 😊 |
There is no fix yet. We can't access |
* Post Template Block: Set block context via filter In the Post Template block's render callback, use the `render_block_context` filter to set `postId` and `postType` context, rather than passing that context directly to the `WP_Block` constructor. This approach arguably follows our established patterns with regard to handling block context better. Notably, with the previous approach, we were only setting block context for the Post Template block itself. In this PR, we extend it to apply to all child blocks, including ones that are dynamically inserted, e.g. via the `render_block` filter. This is relevant for Auto-inserting blocks (see WordPress#50103). This follows the precedent of the Comment Template block, see WordPress#50279. Furthermore, add some test coverage to guard against duplicated block-supports class names, which was an issue in a previous iteration of this PR.
|
I've seen this PR landed in WP6.3. With this code, is it possible to tell the post template to provide the queryId as context to the block below it? By reading the Thank you for your explanations. |
What?
In the Post Template block's render callback, instead of creating a new
WP_Blockinstance withpostIdandpostTypecontext set, use therender_block_contextfilter to set that context and render the existingWP_Blockinstance.Why?
This approach arguably follows our established patterns with regard to handling block context better. Notably, with the previous approach, we were only setting block context for the Post Template block itself. In this PR, we extend it to apply to all child blocks, including ones that are dynamically inserted, e.g. via the
render_blockfilter. This is relevant for Auto-inserting blocks (see #50103).This follows the precedent of the Comment Template block, see #50279.
How?
See "What".
Testing Instructions
Verify that the Post Template block (and children) are still working as expected on the frontend:
trunk, create a number of posts. View on the frontend, and take note (and a screenshot) of what the posts look like.Testing Instructions for Keyboard
N/A.
Screenshots or screencast
N/A.