Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented Jan 31, 2020

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.

@draganescu draganescu added the [Block] Navigation Affects the Navigation Block label Feb 1, 2020
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty straight-forward 👍

)
);
}
add_action( 'init', 'register_block_core_navigation' );
add_filter( 'render_block', 'render_block_navigation', 10, 2 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@draganescu draganescu Feb 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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.

@draganescu draganescu merged commit 1e35ca3 into master Feb 3, 2020
@draganescu draganescu deleted the try/render-navigation-with-hook branch February 3, 2020 16:03
@epiqueras epiqueras added this to the Gutenberg 7.4 milestone Feb 3, 2020
@marekhrabe
Copy link
Contributor

marekhrabe commented Feb 3, 2020

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
Copy link
Member

aduth commented Feb 3, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants