WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 2 months ago

#52949 closed defect (bug) (fixed)

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 commit fixed-major
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 4 months ago.
52949.diff (976 bytes) - added by Chouby 4 months ago.
Menus.png (115.5 KB) - added by mukesh27 4 months ago.
52949.2.diff (833 bytes) - added by Chouby 4 months ago.
52949.3.diff (1.7 KB) - added by Chouby 4 months ago.

Download all attachments as: .zip

Change History (28)

@Chouby
4 months ago

@Chouby
4 months ago

#1 @Chouby
4 months ago

  • Keywords has-patch added

#2 @audrasjb
4 months 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
4 months ago

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

#4 @audrasjb
4 months ago

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

#5 @audrasjb
4 months ago

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

#6 @Chouby
4 months 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 4 months ago by Chouby (previous) (diff)

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


4 months ago

#8 @davidbaumwald
4 months ago

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

#9 follow-up: @davidbaumwald
4 months 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 4 months ago by davidbaumwald (previous) (diff)

#10 in reply to: ↑ 9 @mukesh27
4 months 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
4 months ago

@Chouby
4 months ago

#11 @Chouby
4 months 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
4 months 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
4 months ago

#13 @Chouby
4 months 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.


4 months ago

#15 @audrasjb
4 months 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.


4 months ago

#17 @peterwilsoncc
3 months 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
3 months ago

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

#19 @dd32
2 months 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

#20 @peterwilsoncc
2 months ago

  • Keywords commit added

Tested manually, working for me too.

#21 @peterwilsoncc
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#22 @peterwilsoncc
2 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for committing to 5.7 branch.

#23 @peterwilsoncc
2 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.