Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#65884 new defect (bug)

wp_map_nav_menu_locations() reuses $slug for a nested loop, so some new theme locations are skipped

Reported by: bejignesh Owned by:
Priority: normal Milestone: Awaiting Review
Component: Menus Version: 4.9
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

wp_map_nav_menu_locations() uses $slug for two nested loops. The outer one at
wp-includes/nav-menu.php:1276 iterates the slug group, and the loop at :1292, nested three
levels inside it, iterates the same group again with the same variable name.

PHP leaves a foreach variable at its last assigned value, so once any new location
descends into the old-location loops, $slug no longer holds the outer loop's value. The
test at :1282 then compares every later new location in that pass against the wrong slug:

foreach ( $slug_group as $slug ) {                          // 1276
    foreach ( $registered_nav_menus as $new_location => $name ) {
        if ( ... stripos( $new_location, $slug ) ... )      // 1282, reads the outer $slug
        foreach ( $old_nav_menu_locations as $location => $menu_id ) {
            foreach ( $slug_group as $slug ) {              // 1292, overwrites it

The skipped location is only recovered if it happens to match a later slug in the same
group. When it matches exactly one slug, nothing picks it up and the assignment is lost.

Measured against trunk, new theme locations on the left, previous theme's assignments on
the right:

primary + primary-menu   old = header, mainmenu     1 of 2 mapped
primary + primary-nav    old = header, mainmenu     1 of 2 mapped
footer  + footer-menu    old = secondary, bottom    1 of 2 mapped
primary + main           old = navigation-menu, top-menu   2 of 2 mapped
header  + header-top     old = primary, top-menu    2 of 2 mapped

primary + main is the case test_location_guessing_one_menu_per_location() already
covers. It passes because main is itself a slug in the group and is picked up on a later
pass, which is why the existing tests do not catch this. header-top recovers the same way
via top. primary-menu, primary-nav and footer-menu match only one slug each, so they
are lost.

Reached on every theme switch through _wp_menus_changed(), hooked to after_switch_theme
in wp-includes/default-filters.php:376, and from
wp-includes/class-wp-customize-nav-menus.php:734.

Present since [41237] (#39692) in 4.9. The inner loop has carried the outer loop's variable
name from the first version.

Patch renames the inner variable and adds a regression test. The test fails without the
source change and is the only failure of the 106 in the menu group; menu and customize
are green with it.

Change History (1)

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


4 weeks ago
#1

  • Keywords has-patch has-unit-tests added

wp_map_nav_menu_locations() uses $slug for two nested loops: the one over the slug group
at nav-menu.php:1276, and the one at :1292 nested three levels inside it. PHP leaves a
foreach variable at its last assigned value, so once any new location descends into the
old-location loops, the comparison at :1282 runs against whatever the inner loop left
behind rather than the outer loop's slug.

A skipped location is only recovered if it happens to match a later slug in the same group.
When it matches exactly one, nothing picks it up and the assignment is lost.

Measured on trunk. New theme locations on the left, previous theme's assignments on the right:

New locations Previous assignments Before After
primary, primary-menu header, mainmenu 1 of 2 mapped 2 of 2
primary, primary-nav header, mainmenu 1 of 2 mapped 2 of 2
footer, footer-menu secondary, bottom 1 of 2 mapped 2 of 2
primary, main navigation-menu, top-menu 2 of 2 2 of 2
header, header-top primary, top-menu 2 of 2 2 of 2

The last two rows are why this has gone unnoticed. primary + main is exactly
test_location_guessing_one_menu_per_location(), and it passes today because main is
itself a slug in the group, so the skipped location is picked up on a later pass.
header-top recovers the same way via top. Only a location matching a single slug is
actually lost.

Reached on every theme switch via _wp_menus_changed() on after_switch_theme
(default-filters.php:376) and from class-wp-customize-nav-menus.php:734.

Present since [41237] (#39692) in 4.9 — the inner loop has carried the outer loop's variable
name since the first version.

## Approach

Renamed the inner loop variable to $old_slug. That is the whole change; the matching
logic is untouched. I confirmed the rename is what fixes it by applying it in isolation to
an extracted copy of the function and re-running the table above.

## Testing

The new test fails without the nav-menu.php change and is the only failure of the 106 in
the menu group, so it fails on its own assertion rather than a precondition. With the
change, menu is 106/106 and customize is 243/243. phpcs passes on both files.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting this description and a first pass at the test. I confirmed the mechanism
myself against the source, produced the before/after table by running the real function,
verified the rename alone is what changes the outcome, checked the test fails without the
patch, ran the menu and customize suites and phpcs, and I take responsibility for the
change.

Note: See TracTickets for help on using tickets.