WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 8 weeks ago

#42121 closed defect (bug) (fixed)

Menus: Filter orphaned locations on theme switch

Reported by: obenland Owned by: obenland
Milestone: 4.9 Priority: high
Severity: normal Version: 4.9
Component: Menus Keywords: needs-testing has-patch has-screenshots
Focuses: administration Cc:

Description

With menu mapping in place (#39692), it is possible to get unexpected mapping results after nav menu locations have been removed. If the nav_menu_locations theme mod still contains data for that removed location, and the slug happens to be part of the group of slugs that are being mapped, it is possible to have that outdated value mapped to a new menu location when switching themes.

I first noticed it when debugging the menu preview in the customizer, which in its call to wp_map_nav_menu_locations() would always return the menu locations of both old and new themes.

Attachments (2)

42121.diff (2.6 KB) - added by obenland 2 months ago.
42121.2.diff (2.7 KB) - added by obenland 2 months ago.

Download all attachments as: .zip

Change History (19)

@obenland
2 months ago

#1 @obenland
2 months ago

  • Keywords has-patch added

@afercia Would you mind testing menu mappings again with 42121.diff?

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


2 months ago

#4 @obenland
2 months ago

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

#5 @afercia
2 months ago

  • Keywords has-screenshots added

@obenland this was what I got during our testing on Slack:

https://cldup.com/KttOlKbYCo.png

with the patch applied there's a difference in what get_nav_menu_locations() returns (just 2 locations instead of 3) but the fallback menu is still there for me:

https://cldup.com/Ooyb1aNMAC.png

#6 @obenland
2 months ago

Right, with the patch it will now only return the menu locations that are registered. I don't know what the issue is with the fallback menu, but it seems unrelated to menu mapping.

@obenland
2 months ago

#7 @obenland
2 months ago

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

In 41811:

Menus: Limit mapping to registered locations

Weeds out orphaned locations, so their information will not continue to be mapped to future themes.

Fixes #42121.

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


2 months ago

#9 @afercia
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Agreed with @westonruter to reopen for investigation on the edge case illustrated in the screenshots above, see also conversation on Slack here: https://wordpress.slack.com/archives/C0381N237/p1507235780000471

#10 @westonruter
2 months ago

  • Priority changed from normal to high

#11 @obenland
2 months ago

@afercia Could you be a bit more specific as to what I should be testing for? Switching back and forth between 2016 and 2017 with empty menu locations doesn't produce the results you described in Slack. Both menu locations remain empty, no theme displays a fallback menu.

It would be great if you could provide steps to reproduce this from a state where theme_mods for both themes have been removed.

@westonruter Have you been able to reproduce?

#12 follow-up: @afercia
2 months ago

@obenland I guess @westonruter wanted to reopen for investigation exactly for the reason that seems there's an edge case that it's difficult to reproduce. I have been able to reproduce it again randomly switching themes, even to 2013 back and forth. I can provide a dump of my local environment database if needed.

#13 in reply to: ↑ 12 @obenland
2 months ago

Replying to afercia:

I have been able to reproduce it again

What is it in this case? Assigned menu locations when there weren't any or falling back to default menu when it shouldn't?
As much as I'd love to help, I'm not sure how really without knowing what to test for. Randomly switching themes hasn't done anything for me since [41811].

#14 @obenland
8 weeks ago

@afercia Any update here or can this be closed?

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


8 weeks ago

#16 @afercia
8 weeks ago

@obenland I've tested again and wasn't able to reproduce the issue any longer. That doesn't necessarily means we've clearly identified the root cause :)

#17 @obenland
8 weeks ago

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

Let's close it out and open new tickets for reproducible bugs.

Note: See TracTickets for help on using tickets.