-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix block supports for inner blocks #26344
Conversation
Size Change: +4 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I see there are some failing e2e tests that are also causing issues on the master branch. This specific PR is a fix that needs to go in an upcoming patch version, so I'm going to merge. |
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 ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 classWP_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).
👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Alternative to #26311
It fixes an issue with updating the
current_parsed_block
global value with inner blocks for the plugin, in accordance to what we did for WP core at #26291 (ported over at WordPress/wordpress-develop#626).