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.

Sign up for GitHub

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

Fix block supports for inner blocks #26344

Merged
merged 4 commits into from
Oct 21, 2020
Merged

Fix block supports for inner blocks #26344

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84cb8ed
Hook into the render callback of blocks
oandregal Oct 21, 2020
1e2c7f7
Keep track of the current parsed block
oandregal Oct 21, 2020
3f6a46c
Rename, clarify, and refactor
oandregal Oct 21, 2020
304e9ba
Make linter happy
oandregal Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions lib/compat.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,18 +366,11 @@ function gutenberg_replace_default_block_categories( $default_categories ) {
}
add_filter( 'block_categories', 'gutenberg_replace_default_block_categories' );

global $current_parsed_block;
$current_parsed_block = array(
'blockName' => null,
'attributes' => null,
);

/**
* Shim that hooks into `pre_render_block` so as to override `render_block` with
* a function that assigns block context.
*
* The context handling can be removed when plugin support requires WordPress 5.5.0+.
* The global current_parsed_block assignment can be removed when plugin support requires WordPress 5.6.0+.
*
* @see https://core.trac.wordpress.org/ticket/49927
* @see https://core.trac.wordpress.org/changeset/48243
Expand All @@ -389,7 +382,6 @@ function gutenberg_replace_default_block_categories( $default_categories ) {
*/
function gutenberg_render_block_with_assigned_block_context( $pre_render, $parsed_block ) {
global $post, $wp_query;
global $current_parsed_block;

/*
* If a non-null value is provided, a filter has run at an earlier priority
Expand All @@ -399,8 +391,6 @@ function gutenberg_render_block_with_assigned_block_context( $pre_render, $parse
return $pre_render;
}

$current_parsed_block = $parsed_block;

$source_block = $parsed_block;

/** This filter is documented in src/wp-includes/blocks.php */
Expand Down Expand Up @@ -518,3 +508,40 @@ function gutenberg_override_reusable_block_post_type_labels() {
);
}
add_filter( 'post_type_labels_wp_block', 'gutenberg_override_reusable_block_post_type_labels', 10, 0 );

global $current_parsed_block;
$current_parsed_block = array(
'blockName' => null,
'attributes' => null,
);

/**
* Wraps the render_callback of dynamic blocks to keep track
* of the current block being rendered via a global variable
* called $current_parsed_block.
*
* This is for get_block_wrapper_attributes to get access
* to the runtime data of the block being rendered.
*
* This shim can be removed when the plugin requires WordPress 5.6.
*
* @since 9.2.1
*
* @param array $args Block attributes.
* @return array Block attributes.
*/
function gutenberg_current_parsed_block_tracking( $args ) {
if ( null !== $args['render_callback'] ) {
$block_render_callback = $args['render_callback'];
$args['render_callback'] = function( $attributes, $content, $block ) use ( $block_render_callback ) {
Copy link
Contributor

@ockham ockham Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @nosolosw, I've tried running GB 9.2.1 in our CI environment, and I got a test failure that pointed me at this line:

ArgumentCountError : Too few arguments to function {closure}(), 2 passed in /home/_redacted_/trunk/wp-includes/class-wp-block-type.php on line 186 and exactly 3 expected
 /home/_redacted_/trunk/wp-content/plugins/gutenberg-core/9.2.1/lib/compat.php:536
 /home/_redacted_/trunk/wp-includes/class-wp-block-type.php:186
 /home/_redacted_/trunk/bin/tests/core/tests/blocks/render.php:325

Checking the callsite of render_callback in the WP_Block_Type class' render method, I see indeed only 2 arguments passed to render_callback.

In the WP_Block class' render method OTOH, render_callback is indeed invoked with 3 arguments. I'm not entirely sure which render method is called when, but I wanted to flag this, since apparently in our test setup, the wrong one is being called. (Or maybe there shouldn't be two of them?)

Edit: This is the failing test: https://github.com/WordPress/wordpress-develop/blob/fe2053f2c1cff0c416112103988e832687ca3836/tests/phpunit/tests/blocks/render.php#L324. It invokes the render() method of an object of class WP_Block_Type, so it's quite natural that it fails.

cc/ @sirreal @WunderBart @fullofcaffeine

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you mind sharing the full content of the test (which block was it testing and so on)? I'd like to understand that block's attributes and its inner blocks.

Another thing: are you running WordPress 5.6 beta or WordPress 5.5.2?

Finally, there's #26356 that has improved over this PR, although it hasn't changed the part that is giving you the failure (just moved it to a different place).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind sharing the full content of the test (which block was it testing and so on)? I'd like to understand that block's attributes and its inner blocks.

Hey @nosolosw, I've edited my original comment to include a pointer to the failing test, not sure you saw:

Edit: This is the failing test: https://github.com/WordPress/wordpress-develop/blob/fe2053f2c1cff0c416112103988e832687ca3836/tests/phpunit/tests/blocks/render.php#L324. It invokes the render() method of an object of class WP_Block_Type, so it's quite natural that it fails.

Since this is a Core test, it's a bit surprising that that test wasn't run by GB's own CI 🤔

Another thing: are you running WordPress 5.6 beta or WordPress 5.5.2?

5.5 🙂 This is a familiar, large-scale, multisite install 😉

Finally, there's #26356 that has improved over this PR, although it hasn't changed the part that is giving you the failure (just moved it to a different place).

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Per DM with @nosolosw, our current hypothesis is that WP_Block_Type->render() is basically dead code that's only used by that test, and that we should update that test to remove WP_Block->render() instead, and remove WP_Block_Type->render() altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's #26356 to port similar changes to core and it's reporting similar errors. I'm taking a look.

global $current_parsed_block;
$parent_parsed_block = $current_parsed_block;
$current_parsed_block = $block->parsed_block;
$result = $block_render_callback( $attributes, $content, $block );
$current_parsed_block = $parent_parsed_block;
return $result;
};
}
return $args;
}
add_filter( 'register_block_type_args', 'gutenberg_current_parsed_block_tracking' );