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

[Chouby]

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

[audrasjb]

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.

[audrasjb]

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

[audrasjb]

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

[Chouby]

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.

[davidbaumwald]

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;

[mukesh27]

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

[Chouby]

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.

[peterwilsoncc]

@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]

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

[audrasjb]

Hi there,

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

[peterwilsoncc]

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.

[audrasjb]

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

[dd32]

WordPress 5.7.2 has been released, moving open tickets to 5.7.3

[peterwilsoncc]

Tested manually, working for me too.

[peterwilsoncc]

In 50938:

Menus: Do not auto-set locations for new menus.

Do not auto-set new menus to all vacant locations on the Appearance > Menus screen in the dashboard.

Follow up to [48051].

Props Chouby, audrasjb, davidbaumwald, mukesh27.
Fixes #52949.

[peterwilsoncc]

Reopening for committing to 5.7 branch.

[peterwilsoncc]

In 50988:

Menus: Do not auto-set locations for new menus.

Do not auto-set new menus to all vacant locations on the Appearance > Menus screen in the dashboard.

Follow up to [48051].

Props Chouby, audrasjb, davidbaumwald, mukesh27.
Merges [50938] in to the 5.7 branch.
Fixes #52949.