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
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
wp_map_nav_menu_locations()uses$slugfor two nested loops: the one over the slug groupat
nav-menu.php:1276, and the one at:1292nested three levels inside it. PHP leaves aforeachvariable at its last assigned value, so once any new location descends into theold-location loops, the comparison at
:1282runs against whatever the inner loop leftbehind 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:
primary,primary-menuheader,mainmenuprimary,primary-navheader,mainmenufooter,footer-menusecondary,bottomprimary,mainnavigation-menu,top-menuheader,header-topprimary,top-menuThe last two rows are why this has gone unnoticed.
primary+mainis exactlytest_location_guessing_one_menu_per_location(), and it passes today becausemainisitself a slug in the group, so the skipped location is picked up on a later pass.
header-toprecovers the same way viatop. Only a location matching a single slug isactually lost.
Reached on every theme switch via
_wp_menus_changed()onafter_switch_theme(
default-filters.php:376) and fromclass-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 matchinglogic 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.phpchange and is the only failure of the 106 inthe
menugroup, so it fails on its own assertion rather than a precondition. With thechange,
menuis 106/106 andcustomizeis 243/243.phpcspasses 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
menuandcustomizesuites andphpcs, and I take responsibility for thechange.