Menus: Fix wp_get_nav_menu_items() dropping items when the term count is stale or the query is not publish-only. - #13513
MarcinDudekDev wants to merge 1 commit into
Conversation
…r the query is not publish-only. wp_get_nav_menu_items() short-circuited to an empty array whenever $menu->count was 0. That gate was introduced in [556492a] for #55372 (the tax_query conversion), replacing the previous status-agnostic ! empty() check on the queried items. $menu->count is not a safe proxy for "no items match these args": * _update_post_term_count() only counts 'publish' nav_menu_item posts, so a draft-only or trash-only menu reports count 0 even though a caller querying 'draft', 'any', 'publish,draft', or 'trash' has matching items. * The count can be stale — wp_defer_term_counting() leaves it at 0 while items are already attached, and wp_get_nav_menu_object() returns a passed WP_Term object verbatim, so a caller holding an old snapshot is also gated. This also made wp_update_nav_menu_item() assign colliding menu_order = 0 positions: it queries 'publish,draft' items to position a new item, so a second draft on a draft-only menu could not be placed after the first. Remove the gate and always run the query, restoring pre-#55372 behavior. get_posts() already returns array() for a genuinely empty menu, and the query is served from the persistent 'post-queries' cache group, so warm caches do not gain a per-request query; one cold query for a truly empty menu is the accepted tradeoff. Adds 7 regression cases covering: draft-only menus queried with 'draft', 'any', and 'publish,draft'; menu_order positioning of a second draft item; deferred term counting; stale WP_Term snapshots; and trash-only menus queried with 'trash'. Each fails before this change and passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 reportTested against a fresh 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:
Example before/after ( The diff also applied cleanly ( No regressions found across the surrounding menu/customizer/REST/ajax test surface. LGTM. |
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 556492a] (#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