Menus: Stop a nested loop clobbering the slug it is matching against. - #13058
Menus: Stop a nested loop clobbering the slug it is matching against.#13058jigneshbhavani wants to merge 1 commit into
Conversation
`wp_map_nav_menu_locations()` used `$slug` for both the loop over the slug group and a loop nested three levels inside it. PHP leaves a `foreach` variable at its last assigned value, so once a new location descended into the old-location loops, every later new location in that pass was compared against the wrong slug and skipped. A skipped location was only recovered if it happened to match a later slug in the same group, which is why `test_location_guessing_one_menu_per_location()` passes: `main` is itself a group slug. A location matching exactly one slug, such as `primary-menu`, had nothing to pick it up and lost its assignment. See #65884.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
1 participant
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.Trac ticket: https://core.trac.wordpress.org/ticket/65884
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.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.