Navigation Block: Fix warning error when no ref

[t-hamano]

Related: #47453

What?

This PR fixes a problem that caused a warning error when rendering navigation blocks.

Why?

My understanding is that if any changes are made to the navigation block, the content is stored in the database and ref attribute is given. It then retrieves the post from the database based on this ref, stores it in the $navigation_post variable, and checks whether the navigation block is nested based on its content.

However, if the user has not made any changes to this block, the ref does not exist. As a result, $navigation_post is undefined in the current implementation and an error occurs.

How?

I have moved the process of checking for nesting into an if statement that is only executed if the block has a ref.

Used $inner_blocks instead of $navigation_post in the block_core_navigation_block_contains_core_navigation argument to prevent undefined errors. The block_core_navigation_get_menu_items_at_location function has also been adjusted to reflect the change in argument types.

Testing Instructions

• Activate Twenty Twenty Three theme.
• Clear all customizations made on Template and Template Part.
• Go to http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation and remove all navigation.
• Access the top page of the site.
• Confirm that no PHP errors occur.

Regression Test

Please confirm that block_core_navigation_get_menu_items_at_location is working properly as before by following the steps below.

• Remove all your wp_navigation posts and Classic Menus from your site
• Create a brand new Navigation using the Navigation block. Be sure to save it.
• Now open your MySQL client and update the post_content of your wp_navigation post with the following content, being sure to replace %%REPLACE_ME%% with the ID of the wp_navigation post YOU just created:

<!-- wp:navigation {"ref":%%REPLACE_ME%%,"__unstableLocation":"primary","className":"social-links","layout":{"type":"flex","setCascadingProperties":true,"justifyContent":"right","orientation":"horizontal"},"style":{"spacing":{"margin":{"top":"0"}}}} /-->

• Reload the same Post.
• See that it loads ok and the Navigation block is not rendered (because there are no blocks).

[talldan]

Could the check be kept here, but changed to something like this?

if ( block_core_navigation_block_contains_core_navigation( $inner_blocks ) ) {

I think that would cover more cases (i.e. where the inner blocks are serialized instead of pulled from the wp_navigation post).

$inner_blocks is a WP_Block_List, but it implements Iterator, so should still support loops. The values will be WP_Block objects, so it'd have to internally check for $block->name instead of $block['blockName'].

I think it'd be good to also add extra safety to block_core_navigation_block_contains_core_navigation, by having it return false when the parsed_blocks isn't set.

[t-hamano]

$inner_blocks is a WP_Block_List, but it implements Iterator, so should still support loops. The values will be WP_Block objects, so it'd have to internally check for $block->name instead of $block['blockName'].

Makes Sense! I have responded with dec0f03, but I would like to fix the unit test now.

I think it'd be good to also add extra safety to block_core_navigation_block_contains_core_navigation, by having it return false when the parsed_blocks isn't set.

Do you mean to change it as follows?

if ( ! isset( $parsed_blocks ) || block_core_navigation_block_contains_core_navigation( $inner_blocks ) ) {
	return '';
}

[draganescu]

Could this be solved simpler by moving this call up to be inside the ref exists condition ?

[t-hamano]

Could this be solved simpler by moving this call up to be inside the ref exists condition ?

I tried that approach with my first commit. But, as a series of comments, it would be better not to limit it to only blocks with ref.

[talldan]

Do you mean to change it as follows?

@t-hamano Something like this is what I had in mind:

function block_core_navigation_block_contains_core_navigation( $inner_blocks ) {
        if ( ! isset( $inner_blocks ) ) {
                return false;
        }

	foreach ( $inner_blocks as $block ) {
		if ( 'core/navigation' === $block->name ) {
			return true;
		}
		if ( block_core_navigation_block_contains_core_navigation( $block->inner_blocks ) ) {
			return true;
		}
	}
	return false;
}

Otherwise the foreach causes an error if $inner_blocks is null.

[t-hamano]

I have added an extra safety in 33780cf. This implementation is based on the post template.

I changed it to check when this function is called recursively in a foreach instead of at the beginning of the function. My understanding is that this problem can occur when the block_core_navigation_block_contains_core_navigation() is called recursively.

[github-actions]

Flaky tests detected in 33780cf.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4044553771
📝 Reported issues:

• [Flaky Test] shouldn't conflict with server-side autosave #35176 in specs/editor/various/autosave.test.js

[scruffian]

Thanks for taking care of this for me. LGTM

[t-hamano]

@scruffian

Thanks for the review!

@talldan

I think it'd be good to also add extra safety to block_core_navigation_block_contains_core_navigation, by having it return false when the parsed_blocks isn't set.

I have not yet been able to address this issue due to a lack of my understanding. If you have anything to add to this PR, I would be glad to hear your advice.

[talldan]

I have not yet been able to address this issue due to a lack of my understanding. If you have anything to add to this PR, I would be glad to hear your advice.

I've left a comment in the thread above that should clear that up 👍