Skip to content

Prevent error in Menu location checkbox settings #10859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sabernhardt wants to merge 2 commits into
base: trunk
Choose a base branch
from
+19 −5
Open

Prevent error in Menu location checkbox settings #10859

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2429319
Create text string for invalid menu ID, and add `id` and `aria-descri…
sabernhardt Feb 3, 2026
d07b0b3
Move `aria-describedby` escaping later
sabernhardt Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/wp-admin/nav-menus.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,25 +1200,39 @@ function wp_nav_menu_max_depth( $classes ) {
<legend class="menu-settings-group-name howto"><?php _e( 'Display location' ); ?></legend>
<?php
foreach ( $locations as $location => $description ) :
$checked = false;
$checked = false;
$aria_describedby = '';
Copy link
Author

@sabernhardt sabernhardt Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


if ( isset( $menu_locations[ $location ] )
&& 0 !== $nav_menu_selected_id
&& $menu_locations[ $location ] === $nav_menu_selected_id
) {
$checked = true;
}
Comment on lines 1206 to 1211
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

if ( ! empty( $menu_locations[ $location ] )
&& $menu_locations[ $location ] !== $nav_menu_selected_id
) {
$aria_describedby = "theme-location-set-$location";
}
Comment on lines +1213 to +1217
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
?>
<div class="menu-settings-input checkbox-input">
<input type="checkbox"<?php checked( $checked ); ?> name="menu-locations[<?php echo esc_attr( $location ); ?>]" id="locations-<?php echo esc_attr( $location ); ?>" value="<?php echo esc_attr( $nav_menu_selected_id ); ?>" />
<input type="checkbox"<?php checked( $checked ); ?>
name="menu-locations[<?php echo esc_attr( $location ); ?>]"
id="locations-<?php echo esc_attr( $location ); ?>"
value="<?php echo esc_attr( $nav_menu_selected_id ); ?>"
<?php if ( '' !== $aria_describedby ) : ?>
aria-describedby="<?php echo esc_attr( $aria_describedby ); ?>"
<?php endif; ?>
/>
Copy link
Author

@sabernhardt sabernhardt Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<label for="locations-<?php echo esc_attr( $location ); ?>"><?php echo $description; ?></label>
<?php if ( ! empty( $menu_locations[ $location ] ) && $menu_locations[ $location ] !== $nav_menu_selected_id ) : ?>
<span class="theme-location-set">
<?php if ( '' !== $aria_describedby ) : ?>
<span class="theme-location-set" id="theme-location-set-<?php echo esc_attr( $location ); ?>">
<?php
printf(
/* translators: %s: Menu name. */
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. */

Copilot uses AI. Check for mistakes.
Copy link
Author

@sabernhardt sabernhardt Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_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' )
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
}

Copy link
Author

@sabernhardt sabernhardt Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. Thanks for details.

Comment on lines 1234 to +1235
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' )
)

Copilot uses AI. Check for mistakes.
Copy link
Member

@westonruter westonruter Feb 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary. Core translations are considered trusted.

Copy link
Member

@westonruter westonruter Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
Comment on lines +1235 to 1236
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@sabernhardt sabernhardt Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another possibility: 'a nonexistent menu'

Copy link
Member

@westonruter westonruter Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just "unknown menu"

?>
</span>
Expand Down
Loading