Prevent error in Menu location checkbox settings by sabernhardt · Pull Request #10859 · WordPress/wordpress-develop

sabernhardt · 2026-02-03T23:34:00Z

Creates a text string to display for invalid menu ID.
Assigns id and aria-describedby to the connect the span with the checkboxes.

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.

…bedby` for checkboxes

github-actions · 2026-02-03T23:34:12Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props sabernhardt, mukesh27, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2026-02-03T23:40:00Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

sabernhardt · 2026-02-03T23:52:27Z

Before the patch, an invalid menu ID can result in a PHP error:

With the patch, the text says that the ID is invalid (but it is not an error message):

Note: To assign an invalid ID, with Twenty Twenty-One as the active theme, I needed to edit the theme_mods_twentytwentyone value in the database, in the *_options table.

src/wp-admin/nav-menus.php

mukeshpanchal27 · 2026-02-04T04:55:22Z

src/wp-admin/nav-menus.php

 															/* translators: %s: Menu name. */
 															_x( '(Currently set to: %s)', 'menu location' ),
-															wp_get_nav_menu_object( $menu_locations[ $location ] )->name
+															is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )


I suggest using a similar defensive approach as used here: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/nav-menus.php#L643-L646

$menu_name = __( 'an invalid menu ID' ); if ( is_nav_menu( $menu_locations[ $location ] ) ) { $menu_object = wp_get_nav_menu_object( $menu_locations[ $location ] ); if ( ! is_wp_error( $menu_object ) ) { $menu_name = $menu_object->name; } }

The is_nav_menu() function already checks for an error from wp_get_nav_menu_object(), and it also makes sure it is not false or an incorrect taxonomy (as noted on the ticket).

Make sense. Thanks for details.

mukeshpanchal27 · 2026-02-04T04:55:58Z

@sabernhardt Request some changes on PR.

sabernhardt · 2026-02-06T18:11:13Z

src/wp-admin/nav-menus.php

 										foreach ( $locations as $location => $description ) :
-											$checked = false;
+											$checked          = false;
+											$aria_describedby = '';


The $aria_describedby variable is defined at the top of the foreach loop. When I tried setting the variable conditionally, checking isset() pulled values from other display locations (the variable was set earlier in the loop).

sabernhardt · 2026-02-06T18:15:31Z

src/wp-admin/nav-menus.php

+													<?php if ( '' !== $aria_describedby ) : ?>
+														aria-describedby="<?php echo esc_attr( $aria_describedby ); ?>"
+													<?php endif; ?>
+												/>


Yes, this input is still self-closing. That helps a little to identify where the multi-line tag ends. Until WordPress replaces all of the self-closing tags, I want to keep this here.

Copilot

Pull request overview

Updates the nav menus admin screen to avoid errors when a theme location is assigned to an invalid menu ID, and improves accessibility by associating the “currently set to” text with the location checkbox via aria-describedby.

Changes:

Add fallback display text when a theme location points to an invalid menu ID (avoids fatal when accessing ->name).
Add id to the “currently set to” <span> and conditionally add aria-describedby on the corresponding checkbox.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-06T19:38:38Z

src/wp-admin/nav-menus.php

 															_x( '(Currently set to: %s)', 'menu location' ),
-															wp_get_nav_menu_object( $menu_locations[ $location ] )->name
+															is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )


This uses is_nav_menu() and then calls wp_get_nav_menu_object() again, which results in duplicate lookups (since is_nav_menu() already calls wp_get_nav_menu_object()). Consider fetching the menu object once, validating it, and then using its name to avoid redundant work and simplify the expression.

Copilot · 2026-02-06T19:38:38Z

src/wp-admin/nav-menus.php

+													<span class="theme-location-set" id="theme-location-set-<?php echo esc_attr( $location ); ?>">
 													<?php
 														printf(
 															/* translators: %s: Menu name. */


The translator comment says %s is a menu name, but in the invalid-ID case %s becomes a fallback message instead. Please update the translator comment (or refactor) so it accurately describes all possible substitution values.

Suggested change

/* translators: %s: Menu name. */

/* translators: %s: Menu name or a message indicating an invalid menu ID. */

The text would depend on whether the message changes, but possibly:
Menu name or a message indicating that the menu was not found.

Copilot · 2026-02-06T19:38:38Z

src/wp-admin/nav-menus.php

 															/* translators: %s: Menu name. */
 															_x( '(Currently set to: %s)', 'menu location' ),
-															wp_get_nav_menu_object( $menu_locations[ $location ] )->name
+															is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )


The menu name is output without escaping (->name is printed directly into HTML). Please escape the computed display string for the %s placeholder (e.g., esc_html(...)) so the output is safe and consistent with WP output-escaping practices.

Suggested change

is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )

esc_html(

is_nav_menu( $menu_locations[ $location ] )

? wp_get_nav_menu_object( $menu_locations[ $location ] )->name

: __( 'an invalid menu ID' )

)

This is not necessary. Core translations are considered trusted.

Oh wait, nevermind, the name is not a translation string. So yes, this is good feedback.

Copilot · 2026-02-06T19:38:39Z

src/wp-admin/nav-menus.php

+															is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )
 														);


The fallback text an invalid menu ID is user-facing and uses internal jargon (“ID”). Consider a more user-friendly string (and possibly including the numeric ID for debugging) so admins understand what’s wrong.

another possibility: 'a nonexistent menu'

or just "unknown menu"

Copilot · 2026-02-06T19:38:39Z

src/wp-admin/nav-menus.php

 											if ( isset( $menu_locations[ $location ] )
 												&& 0 !== $nav_menu_selected_id
 												&& $menu_locations[ $location ] === $nav_menu_selected_id
 											) {
 													$checked = true;
 											}


The assignment inside this if block is over-indented compared to surrounding control structures in this file, which breaks WP coding style and makes diffs noisier. Please fix indentation so the block contents are indented exactly one level inside the if.

Copilot · 2026-02-06T19:38:39Z

src/wp-admin/nav-menus.php

+											if ( ! empty( $menu_locations[ $location ] )
+												&& $menu_locations[ $location ] !== $nav_menu_selected_id
+											) {
+													$aria_describedby = "theme-location-set-$location";
+											}


Same indentation issue here: $aria_describedby = "theme-location-set-$location"; is indented deeper than expected for the body of the if block. Adjust indentation to match the rest of the file's style.

Create text string for invalid menu ID, and add id and `aria-descri…

2429319

…bedby` for checkboxes

mukeshpanchal27 reviewed Feb 4, 2026

View reviewed changes

src/wp-admin/nav-menus.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Feb 4, 2026

View reviewed changes

Move aria-describedby escaping later

d07b0b3

sabernhardt commented Feb 6, 2026

View reviewed changes

westonruter requested a review from Copilot February 6, 2026 19:34

Copilot started reviewing on behalf of westonruter February 6, 2026 19:35 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

	/* translators: %s: Menu name. */
	/* translators: %s: Menu name or a message indicating an invalid menu ID. */

		is_nav_menu( $menu_locations[ $location ] ) ? wp_get_nav_menu_object( $menu_locations[ $location ] )->name : __( 'an invalid menu ID' )
		);

Conversation

sabernhardt commented Feb 3, 2026

Uh oh!

github-actions bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 3, 2026

Test using WordPress Playground

Some things to be aware of

Uh oh!

sabernhardt commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

westonruter Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 3, 2026 •

edited

Loading

sabernhardt commented Feb 3, 2026 •

edited

Loading

westonruter Feb 7, 2026 •

edited

Loading