Do advanced rendering by hooking into render_block for navigation block #19991

draganescu · 2020-01-31T19:23:09Z

Description

Instead of implementing any api changes, such as passing the whole $block to the render callback, we can hook into the render_block filter and render the block using the entire $block contents, with a callback for said filter.

This tries to avoid the need to change how the render_callback 's work by adding the changes from Trac ticket #48104

How has this been tested?

Created navigation menus and they all seem to render just as good without the shim, using the filter's callback.

aduth

Looks pretty straight-forward 👍

aduth · 2020-02-01T18:20:57Z

packages/block-library/src/navigation/index.php

 		)
 	);
 }
 add_action( 'init', 'register_block_core_navigation' );
+add_filter( 'render_block', 'render_block_navigation', 10, 2 );


I don't know if the convention is codified anywhere, but typically I would expect to see the add_filter to be placed immediately following the callback function (render_block_navigation in this case).

aduth · 2020-02-01T18:21:55Z

packages/block-library/src/navigation/index.php

@@ -234,7 +239,7 @@ function build_navigation_html( $attributes, $block, $colors, $font_sizes, $is_l
 		$html .= '</span>';

 		// Append submenu icon to top-level item.
-		if ( $attributes['showSubmenuIcon'] && $is_level_zero && $has_submenu ) {
+		if ( ! empty( $attributes['showSubmenuIcon'] ) && $is_level_zero && $has_submenu ) {


What relevance does this change have in the original goal of using the filter instead of a new argument?

I am not sure why, but in my local setup while testing, I got a notice of this not being defined. Also I think this check is better than the initial code, if ( $attributes['showSubmenuIcon'] assumes the item exists.

jorgefilipecosta

This worked well on my tests in the plugin 👍
Thank you for trying this option right away!

I tried to check if this solution works on the core, but we have another problem in core. The code to copy the PHP file is not included so core does not have navigation block PHP. I will submitt a trac ticket very soon to fix this problem.
I think with that problem fixed plus this PR navigation block should start working as expected.

jorgefilipecosta · 2020-02-02T22:24:49Z

I submitted a patch to include the navigation block PHP in the core. I tested it with the file being proposed in this PR, and things worked as expected.

marekhrabe · 2020-02-03T20:06:18Z

This has broken how the render function worked. When it is used through the render_callback, attributes of the block are resolved against the list of its defaults. After this change, only non-default attributes are passed to the function and defaults are omitted entirely. showSubmenuIcon attr is true by default and suddenly stopped rendering because the value is now omitted and thus falsy.

It was fixed here by using empty but it makes me wonder if we are not losing too much by switching how this is called and skipping some core functionality like attr defaulting.

aduth · 2020-02-03T20:34:53Z

This has broken how the render function worked. When it is used through the render_callback, attributes of the block are resolved against the list of its defaults. After this change, only non-default attributes are passed to the function and defaults are omitted entirely. showSubmenuIcon attr is true by default and suddenly stopped rendering because the value is now omitted and thus falsy.

It was fixed here by using empty but it makes me wonder if we are not losing too much by switching how this is called and skipping some core functionality like attr defaulting.

It's a good point to raise, but I think it's also worth clarifying that with the revisions in this pull request, it wouldn't strictly be correct to call this function a "render callback" anymore. Perhaps it should have been renamed accordingly. It behaves quite similar, true, but the change reflects the fact that it's intent is to bypass render_callback with its own substitute value by filtering render_block.

However, that being said, the underlying issue is that this render_block override had changed the original behavior from core in a way which I was not aware of, nor have I seen any discussion for upstream revision.

Namely, this bit of logic:

https://github.com/WordPress/wordpress-develop/blob/ec83cd0/src/wp-includes/blocks.php#L485-L489

It had been changed from:

	if ( $is_dynamic ) {
		$global_post   = $post;
		$block_content = $block_type->render( $block['attrs'], $block_content );
		$post          = $global_post;
	}

...to:

gutenberg/lib/compat.php

Lines 69 to 76 in 6de983f

    
           if ( $is_dynamic ) { 
        
           	$global_post = $post; 
        
           	$prepared_attributes = $block_type->prepare_attributes_for_render( $block['attrs'] ); 
        
           	$block_content       = (string) call_user_func( $block_type->render_callback, $prepared_attributes, $block_content, $block ); 
        
           	$post = $global_post; 
        
           }

Was there some discussion of this, or a corresponding Trac ticket? I didn't see it mentioned in Trac#48104, though it may have been a further evolution of this override from how it was originally forked from the core implementation.

do advanced rendering by hooking into render_block for navigation block

7848796

draganescu requested review from ajitbohra, jorgefilipecosta, Soean, talldan and TimothyBJacobs as code owners January 31, 2020 19:23

jorgefilipecosta added this to Needs Review in WordPress 5.4 Must Have Jan 31, 2020

aduth mentioned this pull request Jan 31, 2020

Lib: Limit pre_render_block extension. #19989

Merged

removes superfluous comment, lint fixes

1cfdf42

draganescu added the [Block] Navigation Affects the Navigation Block label Feb 1, 2020

aduth approved these changes Feb 1, 2020

View reviewed changes

jorgefilipecosta approved these changes Feb 1, 2020

View reviewed changes

Merge branch 'master' into try/render-navigation-with-hook

06adaf5

draganescu merged commit 1e35ca3 into master Feb 3, 2020

WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 3, 2020

draganescu deleted the try/render-navigation-with-hook branch February 3, 2020 16:03

epiqueras added this to the Gutenberg 7.4 milestone Feb 3, 2020

ockham mentioned this pull request Feb 25, 2020

Block Library: Implement Post block and Post blocks editing. #19572

Closed

6 tasks

aduth mentioned this pull request Apr 7, 2020

Block API: Add Block Context support #21467

Merged

aduth mentioned this pull request Apr 16, 2020

Block Library: Navigation: Refactor to use render_callback #21660

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do advanced rendering by hooking into render_block for navigation block #19991

Do advanced rendering by hooking into render_block for navigation block #19991

draganescu commented Jan 31, 2020 •

edited

aduth left a comment

aduth Feb 1, 2020

aduth Feb 1, 2020

draganescu Feb 3, 2020 •

edited

jorgefilipecosta left a comment

jorgefilipecosta commented Feb 2, 2020

marekhrabe commented Feb 3, 2020 •

edited

aduth commented Feb 3, 2020 •

edited

Do advanced rendering by hooking into render_block for navigation block #19991

Do advanced rendering by hooking into render_block for navigation block #19991

Conversation

draganescu commented Jan 31, 2020 • edited

Description

How has this been tested?

aduth left a comment

Choose a reason for hiding this comment

aduth Feb 1, 2020

Choose a reason for hiding this comment

aduth Feb 1, 2020

Choose a reason for hiding this comment

draganescu Feb 3, 2020 • edited

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Feb 2, 2020

marekhrabe commented Feb 3, 2020 • edited

aduth commented Feb 3, 2020 • edited

draganescu commented Jan 31, 2020 •

edited

draganescu Feb 3, 2020 •

edited

marekhrabe commented Feb 3, 2020 •

edited

aduth commented Feb 3, 2020 •

edited