Fix error when using a navigation block that returns an empty fallback result

[talldan]

What?

Fixes #56600

That issue explains how an error can trigger when navigation fallbacks return empty results.

How?

After the refactor in #55605, some of the code was moved from the navigation block's render function (where the code returned an empty string to indicate empty block content) over to smaller helper functions that return fallback blocks.

In these new smaller helper functions, the code should return an array when there are no results, as the new calling code expects to handle arrays/iterables. Probably as a result of the copy/paste, some of them were still returning strings.

Testing Instructions

Testing is a little challenging. I think you need to:

• Delete all wp_navigation menus (so that you don't fallback to using the first one)
• Not have a page list block registered (so that the page list fallback isn't used)
• Delete all classic menus (so that you don't fall back to those either)
• Have a navigation block that has an __unstableLocation attribute specified, and no ref attribute.

All you need to then is view the front end of the site that renders that navigation block and you should be able to trigger the error in trunk.

Screenshots or screencast

Before

After

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php
❔ phpunit/class-wp-navigation-block-renderer-test.php

[github-actions]

Flaky tests detected in c23ede6.
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/7162763338
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js

[getdave]

I tried my best to test this by following the instructions. Here's what I did (you can correct me if I am wrong or you have a better way):

• De-registered the core/page-list block by putting the following in a Plugin file

function getdave_unregister_core_page_list_block() {
	unregister_block_type( 'core/page-list' );
}
add_action( 'wp_loaded', 'getdave_unregister_core_page_list_block' );

• Cleared all Block-based and Classic Navigation Menus.
  • You can check for Nav Posts using npm run wp-env run cli wp post list -- --post_type=wp_navigation --format=ids and then delete with npm run wp-env run cli wp post delete {ID} -- --force.
• New Post -> Switch to Code view
• Open dev tools console. De-register the page list within the editor:

wp.blocks.unregisterBlockType( 'core/page-list' );

• Insert following block into the code editor view in the editor:

<!-- wp:navigation {"__unstableLocation":"foo"} /-->

• Save the post

• View Post on front of site

• On trunk I then observed the error.

• On this branch I no longer observed the error.

My only comment would be should we augment the PHPUnit test suite to provide coverage for this?

[talldan]

Thanks for the review, @getdave! I realise it was a tricky one to test.

My only comment would be should we augment the PHPUnit test suite to provide coverage for this?

I'm personally not sure about adding unit tests after the fact, I think they're most valuable for asserting code that's being written at the time does what's expected and is nicely structured.

That said, it was easy enough to add one for the failing branch of code in get_inner_blocks_from_navigation_post, so I did that.

For get_inner_blocks_from_fallback, I think block_core_navigation_get_fallback_blocks would need to be mocked and I personally don't know how. I had a search online to see if it's possible and couldn't see any. Most results said to refactor the code and make it a method, so that could be an option for the future.

[getdave]

Thanks for the review, @getdave! I realise it was a tricky one to test.

You are welcome. It was a fun exercise 😅

I'm personally not sure about adding unit tests after the fact, I think they're most valuable for asserting code that's being written at the time does what's expected and is nicely structured.
That said, it was easy enough to add one for the failing branch of code in get_inner_blocks_from_navigation_post, so I did that.

Thank you for taking the time to add that. As you know, in the past we've suffered a number of regressions on the Nav block and so we [regular contributors] are now focused on providing test coverage where we can - especially when the overhead of running the tests is low (like the one you kindly wrote here). Appreciate it.

For get_inner_blocks_from_fallback, I think block_core_navigation_get_fallback_blocks would need to be mocked and I personally don't know how. I had a search online to see if it's possible and couldn't see any. Most results said to refactor the code and make it a method, so that could be an option for the future.

I think you could add_filter on block_core_navigation_render_fallback?

[getdave]

Confirmed present in new block-library version of this file

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

Lines 218 to 221 in e0005a8

	// Fallback my have been filtered so do basic test for validity.
	if ( empty( $fallback_blocks ) || ! is_array( $fallback_blocks ) ) {
	return new WP_Block_List( array(), $attributes );
	}

[getdave]

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR no longer requires a backport for WP 6.5.

Why? Because the code was moved into the block-library package in #57979 and thus should be automatically handled by the packages sync process.

[talldan]

Thanks ❤️