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

Comment Template Block: Retain inner blocks inserted via render_block_data filter #50883

Merged

Conversation

ockham
Copy link
Contributor

@ockham ockham commented May 23, 2023

What?

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.

Follow up to #50279.

Why?

See https://github.com/WordPress/gutenberg/pull/50279/files#r1184916607.

How?

The unit test: By adding a render_block_data filter to insert a Social Icons block, and a filter mock to the render_block hook to check the length of the Comment Template block's inner blocks.

The fix: By creating a new WP_Block instance from the parsed block (similar to how we did things before #50279; however, we continue to set block context via the render_block_context filter).

Testing Instructions

See CI. Locally, this can be tested via npm run test:unit:php -- --group=blocks.

@ockham ockham added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Block] Comment Template Affects the Comment Template Block labels May 23, 2023
@ockham ockham self-assigned this May 23, 2023
@ockham ockham force-pushed the add/test-coverage-for-inner-blocks-added-in-render-block-data branch from ddabd04 to 0f1fb46 Compare May 23, 2023 16:02
@ockham ockham changed the title Comment Template Block: Test that inner block inserted via render_blo…ck_data filter is retained Comment Template Block: Test that inner block inserted via render_block_data filter is retained May 23, 2023
@github-actions
Copy link

github-actions bot commented May 25, 2023

Flaky tests detected in 703e6c7.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5122178734
📝 Reported issues:

@ockham
Copy link
Contributor Author

ockham commented May 25, 2023

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();

@ockham ockham changed the title Comment Template Block: Test that inner block inserted via render_block_data filter is retained Comment Template Block: Retain inner blocks inserted via render_block_data filter May 25, 2023
@ockham ockham marked this pull request as ready for review May 25, 2023 09:30
phpunit/blocks/render-comment-template-test.php Outdated Show resolved Hide resolved
* 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 ) );
Copy link
Member

@gziolo gziolo May 26, 2023

Choose a reason for hiding this comment

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

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:

https://github.com/WordPress/wordpress-develop/blob/d46c47f0fdd43ad67743fb390187c4e32876d8a3/src/wp-includes/blocks.php#L1072

https://github.com/WordPress/wordpress-develop/blob/d46c47f0fdd43ad67743fb390187c4e32876d8a3/src/wp-includes/class-wp-block-list.php#L96

It would be great to investigate with some additional unit test. I guess it could be another PR, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😊

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

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 🙂 .

@ockham ockham force-pushed the add/test-coverage-for-inner-blocks-added-in-render-block-data branch from 8375cb7 to 977c5f0 Compare May 30, 2023 13:22
@ockham ockham force-pushed the add/test-coverage-for-inner-blocks-added-in-render-block-data branch from 977c5f0 to 1082780 Compare May 30, 2023 13:28
@ockham ockham merged commit 660b34c into trunk May 30, 2023
@ockham ockham deleted the add/test-coverage-for-inner-blocks-added-in-render-block-data branch May 30, 2023 16:57
@github-actions github-actions bot added this to the Gutenberg 16.0 milestone May 30, 2023
sethrubenstein pushed a commit to pewresearch/gutenberg that referenced this pull request Jul 13, 2023
…_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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comment Template Affects the Comment Template Block [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants