Conversation
ddabd04 to
0f1fb46
Compare
|
Flaky tests detected in 703e6c7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5122178734
|
|
Note that the test is currently failing (as expected, per this comment). (It passes however after reverting 4409ba4 and rebuilding blocks). For a more permanent fix -- that will also retain the behavior encoded in #50879 -- I'm considering applying this patch to this PR: diff --git a/packages/block-library/src/comment-template/index.php b/packages/block-library/src/comment-template/index.php
index 6a8698f60f..d3ffc67cf8 100644
--- a/packages/block-library/src/comment-template/index.php
+++ b/packages/block-library/src/comment-template/index.php
@@ -31,7 +31,7 @@ function block_core_comment_template_render_comments( $comments, $block ) {
return $context;
};
add_filter( 'render_block_context', $filter_block_context );
- $block_content = $block->render( array( 'dynamic' => false ) );
+ $block_content = ( new WP_Block( $block->parsed_block ) )->render( array( 'dynamic' => false ) );
remove_filter( 'render_block_context', $filter_block_context );
$children = $comment->get_children(); |
| * We construct a new WP_Block instance from the parsed block so that | ||
| * it'll receive any changes made by the `render_block_data` filter. | ||
| */ | ||
| $block_content = ( new WP_Block( $block->parsed_block ) )->render( array( 'dynamic' => false ) ); |
There was a problem hiding this comment.
I believe that we still need to propagate the available context here to avoid issues like the one reported in https://core.trac.wordpress.org/ticket/58278 by @jdm-web. See also his comment in #50313 (comment).
I checked how we instantiate WP_Block in WordPress core and it both places the upper context gets passed down:
It would be great to investigate with some additional unit test. I guess it could be another PR, too.
There was a problem hiding this comment.
Yeah, I'd prefer to tackle this separately. Since we're basically restoring the way we rendered the block from prior to #50279 here -- except for how we set block context -- I don't think this PR marks a regression compared to that state; if anything, it's a small improvement with regard to (parts of) block context. So I think it's fine to iterate separately 😊
michalczaplinski
left a comment
There was a problem hiding this comment.
LGTM but FWIW I've just learned from you about this need to construct a new WP_Block instance so that the render_block_data filter works correctly 🙂 .
8375cb7 to
977c5f0
Compare
977c5f0 to
1082780
Compare
…_data filter (WordPress#50883) Add a unit test to verify that an inner block added to a given Comment Template block via the `render_block_data` filter is retained at `render_block` time. Then, change the block's code to actually retain those modifications from `render_block_data`.
What?
Add a unit test to verify that an inner block added to a given Comment Template block via the
render_block_datafilter is retained atrender_blocktime. Then, change the block's code to actually retain those modifications fromrender_block_data.Follow up to #50279.
Why?
See https://github.com/WordPress/gutenberg/pull/50279/files#r1184916607.
How?
The unit test: By adding a
render_block_datafilter to insert a Social Icons block, and a filter mock to therender_blockhook to check the length of the Comment Template block's inner blocks.The fix: By creating a new
WP_Blockinstance from the parsed block (similar to how we did things before #50279; however, we continue to set block context via therender_block_contextfilter).Testing Instructions
See CI. Locally, this can be tested via
npm run test:unit:php -- --group=blocks.