Opened 4 years ago
Closed 4 years 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
- Start from a WordPress fresh install and activate Twenty Twenty (selected because it has 5 navmenu locations, so the bug is more visible).
- Go to Appearance > Menus.
- Create a new menu and save (no need to add menu items).
- Click on the "Manage Locations" tab.
- Assign the unique menu to one location. Let say "Desktop Horizontal Menu" and Save Changes.
- Go back to the "Edit Menus" tab and click on "create a new menu".
- 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)
Change History (28)
#4
@
4 years ago
@Chouby the patch doesn't fix the issue on my side
See video capture: https://i.gyazo.com/dc73bf2c1b72a7afa3dd023c417695d4.mp4
#6
@
4 years 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.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 years ago
#8
@
4 years ago
- Owner changed from audrasjb to davidbaumwald
- Status changed from accepted to assigned
#9
follow-up:
↓ 10
@
4 years 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;
#10
in reply to:
↑ 9
@
4 years 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
#11
@
4 years 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
@
4 years 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.
#13
@
4 years 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 years ago
#15
@
4 years 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 years ago
#17
@
4 years 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
@
4 years ago
I re-tested the patch 52949.3.diff
solves the issue on my side.
Pinging @SergeyBiryukov for double check.
#19
@
4 years 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
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.