Make WordPress Core

Opened 17 months ago

Closed 14 months ago

Last modified 14 months ago

#57122 closed defect (bug) (fixed)

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

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Menus Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Following [54478] for #28620, it's possible for wp_nav_menu() to throw an undefined offset notice if a menu item has a lower post ID than its parent.

This can occur if the menu items are created and then restructured/re-parented after they are initially created.

The following test demonstrates the problem.

<?php

// Add to tests/phpunit/tests/menu/wp-nav-menu.php

public function test_parent_with_higher_id_should_not_error() {
        // 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(
                        '<ul id="menu-test" class="menu"><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 as the first menu item.'
        );

}

Note: wp_get_nav_menu_items() queries by menu_order, so the sorting for items with a matching menu_order is somewhat undefined. Results may differ depending on the DB engine/version/phase of the moon.

This is the result on my local environment:

$ phpunit --filter test_parent_with_higher_id_should_not_throw
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

E                                                                   1 / 1 (100%)

Time: 12.79 seconds, Memory: 242.05 MB

There was 1 error:

1) Tests_Menu_wpNavMenu::test_parent_with_higher_id_should_not_throw
Undefined offset: 5

/vagrant/wordpress-develop/src/wp-includes/nav-menu-template.php:210
/vagrant/wordpress-develop/tests/phpunit/tests/menu/wp-nav-menu.php:300

Change History (8)

This ticket was mentioned in PR #3635 on WordPress/wordpress-develop by @peterwilsoncc.


17 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 @SergeyBiryukov
17 months ago

  • Milestone changed from Awaiting Review to 6.1.2

#3 follow-up: @peterwilsoncc
17 months ago

I noticed this-morning my local environment was on an old commit when I tested this yesterday. It's possible it was fixed in [54801] for #56946.

I'll double check to make sure and close this if needs be.

#4 @SergeyBiryukov
16 months ago

#57160 was marked as a duplicate.

#5 @peterwilsoncc
16 months ago

#57333 was marked as a duplicate.

#6 in reply to: ↑ 3 @peterwilsoncc
14 months ago

  • Milestone changed from 6.1.2 to 6.2

Replying to peterwilsoncc:

I noticed this-morning my local environment was on an old commit when I tested this yesterday. It's possible it was fixed in [54801] for #56946.

I'll double check to make sure and close this if needs be.

This was indeed fixed in [54801] for WordPress 6.1.1

In the initial push the tests failed in the linked pull request. Rebasing against trunk has the tests passing.

I'll commit the additional test to close this ticket. I've moved this ticket to the 6.2 milestone for the tests commit.

#7 @peterwilsoncc
14 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 55328:

Menus: Test creating parent after a child doesn't throw an error.

As menus are re-arranged, it's possible a menu item was created prior to its parent.

This introduces a test to ensure the order in which menu items are created relevant to their parents does not trigger errors.

Props costdev, peterwilsoncc.
Fixes #57122.

Note: See TracTickets for help on using tickets.