WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 2 months ago

Last modified 2 months ago

#48104 closed enhancement (invalid)

Add the gutenberg_provide_render_callback_with_block_object shim to core

Reported by: andraganescu Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch needs-testing needs-dev-note needs-docs
Focuses: Cc:

Description (last modified by andraganescu)

Part of making the navigation block dynamic was adding a new filter at pre_render_block which enhances the way blocks are passed to the server render callback by passing render_callback the block object as the argument.

We should to update WP Core with these changes and remove them from the Gutenberg compat lib https://github.com/WordPress/gutenberg/blob/95e769df1f82f6b0ef587d81af65dd2f48cd1c38/lib/compat.php#L33

Attachments (1)

48104.diff (1.5 KB) - added by andraganescu 7 months ago.

Download all attachments as: .zip

Change History (18)

#1 @andraganescu
7 months ago

  • Description modified (diff)

#2 @andraganescu
7 months ago

  • Description modified (diff)

@andraganescu
7 months ago

#3 @andraganescu
7 months ago

So the shim in Gutenberg compat.php only sends $block to the render method of WP_Block_Type. 48104.diff adds this to render_block and an argument to the render method of WP_Block_Type.

#4 @andraganescu
7 months ago

  • Keywords has-patch needs-testing needs-dev-note needs-docs added; needs-patch removed

#5 @youknowriad
7 months ago

  • Milestone changed from 5.3 to 5.4

#6 @noisysocks
7 months ago

48104.diff looks good to me. I wonder, though, if we should move away from giving third parties the raw block array and instead create a WP_Block class to encapsulate that data. This would make the API nicer (e.g. $block->attributes instead of $block['attrs']), and would help developers avoid nasty surprises like default block attributes not being set. We could consider updating parse_blocks() to also return WP_Block.

What do you think?

At any rate, since this isn't needed for 5.3, let's punt on this for now and focus on landing it when trunk opens for 5.4.

#7 @aduth
4 months ago

Part of making the navigation block dynamic was adding a new filter at pre_render_block which enhances the way blocks are passed to the server render callback by passing render_callback the block object as the argument.

Can you elaborate on why these changes were needed? At a glance, the $block parameter seems highly redundant with the previous parameters (which are effectively $block['attrs'] and a derivation of $block['innerContent'] respectively). Just seems like we're adding more, without having a good sense of what we're expecting people to be using this function for. Or it's a scenario where, in retrospect, we should have just given the $block object as the sole parameter and left it to the block to deal with.

I wonder if there's also another option for these blocks where, instead of adding this through the block interface render callback, a block can choose to implement their own advanced rendering by applying their own hooks to render_block filter, etc.

#8 @andraganescu
3 months ago

@aduth I tried and tried by can't remember all the details, other than the main issue being that innerBlocks were not passed into the render callback so we could not loop through them to make the navigation nested lists.

I believe you're right at this point:

it's a scenario where, in retrospect, we should have just given the $block object as the sole parameter and left it to the block to deal with.

Indeed, allowing blocks to set their own hooks to render_block filter might be a great idea, but more usecases could let us know of how this will be used.

#9 @jorgefilipecosta
2 months ago

It seems currently on wp-trunk the navigation block does not work on the front end. The block is not rendered(on the plugin it works). I think the problem affecting the block _render parameter issue. The navigation block will land on WordPress 5.4, so we will need to land a fix, even if temporary.

@andraganescu what are your thoughts on the option @aduth suggested:

I wonder if there's also another option for these blocks where, instead of adding this through the block interface render callback, a block can choose to implement their own advanced rendering by applying their own hooks to render_block filter, etc.

It seems this option suggested by @aduth does not involve a public API change. Is that right @aduth? @andraganescu, what are your thoughts on this option?

#10 @aduth
2 months ago

It seems this option suggested by @aduth does not involve a public API change. Is that right @aduth?

Correct, it was my hope that it wouldn't require any public API changes.

#11 @andraganescu
2 months ago

Well, @aduth @jorgefilipecosta the way to find out is to test and see if there is any issue. I will open a PR and see if the Navigation block can instead work instead by hooking into render_block filter as indeed that filter passes the whole $block so the rest of the logic should be unchanged. I will post the PR here as well asap!

#12 @andraganescu
2 months ago

@aduth @jorgefilipecosta here is a PR implementing the hook into the render_block filter. It renders fine without the shim, so that is an option - forgot how PR's link to Trac!

Not sure about the fact that we will call one function for every block render, cost should be minimal since we bail early if the block is not the one we need.

This ticket was mentioned in Slack in #core-editor by andraganescu. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-editor by jorgefilipecosta. View the logs.


2 months ago

#16 @andraganescu
2 months ago

  • Milestone 5.4 deleted
  • Resolution set to invalid
  • Status changed from new to closed

#17 @andraganescu
2 months ago

This ticket is not needed anymore so I closed it. We don't need any API updates on the block rendering callback execution because of the better way for advanced rendering which is to "hook" into the render_block filter and use the whole block which exists there.

Note: See TracTickets for help on using tickets.