WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 34 hours ago

#52949 assigned defect (bug)

All available nav menu locations checked by default when creating a new menu

Reported by: Chouby Owned by: davidbaumwald
Milestone: 5.7.3 Priority: normal
Severity: normal Version: 5.5
Component: Menus Keywords: has-patch
Focuses: administration Cc:

Description

  1. Start from a WordPress fresh install and activate Twenty Twenty (selected because it has 5 navmenu locations, so the bug is more visible).
  2. Go to Appearance > Menus.
  3. Create a new menu and save (no need to add menu items).
  4. Click on the "Manage Locations" tab.
  5. Assign the unique menu to one location. Let say "Desktop Horizontal Menu" and Save Changes.
  6. Go back to the "Edit Menus" tab and click on "create a new menu".
  7. Observe that all available locations are pre-selected.

Notes:
The issue doesn't occur in the Customizer.
The bug has been introduced in [48051].

Attachments (5)

52949.png (84.7 KB) - added by Chouby 6 weeks ago.
52949.diff (976 bytes) - added by Chouby 6 weeks ago.
Menus.png (115.5 KB) - added by mukesh27 6 weeks ago.
52949.2.diff (833 bytes) - added by Chouby 6 weeks ago.
52949.3.diff (1.7 KB) - added by Chouby 5 weeks ago.

Download all attachments as: .zip

Change History (24)

@Chouby
6 weeks ago

@Chouby
6 weeks ago

#1 @Chouby
6 weeks ago

  • Keywords has-patch added

#2 @audrasjb
6 weeks ago

  • Milestone changed from Awaiting Review to 5.7.1

Related: #44286

Thanks @Chouby!

This was introduced by my fault in milestone 5.5.

I can reproduce the bug as well.
I tested your patch and it seems to fix the issue.

Given this is a very small and self contained fix, I'd lean towards fixing this in the next point release. Moving for 5.7.1 consideration.

#3 @audrasjb
6 weeks ago

  • Owner set to audrasjb
  • Status changed from new to accepted

#4 @audrasjb
6 weeks ago

@Chouby the patch doesn't fix the issue on my side
See video capture: https://i.gyazo.com/dc73bf2c1b72a7afa3dd023c417695d4.mp4

#5 @audrasjb
6 weeks ago

But it looks better when replacing your ! empty with an empty check…

#6 @Chouby
6 weeks ago

You are right. The patch is not sufficient. I focused on the UI, missing that saving a menu with no location selected would assign it all the available locations. Replacing ! empty() by empty() doesn't fix anything for me.

Last edited 6 weeks ago by Chouby (previous) (diff)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 weeks ago

#8 @davidbaumwald
6 weeks ago

  • Owner changed from audrasjb to davidbaumwald
  • Status changed from accepted to assigned

#9 follow-up: @davidbaumwald
6 weeks ago

  • Keywords needs-refresh added

Looked into this a bit, and the issue isn't the isset logic; it's that $nav_menu_selected_id is 0 for a new menu being created. I've verified that adding a 0 !== $nav_menu_selected_id to the $checked conditional resolves the issue.

@Chouby Are you up for re-working your patch to include this logic?

One other note, after adding the third evaluation, I think it might be a bit more readable if we do something like this:

$checked = false;

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

Versus the existing, one-liner setup.

$checked = isset( $menu_locations[ $location ] ) && 0 !== $nav_menu_selected_id && $menu_locations[ $location ] === $nav_menu_selected_id;
Last edited 6 weeks ago by davidbaumwald (previous) (diff)

#10 in reply to: ↑ 9 @mukesh27
6 weeks ago

Replying to davidbaumwald:

$checked = false;

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

Versus the existing, one-liner setup.

$checked = isset( $menu_locations[ $location ] ) && 0 !== $nav_menu_selected_id && $menu_locations[ $location ] === $nav_menu_selected_id;

After using this code snippet if I'm going to add new menu without any menu items and menu location it automatically select all menu location.

Check attached screenshot

@mukesh27
6 weeks ago

@Chouby
6 weeks ago

#11 @Chouby
6 weeks ago

  • Keywords needs-refresh removed

There are indeed 2 bugs: the first I originally reported (this is only a UI bug) and the second reported in the video by @audrasjb which impacts the data saved in the database.

My first patch or the variation proposed by @davidbaumwald in #comment:10 fix only the first one.

So I came back to review the code introduced in [48051] which lead me to 52949.2.diff. According to my tests, this fixes the 2 issues.

#12 @peterwilsoncc
5 weeks ago

@Chouby Should 52949.2.diff include the changes from 52949.diff too?

I'm only able to see the fixes by applying both patches but want to make sure I'm not making an error while reproducing/hitting a browser caching issue.

thanks.

@Chouby
5 weeks ago

#13 @Chouby
5 weeks ago

@peterwilsoncc, yes you are right. I mixed myself in my patches and tests. We need both. 52949.3.diff merges the proposal from @davidbaumwald and 52949.2.diff.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 weeks ago

#15 @audrasjb
5 weeks ago

Hi there,

Just tested the last patch 52949.3.diff and it completely fixes the above issues on my side.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


5 weeks ago

#17 @peterwilsoncc
5 weeks ago

  • Milestone changed from 5.7.1 to 5.7.2

I've moved this to 5.7.2 as the release is due later this week. As the recommended process is to use the customizer I think it's fine to wait a little longer.

#18 @audrasjb
2 weeks ago

I re-tested the patch 52949.3.diff solves the issue on my side.
Pinging @SergeyBiryukov for double check.

#19 @dd32
34 hours ago

  • Milestone changed from 5.7.2 to 5.7.3

WordPress 5.7.2 has been released, moving open tickets to 5.7.3

Note: See TracTickets for help on using tickets.