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

Fix fatal error when calling undefined block library function. #56459

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

tellthemachines
Copy link
Contributor

What?

Fixes #56453.

The newly-added WP_Navigation_Block_Renderer was calling some block-library functions by their unprefixed names, which will cause a fatal error if those functions happen to not be defined in core. All block library PHP functions are prefixed with gutenberg_ at build time, so anywhere they are called outside of block-library (within the plugin) the prefixed version of their name should be used.

Testing Instructions

This is a tricky one to test in 6.4.1 because it can only be reproduced on a site that has a Navigation block created from a classic menu some time ago, so that it both has the __unstableLocation attribute and doesn't have the ref one. Essentially we need to make sure that this line runs.

An easier way to test is by downgrading to 6.3.2 and making sure there is a Nav with a submenu set to open on click on the page, so that this line runs.

With this PR, there should be no Navigation-related errors in WP 6.4.1 or 6.3.2.

@tellthemachines tellthemachines added [Type] Regression Related to a regression in the latest release [Block] Navigation Affects the Navigation Block Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release labels Nov 23, 2023
@tellthemachines tellthemachines self-assigned this Nov 23, 2023
Copy link

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

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, and for the helpful testing instructions!

I could reproduce the issue in downgraded 6.3.2 using the latest Gutenberg, with a Navigation block set have sub menus click to open. That resulted in the following error:

image

With this PR applied, no errors in 6.3.2 or 6.4.1.

LGTM! ✨

@tellthemachines tellthemachines merged commit 84de158 into trunk Nov 23, 2023
58 checks passed
@tellthemachines tellthemachines deleted the fix/fatal-nav-error branch November 23, 2023 02:19
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 23, 2023
@tellthemachines tellthemachines removed the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label Nov 23, 2023
@getdave
Copy link
Contributor

getdave commented Nov 23, 2023

Thank you all for fixing this so quickly ❤️

@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

@scruffian this will require a backport as part of #57979 🙏

$compacted_blocks = block_core_navigation_filter_out_empty_blocks( $parsed_blocks );
$compacted_blocks = gutenberg_block_core_navigation_filter_out_empty_blocks( $parsed_blocks );
Copy link
Contributor

Choose a reason for hiding this comment

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

@scruffian I'm seeing that these references have reverted to block_ prefix in Gutenberg trunk. Can you verify if that was intentional so that we can avoid any regressions here 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that this is now ok because functions being called are now within block-library and indeed within the same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's my understanding too, so I think this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's ok! What we shouldn't have is gutenberg_ prefixed functions in block-library.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I think that's what must have drawn my attention to this change.

@scruffian why are we now using gutenberg_ prefix? Should this be revised as @tellthemachines indicates?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have the gutenberg_ prefix in the navigation block except where its added by the webpack script.

@getdave getdave removed the Needs PHP backport Needs PHP backport to Core label Jan 25, 2024
@getdave
Copy link
Contributor

getdave commented Jan 25, 2024

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

Also note this discussion.

cc @scruffian for confirmation.

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 [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call to undefined function block_core_navigation_get_menu_items_at_location
4 participants