WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 2 days ago

#51612 accepted defect (bug)

`render_block_data` for nested blocks - WP_Block::render vs render_block

Reported by: gaambo Owned by: noisysocks
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.5
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The changes introduced in #49926 also mean that hooks from render_block (like pre_render_block, render_block_data and render_block_context) only get called for top-level blocks but not for nested blocks, because nested blocks are now just created in WP_Block::render and not passed through render_block.

This leads to multiple problems:

  1. There's an inconsistency in rendering depending on if a block is used top-level or in another block (eg group). This may lead to bugs.
  2. Also the Latest Posts block uses this hook to parse some data (or migrate it?) - see wp-includes/blocks/latest-posts:208. This method is not called if the Latest Posts block is inside a Group block.
  3. I'm not sure I can give the same information to a block via block-context as with render_block_data before.

My use case:
I'm using the 'render_block_data' method to add some custom information (eg. a helper method which parses the is-style-{style} className from the attributes and sets it as variable on the block data.

I didn't see any documentation about this change, but have to admit I still have to read everything about block contexts.

Steps to reproduce the behavior:

  1. Create a new Post and insert a Group block
  2. Insert a nested Latest Posts Block
  3. block_core_latest_posts_migrate_categories is not called (actually it's called but not for the core/latest-posts block)

This can be reproduced for any nested block and any hook to render_block_data (and the others mentioned above).

Expected behavior
I think all blocks should be passed through the same hooks (pre_render_block, render_block_data) - the same method should be used for nested and top-level blocks.

I opened an issue on the Gutenberg Github Repo (https://github.com/WordPress/gutenberg/issues/25900) but didn't get any response yet. Since this affects the current WP version without the plugin as well, I thought I'd open an issue here.

Change History (40)

#1 @hellofromTonya
5 weeks ago

  • Version changed from trunk to 5.5

#2 @noisysocks
3 weeks ago

#51708 was marked as a duplicate.

#3 @noisysocks
3 weeks ago

  • Milestone changed from Awaiting Review to Future Release

#4 @noisysocks
3 weeks ago

Thanks for the report @gaambo.

We should be applying these filters inside WP_Block::_construct as well.

To test, add these filters and then open a post that has some blocks inside a Group block. Note that the filters aren't called for the inner blocks.

<?php
add_filter(
        'pre_render_block',
        function( $pre_render, $parsed_block ) {
                var_dump( 'pre_render_block', $pre_render, $parsed_block );
        },
        10,
        2
);

add_filter(
        'render_block_data',
        function( $parsed_block, $source_block ) {
                var_dump( 'render_block_data', $parsed_block, $source_block );
        },
        10,
        2
);

add_filter(
        'render_block_context',
        function( $context, $parsed_block ) {
                var_dump( 'render_block_context', $context, $parsed_block );
        },
        10,
        2
);

#5 @noisysocks
3 weeks ago

  • Keywords needs-patch added

#6 @gaambo
3 weeks ago

  • Resolution set to invalid
  • Status changed from new to closed

Thanks for answering and adding 👍
Do you think it should be called in WP_Block::construct or WP_Block::render?
I think render makes a little more sense, that would mean extracting this filters from render_block - which makes this function very thin. Would that be okay? Gotta admit I'm a little overwhelmed with this function and all the context stuff happing, so can't really help with a patch here, but can help with testing.

#7 @gaambo
3 weeks ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

I thought I'd clicked "leave as new", don't know why it's closed now 🤷‍♂️ sorry.

#8 @noisysocks
3 weeks ago

Do you think it should be called in WP_Block::construct or WP_Block::render?

No preference 🙂

I think render makes a little more sense, that would mean extracting this filters from render_block - which makes this function very thin. Would that be okay?

Maybe in an ideal world we'd delete render_block but because WordPress is committed to backwards compatibility we're forced to keep it around forever.

But that's OK. I view render_block as a convenience function that wraps new WP_Block(...)->render().

Gotta admit I'm a little overwhelmed with this function and all the context stuff happing, so can't really help with a patch here, but can help with testing.

No worries at all. Block context is definitely tricky and hard to reason about, though worth noting that there are unit tests for WP_Block which ought to catch any accidental regressions.

This ticket was mentioned in PR #691 on WordPress/wordpress-develop by noisysocks.


3 weeks ago

  • Keywords has-patch added; needs-patch removed

Move the pre_render_block, render_block_data, and render_block_context filters from render_block() to WP_Block. This ensures that they are called for all blocks, including nested blocks, not just top-level blocks.

Trac ticket: https://core.trac.wordpress.org/ticket/51612

#10 @noisysocks
3 weeks ago

  • Keywords needs-testing added
  • Owner set to noisysocks
  • Status changed from reopened to accepted

@gaambo: Does PR #691 fix the problems you were encountering?

#11 @gaambo
3 weeks ago

Thank you, I can confirm that it works in my use case with multiple levels of nested blocks and dynamic as well as static blocks.

#12 @prbot
3 weeks ago

gziolo commented on PR #691:

@noisysocks - nice one, it would be really helpful to add unit tests to prevent future regressions.

#13 @noisysocks
3 weeks ago

  • Keywords has-unit-tests added

#14 follow-up: @gziolo
3 weeks ago

@gaambo – can you retest the updated patch? The changes applied shouldn't have an impact but as you have a real-life usage to test with you are the best person to verify :)

Code-wise everything looks great.

#15 in reply to: ↑ 14 @gaambo
3 weeks ago

The original issue (filters not being called) is now solved, but there are a few things (which you also commented in GitHub):

  1. render_block_data (WP_Block::constructor) now get's called before pre_render_block (WP_Block::render) - which may make sense, but it's still different to before, so I think that should be documented somewhere in the changelog (dev-note?).
  2. pre_render_block get's called multiple times for top-level items. Again: May make sense, but I've run into a couple of problems in the past when filters/actions were being called multiple times for the same object. In my case (see below) I just check if it's the same $block['blockName'] as run before, that's only needed for top level blocks.

Maybe add an extra argument whether it's a top-level/it's in render_block? Or is that to confusing?

To better describe my usecase:
It's on a very customized site - some of the blocks need to be rendered differently if they are nested (eg with/without margin/padding; I'm not using groups/covers/containers etc here so the client only has to insert the content-blocks and styling is all done via css/html).
So until WP 5.4 I hook into pre_render_block on the last priority and if it didn't return null I registered the currentRenderedBlock in an internal array. On render_block (again last priorty) I deregistered the block again. In between a block could check if it's nested if the currentRenderedBlocks is an array with more than one item.

I may have to adapt this to the new order of hooks being called, but that should be possible - or maybe it's even possible via block context to check if it's nested. Gotta figure that out.

#16 @prbot
3 weeks ago

TimothyBJacobs commented on PR #691:

I don't really understand why these filters are in the constructor and not inside render? They are render specific, and putting them in the constructor of the Block class introduces side-effects and limits the utility of WP_Block as a model object.

#17 @prbot
3 weeks ago

noisysocks commented on PR #691:

I don't really understand why these filters are in the constructor and not inside render? They are render specific, and putting them in the constructor of the Block class introduces side-effects and limits the utility of WP_Block as a model object.

By "model object" do you mean that you'd expect $parsed_object === ( new WP_Block( $parsed_object ) )->parsed_object to always be true?

WP_Block::render concatenates and returns whatever is already set in WP_Block::$inner_blocks, WP_Block::$inner_html, and WP_Block::$inner_content. These properties are eagerly set in WP_Block::__construct.

We could lazily instantiate this data in WP_Block::render instead but then that would break backwards compatibility because WP_Block::$inner_blocks, WP_Block::$inner_html, and WP_Block::$inner_content are all public properties.

So, I'd like to move these filters to WP_Block::render, but am unsure how to do so given this predicament. Any ideas?

#18 @prbot
3 weeks ago

TimothyBJacobs commented on PR #691:

By "model object" do you mean that you'd expect $parsed_object === ( new WP_Block( $parsed_object ) )->parsed_object to always be true?

Exactly.

So, I'd like to move these filters to WP_Block::render, but am unsure how to do so given this predicament. Any ideas?

Can we initialize the property in the constructor, but then in render backup the current prop value, apply the filters, and then restore the prop value at the end of render?

#19 @prbot
2 weeks ago

noisysocks commented on PR #691:

Can we initialize the property in the constructor, but then in render backup the current prop value, apply the filters, and then restore the prop value at the end of render?

I dislike that this would mean we're doing unnecessary work in the constructor. render_block is a fairly warm path, so performance is important.

I'll have a play. It might be possible to move all of the work into render() and turn the public properties into lazy getters.

#20 @prbot
2 weeks ago

noisysocks commented on PR #691:

OK, I've moved the two filters to WP_Block::render() in cc5c894.

I did this by making it so that most of the properties that are derived from $parsed_block and $available_context are computed lazily.

We can then, in WP_Block::render(), set $parsed_block and $available_context to the filtered values and reset the cache. The properties will be re-derived from the filtered values.

In real usage, this means we compute the properties at most once per render.

#21 @prbot
2 weeks ago

TimothyBJacobs commented on PR #691:

This looks great to me!

#22 @noisysocks
2 weeks ago

  • Keywords commit added; needs-testing removed
  • Milestone changed from Future Release to 5.7

#23 @gaambo
2 weeks ago

I see, that this is not an issue easy to fix, but I still wanted to ask: Any possibility that this goes into 5.6?
Since the original commits which removed the filters for innerBlocks were part of 5.5 means that I'm stuck von 5.4 on a few clients sites and have to wait for 5.7.

#24 @gziolo
2 weeks ago

I personally think that it's a bug fix that should still land in 5.6. @TimothyBlynJacobs accepted the latest revision to the patch so I don't see any blockers. What do you all think?

#25 @TimothyBlynJacobs
12 days ago

I definitely agree it is a bug fix, but it is frighteningly close to RC.

#26 @noisysocks
12 days ago

  • Milestone changed from 5.7 to 5.6

Agree that it would be nice to fix this 5.5 regression in 5.6.

I'll commit this for 5.6. We can revert this if there are any hiccups found in the RC period.

#27 @noisysocks
12 days ago

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

In 49608:

Editor: Move pre_render_block, render_block_data, render_block_context

Move the pre_render_block, render_block_data, and render_block_context
filters from render_block() to WP_Block. This ensures that they are
called for all blocks, including nested blocks, not just top-level
blocks.

Fixes #51612.
Props gaambo, gziolo, TimothyBlynJacobs.

#28 @SergeyBiryukov
11 days ago

In 49609:

Docs: Drop src/ from pre_render_block duplicate hook reference.

Follow-up to [49608].

See #51612.

#29 @azaozz
3 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

May be overthinking this but [49608] seems to introduce two "states" of WP_Block. When it is instantiated it has unfiltered (wrong?) data, then after render() is run the data is reset to the filtered values. As far as I understand WP_Block was modeled after WP_Post that is (mostly) static in nature.

This seems to bring a lot of complexity/possible confusion when using it, and some back-compat concerns. Before [49608] WP_Block was instantiated with the filtered data that was (more or less) immutable while the instance is used. Now the data is mutated after render() (hence the need for WP_Block::reset(), __get(), etc.).

Not sure if this is a good change going forward. It's true that WP_Block::$parsed_block is public and plugins can "hack it" at any time (that btw doesn't seem like a good idea too), but having an instance with unfiltered data doesn't seem useful/helpful.

