Make WordPress Core

Opened 4 weeks ago

Last modified 2 weeks ago

#64505 reviewing defect (bug)

REST API: Block renderer passes incomplete block shape

Reported by: dlh's profile dlh Owned by: dmsnell's profile dmsnell
Milestone: 7.0 Priority: normal
Severity: normal Version: 5.4
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

Since [47360], the WP_REST_Block_Renderer_Controller::get_item() method creates "an array representation simulating the output of parse_blocks" and passes it to render_block(), but the array is missing the innerBlocks key.

This missing key causes issues with downstream code that either treats a block without innerBlocks as invalid input or, more commonly, assumes that innerBlocks is present and accesses it unconditionally (example).

Instead of manually constructing an array, one can be created from a WP_Block_Parser_Block, as proposed in the linked PR, which will provide higher fidelity to the shape returned by parse_blocks().

Change History (12)

This ticket was mentioned in PR #10735 on WordPress/wordpress-develop by @dlh.


4 weeks ago
#1

  • Keywords has-patch added

@dlh commented on PR #10735:


4 weeks ago
#3

@mukeshpanchal27 Looks like that's an effect of 22294af4ed3b46f577f42590ba4cc19ca0a93f3f for https://core.trac.wordpress.org/ticket/64393.

#4 @westonruter
3 weeks ago

  • Milestone changed from Awaiting Review to 7.0
  • Owner set to westonruter
  • Status changed from new to reviewing

@dlh commented on PR #10735:


3 weeks ago
#5

@westonruter As I was doing some other work, I happened to notice that the insert_hooked_blocks() function also contains a malformed block array: it's missing `innerHTML`. Do you think that'd be worth fixing as part of this ticket or in a new one?

@mukesh27 commented on PR #10735:


3 weeks ago
#6

I happened to notice that the insert_hooked_blocks() function also contains a malformed block array: it's missing `innerHTML`. Do you think that'd be worth fixing as part of this ticket or in a new one?

Why it didn't show any error then?

@dlh commented on PR #10735:


3 weeks ago
#7

I happened to notice that the insert_hooked_blocks() function also contains a malformed block array: it's missing `innerHTML`. Do you think that'd be worth fixing as part of this ticket or in a new one?

Why it didn't show any error then?

It might cause errors, I don't know. I'm not using hooked blocks currently in my own code. This is just an observation from reading the function.

@westonruter commented on PR #10735:


3 weeks ago
#8

As I was doing some other work, I happened to notice that the insert_hooked_blocks() function also contains a malformed block array: it's missing `innerHTML`. Do you think that'd be worth fixing as part of this ticket or in a new one?

Probably best to get @dmsnell's thoughts on this.

#9 follow-up: @dmsnell
3 weeks ago

Alas, I strongly urged us not to separate the `WP_Block_Parser_Block` class out of the parser because I knew it would lead to fragility and that being in a separate class would communicate the wrong thing. we should probably mark that @private.

so I would suggest instead of using it that we either just add the missing property or propose a utility function whose purpose is specifically to create a block node. this could be something as basic as wp_create_block( $blockType, $attrs, ...$innerContent ) and could be used directly.

it makes me nervous to learn that we’ve added this class as the type of the block node and I think it would help us all if we separate a parser internal from the specified block node representation. one could argue that this is a philosophical point rather than a pragmatic one, but it will feel that way until the day we realize we accidentally coupled internal parser intrinsics with a funny name into one of our core and fundamental building blocks and we can’t escape and have to live with the awkwardness forever, and can never improve our parser unless we create WP_Block_Parser_Real_Parser_Block or WP_Block_Parser_New_Block, or WP_Block_Parser_New_New_Block as these things go.

an array representation simulating the output of parse_blocks

I’d like to clear up the use of simulation here. block’s don’t really exist in the HTML, and the array form in PHP is the quintessential block type. we’ve explored creating an actual class representing this, and I think that would be great. the type would be best described as a WP_Block_Node to match the language in Gutenberg. a “block” is truly only complete in memory when linked to the block’s implementation; in PHP that implementation is WP_Block.

but the array is the real thing. the HTML is the serialized form of it and therefore more of a simulation than what we are discussing in this issue.


Thank you for bringing up this issue, it has highlighted to me that we should go back and make these parser classes more scary, private, and possibly even @ignore to strip them out of the documentation. They should probably also be final, and have scarier names, but they were never meant to see the light of day so the name was perhaps a little more relaxed than it should have been.

You all are wonderful; and really it’s great to embrace standard types with built-in documentation. In this particular case though I think it is a ticking time bomb to use this particular class. I can propose a WP_Block_Node class before the week’s end if you would like that.

Last edited 3 weeks ago by westonruter (previous) (diff)

#10 @westonruter
3 weeks ago

  • Owner westonruter deleted

#11 @westonruter
2 weeks ago

  • Owner set to dmsnell

#12 in reply to: ↑ 9 @dlh
2 weeks ago

Thanks for the feedback and context, @dmsnell.

so I would suggest instead of using it that we either just add the missing property or propose a utility function whose purpose is specifically to create a block node. this could be something as basic as wp_create_block( $blockType, $attrs, ...$innerContent ) and could be used directly.

While I would welcome such a utility, in my opinion it deserves its own ticket and discussion. For now, I've updated the PR to just add the missing properties (in both locations where I observed them).

Note: See TracTickets for help on using tickets.