Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#50146 new defect (bug)

Blocks: `serialize_block()` doesn't support `WP_Block_Parser_Block`

Reported by: dlh's profile dlh Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.3.1
Component: Editor Keywords: has-patch
Focuses: Cc:

Description

serialize_block() is documented as accepting a WP_Block_Parser_Block, but it treats the passed value as an array. Because WP_Block_Parser_Block doesn't implement ArrayAccess, passing one to serialize_block() generates a fatal error.

The attached patch would allow instances of WP_Block_Parser_Block to be used with serialize_block() by casting them to arrays. It would make the same change to filter_block_kses(), which exhibits the same mismatch in behavior.

If adding support for WP_Block_Parser_Block to these functions is seen as too risky from a perspective of maintaining compatibility with future iterations on block parsing, then simply changing the DocBlocks on these two functions might instead be the way to go.

Attachments (2)

50146.diff (1.9 KB) - added by dlh 5 years ago.
50146.2.diff (3.9 KB) - added by dlh 5 years ago.

Download all attachments as: .zip

Change History (6)

@dlh
5 years ago

#1 @aduth
5 years ago

If I recall correctly, the implementation was based on the fact that WP_Block_Parser::parse claims to return type WP_Block_Parser_Block[], when in fact it returns an array of arrays.

Which isn't to say this change request isn't justified, and likely correct, but the full extent of the problem permeates beyond just this one function.

Aside: Possibly related: #49926

Note: `render_block` and serialize_block currently receive $block in the parsed array form. While I remarked differently on the original pull request, I don't know that these actually need to change, since it seems rather sensible that these would except to receive blocks in their parsed forms. If one has access to a WP_Block instance and needs to create its rendered representation, the instance's render function can be used directly.

#2 @dlh
5 years ago

@aduth Thanks for that context. It sounds like the docs within class-wp-block-parser.php might need a fresh look with respect to WP_Block_Parser_Block. I'll be happy to do that while the original change here collects any additional feedback, and I'll include any other changes from that review in a second patch.

#3 @dlh
5 years ago

50146.2.diff updates the documentation references to WP_Block_Parser_Block in class-wp-block-parser.php where needed, as far as I could determine.

This patch also omits the cast to (array) in serialize_block() from the first patch, which simplifies the change to one of just documentation. While supporting WP_Block_Parser_Block in these functions would still be useful, the discussion in and around #50009 makes me wonder whether it's premature to bind these functions to a specific implementation.

@dlh
5 years ago

#4 @janw.oostendorp
4 years ago

All points raised by @dlh are still relevant.His patch also would still fix it.
His suggestions only change docblocks so I can't break anything.

Last edited 4 years ago by janw.oostendorp (previous) (diff)
Note: See TracTickets for help on using tickets.