A possible solution may be to run the existing filters as soon as WP_Block is instantiated (in the constructor, as mentioned above) and perhaps introduce new filter(s) in render() that only affect the output HTML, if needed.

Last edited 3 days ago by azaozz (previous) (diff)

#30 @SergeyBiryukov
3 days ago

  • Keywords commit removed

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


3 days ago

#32 follow-up: @TimothyBlynJacobs
3 days ago

I think this should probably be reverted and tried again in 5.7-early. This has more ramifications than was previously thought, that should be resolved in Beta not RC.

When it is instantiated it has unfiltered (wrong?) data

IMO this isn't incorrect data. I think it would be incorrect if the render filters were used when the block wasn't in a render context. I think this is analogous to WP_Post::filter and WP_Term::filter. Maybe we should make the WP_Block follow that filter API pattern instead to be more clear.

#33 in reply to: ↑ 32 @azaozz
3 days ago

Replying to TimothyBlynJacobs:

I think this should probably be reverted and tried again in 5.7-early. This has more ramifications than was previously thought, that should be resolved in Beta not RC.

Yeah, starting to look that way.

When it is instantiated it has unfiltered (wrong?) data

IMO this isn't incorrect data. I think it would be incorrect if the render filters were used when the block wasn't in a render context.

Yeah, the render filters would ideally run while the block is being rendered (judging by their names). However before this change they were running before WP_Block was even instantiated. Running them "late" seems to bring unneeded complexity and problems.

