Opened 3 weeks ago
Last modified 3 weeks ago
#66102 new defect (bug)
wp_get_nav_menu_items() returns an empty array whenever the menu term's count is 0 or stale, dropping items that match the query
| Reported by: | MythThrazz | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | Menus | Version: | 6.0 |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: |
Description
wp_get_nav_menu_items() short-circuits before running its item query whenever the menu term's count is 0:
// src/wp-includes/nav-menu.php $args = wp_parse_args( $args, $defaults ); if ( $menu->count > 0 ) { $items = get_posts( $args ); } else { $items = array(); }
$menu->count is not a safe proxy for "no items match these args":
- It counts published items only.
_update_post_term_count()counts posts withpost_status = 'publish'(the default of theupdate_post_term_count_statusesfilter), so a menu whose items are alldraftortrashreportscount = 0even though a caller querying'draft','any','publish,draft', or'trash'has matching items. - It can be stale.
wp_defer_term_counting( true )leaves the count at 0 while items are already attached, andwp_get_nav_menu_object()returns a passedWP_Termobject verbatim, so a caller holding a term snapshot taken before items were added is gated as well.
post_status is a documented argument of wp_get_nav_menu_items(), so all of the following return array() while get_posts() with the same arguments returns real items:
- a draft-only menu queried with
post_status'draft','any', or'publish,draft'; - a trash-only menu queried with
'trash'(trashing the last published item returns the count to 0); - a published item attached while term counting is deferred — hidden even from the default
'publish'query; - any item hidden behind a stale
WP_Termsnapshot passed as$menu.
In-core consequences
- Colliding
menu_order.wp_update_nav_menu_item()positions a new item by querying'publish,draft'items and using1 +the last item'smenu_order(src/wp-includes/nav-menu.php, ~line 464). On a menu with no published items — e.g. one being assembled as drafts — the gate empties the list, so the new item getsmenu_order = 0; every subsequent item collides at position 0 instead of being appended. - Duplicate auto-added pages.
_wp_auto_add_pages_to_menu()queries the'publish,draft'item list to skip pages already in the menu (~line 1164). On a menu with no published items the check sees an empty list and inserts a second item for the same page.
Other in-core callers requesting non-default statuses are affected the same way: wp_get_nav_menu_to_edit() queries 'any' and wp_nav_menu_update_menu_items() queries 'draft,publish' (src/wp-admin/includes/nav-menu.php, ~lines 1266 and 1409), so a draft-only menu lists empty on the edit screen and its items can't be updated or deleted there — the save path can't see them either.
Steps to reproduce
$menu_id = wp_create_nav_menu( 'example' ); $menu = wp_get_nav_menu_object( $menu_id ); $item_id = wp_update_nav_menu_item( $menu_id, 0, array( 'menu-item-type' => 'custom', 'menu-item-title' => 'Example', 'menu-item-url' => 'https://example.org/', 'menu-item-status' => 'draft', ) ); // Returns array() — the gate short-circuits because $menu->count is 0 (drafts are not counted): $via_function = wp_get_nav_menu_items( $menu_id, array( 'post_status' => 'draft' ) ); // Returns array( $item_id ) — the underlying query does find the item: $via_query = wp_list_pluck( get_posts( array( 'post_type' => 'nav_menu_item', 'post_status' => 'draft', 'nopaging' => true, 'tax_query' => array( array( 'taxonomy' => 'nav_menu', 'field' => 'term_taxonomy_id', 'terms' => $menu->term_taxonomy_id, ), ), ) ), 'ID' );
Root cause
The gate was introduced in [53010] (git 556492ad88) for #55372, which replaced get_objects_in_term() + $args['include'] with a tax_query to save one query. The previous ! empty( $items ) check tested whether any objects were attached to the term at all — it was status-agnostic and reflected actual attachments. The replacement $menu->count > 0 tests the published count only and can additionally be stale, so the gate now fires in cases the old check never hit.
Proposed fix
Remove the short-circuit and always run get_posts( $args ) — restoring the pre-[53010] approach of letting the query, not the term count, decide whether matching items exist. get_posts() already returns array() for a genuinely empty menu.
A narrower alternative — keeping the gate only for the default post_status === 'publish' query — was considered and rejected: deferred term counting and stale WP_Term snapshots both leave count = 0 while published items exist, so a publish-only gate would still return a wrong array() in precisely the cases this ticket covers.
Performance impact of removing the gate (measured in a review sweep of the attached patch):
- Warm caches, and sites with a persistent object cache: zero additional queries after a one-time cache miss per menu — the query result, including the empty result, is cached in the
post-queriesgroup. (That result cache did not yet exist when the gate was added in 6.0.) - A site with no object cache: one additional indexed
SELECTper empty rendered menu per request — it joinsterm_relationshipsand filters onterm_taxonomy_idplus thetype_status_dateindex, returning 0 rows. - Non-empty menus: identical — the gate already passed through to the same query.
Tests
The patch adds 7 regression cases to Tests_Post_Nav_Menu: draft-only menus queried with 'draft', 'any', and 'publish,draft'; menu_order positioning of a second draft item; deferred term counting; a stale WP_Term snapshot; and a trash-only menu queried with 'trash'. Each fails before the change and passes after.
In the same review sweep, 762 existing tests across the menu, customize, REST menus, ajax nav-menus, and related groups passed with the change applied, and the _wp_auto_add_pages_to_menu() duplicate insert described above was reproduced under the old code and shown to be gone after it.
Notes
- A GitHub pull request for code review will be linked shortly.
- AI assistance was used in preparing the patch and is disclosed in the PR per the WordPress AI guidelines.
Change History (5)
This ticket was mentioned in PR #13513 on WordPress/wordpress-develop by @MythThrazz.
3 weeks ago
#1
- Keywords has-patch has-unit-tests added
@ugyensupport commented on PR #13513:
3 weeks ago
#2
## Test report
Tested against a fresh wordpress-develop clone at trunk (ba9d19e), PHP 8.5.7, PHPUnit 9.6.36, MySQL.
Method: applied only the new test hunk to unpatched trunk first (source untouched) to confirm the 7 new tests reproduce the bug, then applied the source fix on top and re-ran, then ran the broader menu/customizer/REST/ajax suites for regressions.
Results:
| Run | Result |
|---|---|
| 7 new regression tests, *before* fix | 7 failed / 7 — reproduces the bug exactly as described (draft/any/publish,draft/trash queries and the stale-count/deferred-counting cases all return array() instead of the real items)
|
| 7 new regression tests, *after* fix | 7 / 7 passed (36 assertions) |
--group menu | 112 / 112 passed (272 assertions) |
--group customize | 243 / 243 passed (2258 assertions) |
REST menu controllers (wpRestMenusController, wpRestMenuItemsController, wpRestMenuLocationsController) | 25 / 25 passed |
wpCustomizeNavMenus.php (--group ajax) | 28 / 28 passed |
Full Tests_Post_Nav_Menu class (37 existing + 7 new) | 44 / 44 passed |
phpcs --standard=phpcs.xml.dist on the modified file | 0 errors |
Example before/after (test_wp_get_nav_menu_items_draft_only_menu_draft_query):
Before: Failed asserting that two arrays are identical. -Array &0 ( 0 => 4 ) +Array &0 () After: OK
The diff also applied cleanly (git apply --check) against current trunk with no conflicts.
No regressions found across the surrounding menu/customizer/REST/ajax test surface. LGTM.
This ticket was mentioned in PR #13596 on WordPress/wordpress-develop by rashmicodewing.
3 weeks ago
#4
Problem
wp_get_nav_menu_items() short-circuits before running its item query whenever the menu term's count is 0:
$args = wp_parse_args( $args, $defaults ); if ( $menu->count > 0 ) { $items = get_posts( $args ); } else { $items = array(); }
$menu->count is not a safe proxy for "no items match these args":
- It counts published items only.
_update_post_term_count()counts posts withpost_status = 'publish'(the default of theupdate_post_term_count_statusesfilter), so a menu whose items are all draft or trash reportscount = 0even though a caller querying'draft','any','publish,draft', or'trash'has matching items. - It can be stale.
wp_defer_term_counting( true )leaves the count at0while items are already attached, andwp_get_nav_menu_object()returns a passedWP_Termobject verbatim, so a caller holding a term snapshot taken before items were added is gated as well.
post_status is a documented argument of wp_get_nav_menu_items(), so all of the following return array() while get_posts() with the same arguments returns real items:
- a draft-only menu queried with
post_status'draft','any', or'publish,draft'; - a trash-only menu queried with
'trash'(trashing the last published item returns the count to0); - a published item attached while term counting is deferred — hidden even from the default
'publish'query; - any item hidden behind a stale
WP_Termsnapshot passed as$menu.
In-core consequences
- Colliding
menu_order.wp_update_nav_menu_item()positions a new item by querying'publish,draft'items and using1 +the last item'smenu_order. On a menu with no published items — e.g. one being assembled as drafts — the gate empties the list, so the new item getsmenu_order = 0; every subsequent item collides at position 0 instead of being appended. - Duplicate auto-added pages.
_wp_auto_add_pages_to_menu()queries the'publish,draft'item list to skip pages already in the menu. On a menu with no published items the check sees an empty list and inserts a second item for the same page. - Other in-core callers requesting non-default statuses are affected the same way:
wp_get_nav_menu_to_edit()queries'any'andwp_nav_menu_update_menu_items()queries'draft,publish', so a draft-only menu lists empty on the edit screen and its items cannot be updated or deleted there.
Steps to reproduce
$menu_id = wp_create_nav_menu( 'example' ); $menu = wp_get_nav_menu_object( $menu_id ); $item_id = wp_update_nav_menu_item( $menu_id, 0, array( 'menu-item-type' => 'custom', 'menu-item-title' => 'Example', 'menu-item-url' => 'https://example.org/', 'menu-item-status' => 'draft', ) ); // Returns array() — the gate short-circuits because $menu->count is 0: $via_function = wp_get_nav_menu_items( $menu_id, array( 'post_status' => 'draft' ) ); // Returns array( $item_id ) — the underlying query does find the item: $via_query = wp_list_pluck( get_posts( array( 'post_type' => 'nav_menu_item', 'post_status' => 'draft', 'nopaging' => true, 'tax_query' => array( array( 'taxonomy' => 'nav_menu', 'field' => 'term_taxonomy_id', 'terms' => $menu->term_taxonomy_id, ), ), ) ), 'ID' );
Root cause
The gate was introduced in [53010] (556492ad88) for #55372, which replaced get_objects_in_term() + $args['include'] with a tax_query to save one query. The previous ! empty( $items ) check tested whether any objects were attached to the term at all — it was status-agnostic and reflected actual attachments. The replacement $menu->count > 0 tests the published count only and can additionally be stale, so the gate now fires in cases the old check never hit.
Fix
Remove the short-circuit and always run get_posts( $args ) — restoring the pre-[53010] approach of letting the query, not the term count, decide whether matching items exist. get_posts() already returns array() for a genuinely empty menu.
A narrower alternative — keeping the gate only for the default post_status === 'publish' query — was considered and rejected: deferred term counting and stale WP_Term snapshots both leave count = 0 while published items exist, so a publish-only gate would still return a wrong array() in precisely the cases this ticket covers.
Performance impact of removing the gate
- Warm caches, and sites with a persistent object cache: zero additional queries after a one-time cache miss per menu — the query result, including the empty result, is cached in the
post-queriesgroup. (That result cache did not yet exist when the gate was added in 6.0.) - A site with no object cache: one additional indexed
SELECTper empty rendered menu per request — it joinsterm_relationshipsand filters onterm_taxonomy_idplus thetype_status_dateindex, returning 0 rows. - Non-empty menus: identical — the gate already passed through to the same query.
Testing instructions
Run the new regression tests, which fail before the change and pass after:
phpunit --filter Tests_Post_Nav_Menu
Or reproduce manually with the snippet under "Steps to reproduce": before the change $via_function is empty while $via_query is not; after the change both return the same item.
Tests
Adds 8 regression cases to Tests_Post_Nav_Menu, each tagged @ticket 66102:
| Test | Covers |
|---|---|
test_wp_get_nav_menu_items_returns_draft_items_from_uncounted_menu (x3, data provider) | draft-only menu queried with 'draft', 'any', 'publish,draft'
|
test_wp_get_nav_menu_items_returns_trashed_items_from_uncounted_menu | trash-only menu queried with 'trash'
|
test_wp_get_nav_menu_items_returns_items_while_term_counting_is_deferred | wp_defer_term_counting( true )
|
test_wp_get_nav_menu_items_returns_items_for_stale_menu_object | stale WP_Term snapshot passed as $menu
|
test_wp_update_nav_menu_item_appends_to_a_draft_only_menu | menu_order positions stay distinct and ascending
|
test_wp_auto_add_pages_to_menu_does_not_duplicate_a_draft_item | no duplicate auto-added page |
Each of the 8 fails with the src/ change reverted and passes with it applied.
Locally (single site, PHP 8.2 / MySQL 8.0) the following all pass with the change applied: --group menu (113 tests), the customize suite (119), the REST menus controllers (79), and --group ajax (190, 1 pre-existing skip). PHPCS is clean on both changed files.
AI disclosure
AI assistance was used in preparing this patch, and is disclosed per the WordPress AI guidelines.
🤖 Generated with Claude Code
rashmicodewing commented on PR #13596:
3 weeks ago
#5
Closing this — it duplicates #13513, which was opened four days earlier by @MarcinDudekDev and makes the same change to wp_get_nav_menu_items() (removing the $menu->count > 0 gate so the item query always runs). I missed the existing PR on the ticket before opening this one; apologies for the noise.
#13513 already has a test report on it and should be the one reviewed for #66102. Deferring to it.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
## Trac ticket
https://core.trac.wordpress.org/ticket/66102
## What & why
wp_get_nav_menu_items()short-circuited toarray()whenever thenav_menuterm's cachedcountwas 0:$menu->count(maintained by_update_post_term_count()) counts onlypublishitems and can be stale, butpost_statusis a documented argument of the function. So the gate returnedarray()in cases whereget_posts()with the same args returns real items:post_status=draft/any/publish,draft/trashwp_defer_term_counting( true )(bulk import) while the count is still 0WP_Termmenu object (count captured before an item was added)It also broke core itself:
wp_update_nav_menu_item()positions new items viawp_get_nav_menu_items( $menu_id, array( 'post_status' => 'publish,draft' ) ), so on a menu with no *published* items every added item receivedmenu_order = 0(colliding positions)._wp_auto_add_pages_to_menu()(samepublish,draftquery) could insert a duplicate item for the same page.The gate was introduced in [changeset 556492ad88] (#55372) when the query was converted to
tax_query, replacing the previous status-agnostic! empty()check — i.e. it is a regression.## The fix
Remove the
$menu->countshort-circuit; always runget_posts( $args ).$menu->countis not a safe proxy for "no items match these args".get_posts()already returnsarray()for a genuinely empty menu, and that query is cached in the persistentpost-queriesgroup, so warm caches issue no extra query; a truly empty menu costs one cheap indexed query on a cold cache.## Testing
Adds 7 regression tests to
Tests_Post_Nav_Menu(each fails on trunk, passes with the fix): draft-only queries (draft/any/publish,draft), trash-only +trash,wp_defer_term_counting, staleWP_Term, and thewp_update_nav_menu_item()position increment.Tests_Post_Nav_Menu: pass (37 existing + 7 new)--group menu: pass;--group customize: pass; REST menu controllers + nav-fallback/converter/editor: pass (762 tests green across groups)phpcs --standard=phpcs.xml.dist: 0 errorsPerf: warm/object-cache sites — 0 extra queries (verified end-to-end, incl. a persistent drop-in); non-empty menus — identical; cache-less sites — +1 cheap indexed SELECT per *empty rendered* menu, per page. A
post_status==='publish'-only "surgical" gate was evaluated and rejected: it still returns wrongarray()under deferred counting / stale term snapshots (count=0 while publish items exist).---
AI assistance: Yes
Tool(s): Claude Code (Anthropic), Devin
Model(s): Claude Opus 4.8 (orchestration), Devin (implementation + independent adversarial review)
Used for: Diagnosis (measured that the item-list query is already cached by WP_Query, redirecting the effort to this real gate bug), implementation of the one-line fix + regression tests, and multiple independent adversarial review rounds (regression sweep, perf measurement, negative controls). Reviewed and taking responsibility for the output.
🤖 Generated with Claude Code