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.

Sign up for GitHub

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

Jump to bottom

wp_nav_menu() throws undefined offset if a menu item's parent has a higher ID. #3635

Closed
wants to merge 2 commits into from
Closed

wp_nav_menu() throws undefined offset if a menu item's parent has a higher ID. #3635

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6467467
Test for #57122.
peterwilsoncc Nov 16, 2022
ae45998
Improve description.
peterwilsoncc Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 48 additions & 0 deletions tests/phpunit/tests/menu/wp-nav-menu.php
View file
Open in desktop
Expand Up @@ -197,4 +197,52 @@ public function test_wp_nav_menu_should_not_have_has_children_class_with_custom_
'Level 3 should not be present in the HTML output.'
);
}

/**
* The order in which parent/child menu items are created should not matter.
*
* @ticket 57122
*/
public function test_parent_with_higher_id_should_not_error() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, the parent of self::$lvl1_menu_item does not need to be reset to self::$lvl0_menu_item as the parent set_up()/tear_down() methods handle resetting this. Is that correct?

All other tests pass if you move this new test to the top of the file, and this drop-in test method also passes:

public function test_that_peter_did_not_break_the_test_suite() {
	$menu_items = wp_get_nav_menu_items( self::$menu_id );
	$this->assertSame( (int) $menu_items[1]->menu_item_parent, self::$lvl0_menu_item );
}

But just asking to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm, the parent of self::$lvl1_menu_item does not need to be reset to self::$lvl0_menu_item as the parent set_up()/tear_down() methods handle resetting this. Is that correct?

Yes, that is correct.

Changes to the post and post meta table are stored in a transaction that is started in the test suite's setup and rolled back during the tear-down.

wordpress-develop/tests/phpunit/includes/abstract-testcase.php

Lines 136 to 146 in 389a78e

$this->start_transaction();
$this->expectDeprecated();
add_filter( 'wp_die_handler', array( $this, 'get_wp_die_handler' ) );
}
/**
* After a test method runs, resets any state in WordPress the test method might have changed.
*/
public function tear_down() {
global $wpdb, $wp_query, $wp;
$wpdb->query( 'ROLLBACK' );

// Create a new level zero menu item.
$new_lvl0_menu_item = wp_update_nav_menu_item(
self::$menu_id,
0,
array(
'menu-item-title' => 'Root menu item with high ID',
'menu-item-url' => '#',
'menu-item-status' => 'publish',
)
);

// Reparent level 1 menu item to the new level zero menu item.
self::$lvl1_menu_item = wp_update_nav_menu_item(
self::$menu_id,
self::$lvl1_menu_item,
array(
'menu-item-parent-id' => $new_lvl0_menu_item,
)
);

// Delete the old level zero menu item.
wp_delete_post( self::$lvl0_menu_item, true );

// Render the menu.
$menu_html = wp_nav_menu(
array(
'menu' => self::$menu_id,
'echo' => false,
)
);

$this->assertStringContainsString(
sprintf(
'<li id="menu-item-%1$d" class="menu-item menu-item-type-custom menu-item-object-custom menu-item-has-children menu-item-%1$d">',
$new_lvl0_menu_item
),
$menu_html,
'The level zero menu item should appear in the menu.'
);

}
}