One possible solution could be to deprecate these filters and leave them in place, then introduce new "render" filters that run in render() and do not mutate the block properties. On the other hand deprecating something and leaving it in place means plugins that already use these filters would likely never change...

Another (preferable?) solution would be to remove (and shim) WP_Block::render(), i.e. do the rendering as soon as WP_Block is instantiated. Seems this will bring WP_Block more inline with WP_Post, WP_Term, etc.

I think this is analogous to WP_Post::filter and WP_Term::filter. Maybe we should make the WP_Block follow that filter API pattern instead to be more clear.

WP_Post::filter seems to hold the level of sanitization of the post, doesn't seem to affect specifically the hooks/filters.

#34 follow-up: @TimothyBlynJacobs
3 days ago

However before this change they were running before WP_Block was even instantiated. Running them "late" seems to bring unneeded complexity and problems.

But only if render_block is the only consumer of WP_Block, which I don't think we want to constrain in that way.

Another (preferable?) solution would be to remove (and shim) WP_Block::render(), i.e. do the rendering as soon as WP_Block is instantiated. Seems this will bring WP_Block more inline with WP_Post, WP_Term, etc.

That would bring a performance penalty if someone is trying to use WP_Block as a data structure, and they don't need the rendered content. Developers do weird things during rendering sometimes that can be quite slow. It's why we needed to allow for pulling just the raw content via the REST API for instance.

WP_Post::filter seems to hold the level of sanitization of the post, doesn't seem to affect specifically the hooks/filters.

Right but the sanitization is a series of filters on the individual post fields. Including a display filter.

#35 in reply to: ↑ 34 @azaozz
3 days ago

Replying to TimothyBlynJacobs:

But only if render_block is the only consumer of WP_Block, which I don't think we want to constrain in that way.
...
if someone is trying to use WP_Block as a data structure, and they don't need the rendered content...

Right. In this case guessing plugins and themes will have to learn to live with the fact that WP_Block is different than WP_Post and is "mutable". Still thinking this should be described in more details, perhaps in the top docblock.

However thinking that resetting the properties after rendering (i.e. the class properties are different before running render(), while running render(), and after running render()) makes things more complex, and hard to understand and work with. Same for the magic __get().

Probably missing something but what purpose/use case does this solve? Seems these properties can be instantiated with null or the corresponding values from $parsed_block, then set/changed during render(). Why do they need to be unset/reset after that? render_block() calls $block->render() immediately after instantiating WP_Block, so these will be (should be) correct there.

Right but the sanitization is a series of filters on the individual post fields. Including a display filter.

Right, the difference is not to misunderstand that "filter" in WP_Post::filter means filtering/sanitization of the post object values, nothing to do (directly) with hooks.

Last edited 3 days ago by azaozz (previous) (diff)

#36 @noisysocks
3 days ago

In 49694:

Editor: Remove render_block hooks from WP_Block

Reverts the move of pre_render_block, render_block_data, and
render_block_context to WP_Block.

This change has more implications than first thought so will be revisted later
in 5.7.

Reverts [49609,49608].
See #51612.

#37 @noisysocks
3 days ago

In 49695:

Editor: Remove render_block hooks from WP_Block

Reverts the move of pre_render_block, render_block_data, and
render_block_context to WP_Block.

This change has more implications than first thought so will be revisted later
in 5.7.

Reverts [49609,49608].
See #51612.

#38 @noisysocks
3 days ago

  • Milestone changed from 5.6 to 5.7
  • Status changed from reopened to accepted

I reverted the changes from trunk and 5.6 🙂

This ticket was mentioned in PR #765 on WordPress/wordpress-develop by noisysocks.


2 days ago

Move the pre_render_block, render_block_data, and render_block_context filters from render_block() to WP_Block. This ensures that they are called for all blocks, including nested blocks, not just top-level blocks.

So that there is no performance penalty incurred from calculating block properties twice, WP_Block can now be made to not set any derived block properties until render() is called.

Trac ticket: https://core.trac.wordpress.org/ticket/51612

#40 @prbot
2 days ago

noisysocks commented on PR #765:

@azaozz @TimothyBJacobs: How about something like this? Properties can still be lazily initialised, but it requires explicitly passing an option to new WP_Block(). This way we don't need __get().

Note: See TracTickets for help on using tickets.