Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51850 closed defect (bug) (fixed)

WP_Block: Inner blocks not being set

Reported by: noisysocks's profile noisysocks Owned by: noisysocks's profile noisysocks
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

From https://wordpress.slack.com/archives/C02QB2JS7/p1606038913306200:

Gutenberg 9.4 expects $block→inner_blocks to be set. WordPress's version of the WP_Block class is passing $block->parsed_block['innerBlocks']. Blocks such as Navigation return '' when inner_blocks isn't set. ie My FSE navigation menus don't work anymore. It looks like some extra shimming is needed.

Change History (8)

#1 @noisysocks
4 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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


4 years ago

#3 @bobbingwide
4 years ago

I tested this patch in my local development environment. My navigation menus are working again.

#4 @azaozz
4 years ago

This looks like a "logical companion" to WP_Block::__get(). However in the PHP manual there are some pretty strong, valid objections to using these magic methods: https://www.php.net/manual/en/language.oop5.overloading.php#allnotes.

Looks like in WP 5.5 WP_Block::__get() was used only to retrieve the block attributes: https://core.trac.wordpress.org/browser/tags/5.5/src/wp-includes/class-wp-block.php#L153, however in 5.6 it changed to get everything.

I'm probably missing something but why is that public __get() needed? The $parsed_block is passed in the constructor, so the get_attributes(), get_inner_blocks(), get_inner_content(), etc. would work without additional loading, lazy or not. What is the problem with setting the properties in the constructor at the same time as $parsed_block? (Looks like they will have to be set in the new WP_Block::reset().)

Last edited 4 years ago by azaozz (previous) (diff)

#5 follow-up: @noisysocks
4 years ago

See #51612 for some background context here. (Apologies, I should have linked to this ticket.)

We want to call pre_render_block, render_block_data, and render_block_context in WP_Block::render() as it is not intuitive for these "render" filters to be called in the constructor. WP_Block::$inner_blocks, WP_Block::$inner_html, and WP_Block::$inner_content are all derived from the result of calling these filters.

This means we either have to:

1) Eagerly set the properties in the constructor and then re-set them in render(); or
2) Make the properties lazy.

I opted for (2) because (1) involves doing unnecessary work in the constructor and WP_Block::render() is a fairly warm path that is called when viewing a post.

Note that the dynamic properties are documented using @property PHPDoc attributes and so auto completion will still work for these properties in most IDEs.

Last edited 4 years ago by noisysocks (previous) (diff)

#6 in reply to: ↑ 5 @azaozz
4 years ago

Replying to noisysocks:

We want to call pre_render_block, render_block_data, and render_block_context in WP_Block::render() as it is not intuitive for these "render" filters to be called in the constructor. WP_Block::$inner_blocks, WP_Block::$inner_html, and WP_Block::$inner_content are all derived from the result of calling these filters.

Thanks for the explanation :)

That seems to introduce two states of WP_Block, one with data from before the filters are run, and another after the filters (after render()). This seems to bring some back-compat problems: filters were run on the block data before WP_Block was instantiated, so the filtered, "correct" data was in the instance from the beginning. Now the "correct" data is available only after WP_Block::render() has been run.

Then as far as I see the magic __get() and __isset() are needed to support getting pre-filtered (wrong?) data, or rather may get the wrong data depending on whether render() was run or not.

Going to reopen #51612 with the above questions.

Last edited 4 years ago by azaozz (previous) (diff)

#7 @noisysocks
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

This is fixed by reverting the changes made in #51612 in [49694,49695].

hellofromtonya commented on PR #756:


4 years ago
#8

This is fixed by reverting the changes made in #51612 in changesets 49694 and 49695.

Note: See TracTickets for help on using tickets.