Editor: Move pre_render_block, render_block_data, render_block_context (Attempt #2) by noisysocks · Pull Request #765 · WordPress/wordpress-develop

noisysocks · 2020-11-25T03:37:55Z

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

noisysocks · 2020-11-25T03:39:18Z

@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().

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.

noisysocks · 2020-11-25T03:51:46Z

src/wp-includes/blocks.php

+		array(
+			'filter_data' => true,
+		)
+	);


Potentially these could be combined into one setting?

$block = new WP_Block( $parsed_block, $context, WP_Block_Type_Registry::get_instance(), array( 'filter_on_render' => true, ) ); assert( ! isset( $block->name ) ); // Because `filter_on_render` was provided $rendered_block = $block->render(); assert( isset( $block->name ) ); // Because `render()` was called

talldan · 2020-12-02T10:23:40Z

src/wp-includes/class-wp-block.php

 		global $post;
+
+		/** This filter is documented in wp-includes/blocks.php */
+		$pre_render = apply_filters( 'pre_render_block', null, $this->parsed_block );


I notice this is still defined in render_block and not removed like render_block_data and render_block_context.

My intention was that calling it twice would let one short-circuit render_block and WP_Block::render. Now that I think about it, though, maybe that will cause unexpected results for folks using pre_render_block for things other than return false;.

peterwilsoncc

A few minor notes but still getting my head around it.

peterwilsoncc · 2020-12-02T23:34:52Z

src/wp-includes/class-wp-block.php

 	 * @var array
 	 */
-	public $inner_content = array();
+	public $inner_content;


I am wondering if changing the default type could lead to back-compat problems. null is still falsey but if a plugin is doing strict type checking then it might be problematic.

Once set, can any of these values be an empty() value?

Maintaining the __get() could solve this problem.

There shouldn't be any compatibility problems here because all of the attributes are initialised in WP_Block::__construct() by default. The only change in behaviour is if 'is_eager' => false is provided.

peterwilsoncc · 2020-12-02T23:36:02Z

src/wp-includes/class-wp-block.php

+	public function __construct( $parsed_block, $available_context = array(), $registry = null, $options = null ) {
+		$options = wp_parse_args(
+			$options,
+			array(
+				'is_eager' => true,
+			)
+		);
+
+		$this->parsed_block      = $parsed_block;
+		$this->available_context = $available_context;
+		$this->registry          = $registry ? $registry : WP_Block_Type_Registry::get_instance();
+		$this->options           = $options;
+
+		if ( $this->options['is_eager'] ) {
+			$this->set_derived_properties();
+		}
+	}


Nit: $args instead of $options for consistency with the majority of the code base.

Hm, I was trying to be consistent with WP_Block::render() which already has $options. Reckon I should change both? Or just the constructor's? Or neither?

peterwilsoncc · 2020-12-02T23:40:12Z

tests/phpunit/tests/blocks/block.php

+			$rendered_content .= $block->render( array( 'filter_data' => true ) );
+		}
+
+		remove_filter( 'render_block_context', array( $this, 'filter_render_block_context' ) );


Nit: No need to remove filters/actions in the test, they're reset after each test as part of the default tearDown().

Oh cool! Is it better to be explicit though?

peterwilsoncc · 2020-12-02T23:42:28Z

tests/phpunit/tests/blocks/block.php

+		$parsed_blocks = parse_blocks( '<!-- wp:outer --><!-- wp:skip /--> How are you?<!-- /wp:outer -->' );
+		$parsed_block  = $parsed_blocks[0];
+		$context       = array();
+		$block         = new WP_Block( $parsed_block, $context, $this->registry );


Additional tests to ensure lazy mode works would be dandy.

peterwilsoncc · 2020-12-07T07:07:54Z

Neither works for me. On reread it goes in to an options property so best as is.

azaozz · 2020-12-07T13:18:34Z

Thinking now is the best time to look a bit at the "bigger picture" :)

As far as I see WP_Block is --not-- a "standard OOP" class intended to be sub-classed, extended, instantiated in different ways, etc. It is, in a way, closer to a theme's template part than to a low-level data structure, same as WP_Post, WP_Comment, etc.

What is the purpose of WP_Block? Looking at https://core.trac.wordpress.org/ticket/49926 it is to "to serve the purpose of providing a first-class interface for plugins to interact with an instance of a block from within their own render callbacks." So plugins are expected to replace core's render_block()? Is that the way it works now? Is that the way plugins use it? Are there other (non-intended/non-supported) ways plugins may use it?
Should WP_Block be final like WP_Post, WP_Comment, etc.? If not, what would be the expected/supported use in plugins? Should it be a "wrapper" for the parsed and filtered block data (as used in core), or should there be other expected uses?
What happens with the low-level pre_render_block, render_block_data, etc. filters when five plugins instantiate and/or extend WP_Block in five different ways? What about other plugins that depend on these filters? Should plugins be able to overload $block->render()?
As @TimothyBJacobs mentioned on the trac ticket, WP_Block may be possible to use as data structure in plugins, without needing to call render(). Are there plugins using it that way? What are the advantages to use multiple instances of WP_Block (with different data depending on the level of "rendering") to using the parsed blocks data directly?
Perhaps revisit the need of having WP_Block::render() and maybe deprecate it. I know, that mimics similar structures in React in js, but it doesn't work the same way in PHP. (Actually even in React "plugins" are not supposed to cal render() on different instances manually, afaik. That would break the app).

Uh, sorry this is getting too long. As far as I see there's no clarity about howWP_Block should, and should not be used by plugins, and how plugins will interact with one another when using it.

TimothyBJacobs · 2020-12-14T16:32:58Z

I don't really have much to add that I haven't mentioned on the ticket yet. I think this is a fine approach, though I personally would prefer it followed a pattern closer to our existing patterns using $filter like I mentioned on the ticket because I think it is more explicit about what type of mode the object is operating in.

noisysocks force-pushed the fix/51612-attempt-2 branch from 045f824 to de7aec3 Compare November 25, 2020 03:47

noisysocks commented Nov 25, 2020

View reviewed changes

talldan reviewed Dec 2, 2020

View reviewed changes

peterwilsoncc reviewed Dec 2, 2020

View reviewed changes

noisysocks closed this May 28, 2021

Conversation

noisysocks commented Nov 25, 2020

Uh oh!

noisysocks commented Nov 25, 2020

Uh oh!

noisysocks Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwilsoncc commented Dec 7, 2020 via email

Uh oh!

azaozz commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyBJacobs commented Dec 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noisysocks Nov 25, 2020 •

edited

Loading

azaozz commented Dec 7, 2020 •

edited

Loading