Make WordPress Core

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 with post_status = 'publish' (the default of the update_post_term_count_statuses filter), so a menu whose items are all draft or trash reports count = 0 even 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, and wp_get_nav_menu_object() returns a passed WP_Term object 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_Term snapshot passed as $menu.

In-core consequences

  • Colliding menu_order. wp_update_nav_menu_item() positions a new item by querying 'publish,draft' items and using 1 + the last item's menu_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 gets menu_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-queries group. (That result cache did not yet exist when the gate was added in 6.0.)
  • A site with no object cache: one additional indexed SELECT per empty rendered menu per request — it joins term_relationships and filters on term_taxonomy_id plus the type_status_date index, 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

## Trac ticket
https://core.trac.wordpress.org/ticket/66102

## What & why
wp_get_nav_menu_items() short-circuited to array() whenever the nav_menu term's cached count was 0:

if ( $menu->count > 0 ) {
    $items = get_posts( $args );
} else {
    $items = array();
}

$menu->count (maintained by _update_post_term_count()) counts only publish items and can be stale, but post_status is a documented argument of the function. So the gate returned array() in cases where get_posts() with the same args returns real items:

  • draft-only / trash-only menus queried with post_status = draft / any / publish,draft / trash
  • a published item attached under wp_defer_term_counting( true ) (bulk import) while the count is still 0
  • a stale WP_Term menu object (count captured before an item was added)

It also broke core itself: wp_update_nav_menu_item() positions new items via wp_get_nav_menu_items( $menu_id, array( 'post_status' => 'publish,draft' ) ), so on a menu with no *published* items every added item received menu_order = 0 (colliding positions). _wp_auto_add_pages_to_menu() (same publish,draft query) 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->count short-circuit; always run get_posts( $args ). $menu->count is not a safe proxy for "no items match these args". get_posts() already returns array() for a genuinely empty menu, and that query is cached in the persistent post-queries group, 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, stale WP_Term, and the wp_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 errors

Perf: 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 wrong array() 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

​@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.

#3 @SergeyBiryukov
3 weeks ago

  • Milestone Awaiting Review → 7.2

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 with post_status = 'publish' (the default of the update_post_term_count_statuses filter), so a menu whose items are all draft or trash reports count = 0 even 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, and wp_get_nav_menu_object() returns a passed WP_Term object 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_Term snapshot passed as $menu.

In-core consequences

  • Colliding menu_order. wp_update_nav_menu_item() positions a new item by querying 'publish,draft' items and using 1 + the last item's menu_order. On a menu with no published items — e.g. one being assembled as drafts — the gate empties the list, so the new item gets menu_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' and wp_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-queries group. (That result cache did not yet exist when the gate was added in 6.0.)
  • A site with no object cache: one additional indexed SELECT per empty rendered menu per request — it joins term_relationships and filters on term_taxonomy_id plus the type_status_date index, 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.

Note: See TracTickets for help on using tickets.