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

Navigation: Remove the IS_GUTENBERG_PLUGIN check around block_core_navigation_parse_blocks_from_menu_items #47824

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

scruffian
Copy link
Contributor

What?

As raised in #47822, block_core_navigation_parse_blocks_from_menu_items is now called in code paths outside of the Gutenberg plugin, so we should remove this wrapper.

Why?

This is needed for the case when a classic menu is converted into a wp_navigation menu. Since we want this to happen in core now, we need to make this function available.

How?

Just move the code outside the conditional:
if ( defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN ) {

Testing Instructions

I'm not sure. To hit the code paths you need to:

  1. Switch to a classic theme
  2. Create a classic menu
  3. Switch to a block theme
  4. Delete all your wp_navigation menus
  5. Load the editor/the front end and check that a new wp_navigation menu is created
    However I'm not sure how to test that the fix is working in trunk.

@scruffian scruffian added the [Block] Navigation Affects the Navigation Block label Feb 7, 2023
@scruffian scruffian self-assigned this Feb 7, 2023
@ntsekouras ntsekouras added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 7, 2023
@ntsekouras
Copy link
Contributor

ntsekouras commented Feb 7, 2023

Thank you Ben!

However I'm not sure how to test that the fix is working in trunk.

This file is copied to core, so if it works here it will be working there. Besides that I tested in core with your patch and no fatal is there..

function block_core_navigation_parse_blocks_from_menu_items( $menu_items, $menu_items_by_parent_id ) {
if ( empty( $menu_items ) ) {
return array();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see there are still protected functions above. Are these still Gutenberg-only functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

No from what I see.

E.g. block_core_navigation_sort_menu_items_by_parent_id is called in render_block_core_navigation without garding defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN

Copy link
Contributor

Choose a reason for hiding this comment

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

So potentially there are remaining code paths that can lead to errors right? Or are these impossible to trigger on Core @scruffian

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think the others are guarded.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Tested with WP core trunk by applying the same changes and confirmed this fixes the error.

@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Feb 7, 2023
@ntsekouras ntsekouras merged commit 7a6dcab into trunk Feb 7, 2023
@ntsekouras ntsekouras deleted the fix/47822 branch February 7, 2023 11:55
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 7, 2023
ntsekouras pushed a commit that referenced this pull request Feb 7, 2023
…vigation_parse_blocks_from_menu_items (#47824)

Merging as these changes cannot cause any failure and the mobile test are failing in trunk from this PR: #47229, which is also not included in 6.2 yet.
@ntsekouras
Copy link
Contributor

Cherry-picked this PR to the wp/6.2 branch.

@ntsekouras ntsekouras removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 7, 2023
@DaisyOlsen DaisyOlsen added the [Package] Block library /packages/block-library label Feb 14, 2023
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 [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants