Fix missing assignment of menus on theme switch

[melchoyce]

Switching themes will cause menus to be "lost" in theme switch. ​See this post for examples.

[melchoyce]

Also related: #39693

[lukecavanagh]

@melchoyce 

So if a similar menu names exists on the new theme, then the memu connection would remain?

Would there be an option for mapping the existing menus names, to the new menu names in the new theme easily?

[melchoyce]

Replying to lukecavanagh:

@melchoyce 

So if a similar menu names exists on the new theme, then the memu connection would remain?

Would there be an option for mapping the existing menus names, to the new menu names in the new theme easily?

I think there's a couple things we could do:

1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
2. If your old theme and new theme share a similar IDs or names, map them automatically
3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

FWIW I hate menu locations :)

[lukecavanagh]

@melchoyce 

What would be a better name or term than menu locations then?

[melchoyce]

Replying to lukecavanagh:

@melchoyce 

What would be a better name or term than menu locations then?

I don't hate the name, I hate the entire concept. It brings an extra layer of complexity that stumps pretty much every new user I've ever seen work with WordPress, both in person and via user testing.

[folletto]

1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
2. If your old theme and new theme share a similar IDs or names, map them automatically
3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

This sounds accurate to me.

I don't hate the name, I hate the entire concept. It brings an extra layer of complexity that stumps pretty much every new user I've ever seen work with WordPress, both in person and via user testing.

Same. I think it's clear why it happened that way, but it's incredibly confusing for users.

[melchoyce]

Hey @jonathanbardo, I see you've been assigned to this ticket, along with #39693. Any idea when you'll have a chance to start working on some patches?

[jonathanbardo]

Hey @melchoyce,

Sorry for the late reply, I was on vacations in a country with no internet :( I should be able to look into this pretty soon. I think we should rework the original description to better reflect the issue because I don't think this is a regression rather than adding new UI in the customizer to deal with unassigned menu when doing theme switch.

[melchoyce]

Replying to jonathanbardo:

I think we should rework the original description to better reflect the issue because I don't think this is a regression rather than adding new UI in the customizer to deal with unassigned menu when doing theme switch.

Can you elaborate on this? I don't think this ticket will require any new UI, based on what I proposed in #4.

[jonathanbardo]

@melchoyce I'm a bit confused too since when testing changing themes, menus were assign automatically to a menu location if they shared the same id. Maybe I'm not testing correctly?

[melchoyce]

Replying to jonathanbardo:

@melchoyce I'm a bit confused too since when testing changing themes, menus were assign automatically to a menu location if they shared the same id. Maybe I'm not testing correctly?

Yup! The issue is when menus don't share the same IDs. We're trying to intelligently re-assign menus that don't match, using these criteria:

1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
2. If your old theme and new theme share a similar (not the same) IDs or name, map them automatically (so, primary, main, sidebar 1, etc. are all similar in concept)
3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

[jonathanbardo]

So it seems there is 2 components to this. One is to actually fix the switch_theme method and the other one is to be able to fix the customizer theme preview since it changes options at runtime and isn't calling switch_theme() until the user clicks save & activate.

[melchoyce]

Following up on this ticket after posting 39693#comment:20.

If your old theme and new theme share a similar (not the same) IDs or name, map them automatically (so, primary, main, menu 1, etc. are all similar in concept)

@ipstenu grabbed some data for us from the theme directory. ​Logs here.

Most common menu area names seem to be:

1. Main
2. Primary
3. Secondary
4. Footer
5. Navigation
6. Subsidiary

Using that info & some more research, I think we can do this:

• Main, Primary, and Navigation (along with any combination of these, like Main Navigation) can all be mapped to each other
• Secondary and Subsidiary can be mapped to each other
• Anything with "Footer" or "Bottom" in it can be mapped, if they don't match
• Anything with "Social" in it can be mapped, if they don't match
• Anything with "Top" in it can be mapped, if they don't match

This is in addition to:

1. If your old theme only has one menu and your new theme only has one menu, just map them automatically.
2. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

[obenland]

First pass at guessing menu locations

[obenland]

@melchoyce I added a patch with a first pass.

It maps menus when old and new themes only have one location or locations that are named the same, and also attempts to map locations that share one of the common terms you listed earlier.

I grouped them in primary, secondary, and social menu groups so they only map within that group. 'footer-menu' would map to 'secondary' but not 'main-menu' for example.

The mapping is pretty rudimentary currently, I'm sure that can be optimized. Let me know what you think?

[melchoyce]

Is there a way for this menu "memory" to persist across a couple theme changes? I noticed after I tried live previewing a couple themes in the Customizer, it stopped reassigning my menu.

[obenland]

Hm, I'm not sure how much of my patch actually applies to live preview since it's hooked into after_switch_theme. I'll have to look into that

[westonruter]

As with #39693, I've forked the slurper and have written a parser for obtaining the sidebar args for the themes: ​https://github.com/xwp/WordPress-Theme-Directory-Slurper/tree/master/feature-stats/registered-nav-menu-locations

Here are stats from the parsing: ​https://docs.google.com/spreadsheets/d/1QCormQoVGlI8rKxgn0ylxEw4JAa71372wdoJwgLvENs/edit#gid=0

The most common number of nav menu locations is 1, followed by 0 and then 2. The max a theme has is 9. Here are the stats:

Location Count	Theme Quantity
0	1312
1	1921
2	878
3	291
4	79
5	10
6	2
7	2
9	1

Plotted:

As for the most popular sidebar IDs used, here are the top 30:

Sidebar ID	Count
primary	2141
footer	302
social	295
secondary	249
main-menu	132
header-menu	109
footer-menu	94
top-menu	80
top	76
primary-menu	73
main	65
header	49
main-nav	45
main_menu	37
subsidiary	36
custom_menu	33
footer_menu	31
header_menu	30
Header	24
menu-1	24
main-navigation	23
footer-nav	23
mobile	23
top_menu	21
menu	18
main_nav	18
mainmenu	18
primary_nav	17
notfound	16
top-nav	15

There is a degree of normalization that can be done for these, such as replacing “main” with “primary”, removing hyphens, removing “navigation” and “menu” and so on.

[westonruter]

As for the menu groups, from this data I think that header should be included in the primary group.

Also, the string comparisons should be changed to be case-insensitive, and also to explicitly check for false as strpos() can return 0 if the haystack string begins with the needle string. So something like so:

if ( false !== stripos( $location, $slug ) || false !== stripos( $slug, $location ) ) {

Then we need to figure out how to apply these changes up-front when opening the customizer to preview theme switch. When the customizer is used for theme switching, we should presume that the nav menu locations will be assigned during the customization session. So as with starter content, the customizer should open with the nav menu location settings already dirty and populated with the remapped nav menus, and so they shouldn't be changed when the new theme then gets active.

Also, if a theme was previously active, then it will already have nav menu location assignments in its theme mods. In that case, I think this menu remapping switching logic should only be done if the target theme has no stored nav menu location assignments already.

[melchoyce]

Aside from adding support for live preview, what else needs to happen here to get something mergeable?

[obenland]

I think once #39692:25 was addressed we should be pretty close

[jbpaul17]

Punting to 4.8.1 per today's bug scrub.

[obenland]

In 39692.2.diff​:

• Added header to primary group.
• Hardened string comparisons to check for false and not be case sensitive.
• Maps menus on top of existing menu locations if theme was previously active.
• Added unit tests.

Unit tests helped quite a bit to uncover and fix odd behaviors and make the mapping much more reliable.

[joyously]

Looking at the last patch, I think there are two cases that are strange for theme switch, and apparently this is not handling Customizer previewing at all.

Case 1: the new theme has theme_mods already. Those should be used, untouched.
Case 2: the old theme has no menu selected. Nothing to do for new theme.
The old code (removed) handled these. The new code handles case 2, but not straightforward.

Also, the new function is called _wp_menus_changed(), but it is about changing themes, not menus. Perhaps a little more clarity in the name, so it can be called from both switch_theme() and whatever the Customizer equivalent is?

I also think the call to get the menu locations should be consistent. So in theme.php, instead of

$nav_menu_locations = get_theme_mod( 'nav_menu_locations' ); 

use the same call, like

$nav_menu_locations = get_nav_menu_locations(); 

And since we are talking about a switch, the temp option could be named more clearly. Instead of 'theme_switch_menu_locations', could it be 'theme_switch_old_menu_locations' or 'theme_switch_from_menu_locations'?

I wondered about language. Since Weston shows the most used menu slugs, and they are all English, all the other slugs that didn't make the top 30 list could include words in other languages along with less popular English words. I guess that doesn't matter.

Having looked at over 400 themes for theme review, I might classify the common slugs this way:

$common_slug_groups = array( 
  array( 'header', 'main', 'primary' ), 
  array( 'secondary', 'subsidiary', 'top', 'custom' ), 
  array( 'bottom', 'footer' ), 
); 

[obenland]

Replying to joyously:

apparently this is not handling Customizer previewing at all.

It's not handling it yet, ​like I said.

Case 1: the new theme has theme_mods already. Those should be used, untouched.

They are used, though not untouched. If a menu was changed since the theme was active last, we should account for that.

the new function is called _wp_menus_changed(), but it is about changing themes, not menus. Perhaps a little more clarity in the name, so it can be called from both switch_theme() and whatever the Customizer equivalent is?

That name is modeled after _wp_sidebars_changed(), but I'm open to suggestions. The meat of the function will probably have to be separated out anyway to accommodate the customizer.

[jbpaul17]

Punting to 4.9 per today's bug scrub.

[melchoyce]

Is anyone else available to review @obenland's patch?

[westonruter]

I've put the patches into a pull request which we can use for review and collaboration going forward: ​https://github.com/xwp/wordpress-develop/pull/245

[welcher]

I reviewed the patch and just have a couple of comments.

1. I think we should do the check for ! empty( $old_nav_menu_locations ) right away and only do the processing if so. It's a minor thing, but why do any processing if there is nothing to convert from.
2. The in_array check is not doing a strict check. Technically, nav menus can be registered with a numeric location such as array( 1 => 'First Menu', 2 => 'Second Menu', ) which will produce a false positive and in turn, an undefined offset error.

I have applied the patch and am getting a failure on one of the unit tests:

1) Tests_Nav_Menu_Theme_Change::test_one_location_each

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
+    0 => 1
 )

I've attached a patch with the proposed changes and a new test that demonstrates the in_array error if not using strict.

[westonruter]

@obenland I'm creating a suite of child themes for testing and debugging to confirm that the theme switching is resulting in nav menus being assigned to the new locations as expected: ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations

Right now there is one test for a theme that has nav menu locations (primary, secondary) and another theme that has nav menu locations (top, footer) and it checks to confirm that the menus assigned to locations in the former theme get re-assigned to the locations in the latter theme. So far, so good! ✅ The ​tests pass.

These tests are a level above the unit tests in 39692.3.diff​ (which are still needed) and more in the realm of acceptance testing, which is really going to be the only feasible way to test menu switching in the Customizer.

[obenland]

Whoa, nice! Thanks for that!

[joyously]

I still think the groups are not right being

$common_slug_groups = array( 
   array( 'header', 'main', 'navigation', 'primary', 'top' ), 
   array( 'bottom', 'footer', 'secondary', 'subsidiary' ), 
   array( 'social' ), 
   // TODO: Find a second slug or remove, since locations with same slug are already mapped. 
   );

The themes I have seen will typically have two or more menus at the top of the page and perhaps one at the bottom. So, like I said before, this makes more sense: (top is typically right below the admin bar and not the main menu)

$common_slug_groups = array( 
  array( 'header', 'main', 'primary' ), 
  array( 'secondary', 'subsidiary', 'top', 'custom' ), 
  array( 'bottom', 'footer' ), 
); 

[westonruter]

I'm working on fixing the tests and breaking out the _wp_menus_changed() logic into a wp_get_remapped_nav_menu_locations() function which can then be re-used for the Customizer.

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/245/files/5445156..4054096

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/245/files/4054096e89df767533d3c6a2047f3a44ddda4321..efa8f5e3b8d436af09d0d04777608f59c35e27e6?w=1

[westonruter]

Unit tests are ​fixed now as of 39692.5.diff​

Next up: Customizer integration

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/245/commits/48a67c0bf0410cae15686b4ff1216c3daa2e2301

[westonruter]

And 39692.6.diff​ implements Customizer support for remapping nav menus when theme switching.

Please test.

I'd like to flesh out some more unit tests, like for these Customizer changes.

I'd also like to add Customizer acceptance tests to accompany the non-Customizer acceptance tests in ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations/tree/master/tests/nav-menus

[joyously]

Would it be possible to test it on the theme preview site?

[melchoyce]

Replying to joyously:

Would it be possible to test it on the theme preview site?

@otto42, do you know if this is possible?

[Otto42]

Replying to melchoyce:

@otto42, do you know if this is possible?

The theme previewer is an external tied to trunk. Disconnecting that would be possible, but likely to break things in trying to put it back together later. Help from systems would be needed.

[obenland]

Nice work @westonruter, tests out well. I assumed it would be much more involved to add Customizer support.

Can we change wp_get_remapped_nav_menu_locations() to simply wp_map_nav_menu_locations()? And maybe any variable that says remapped to just mapped?

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/245/files/48a67c0..b2d5c67

[westonruter]

@obenland ok, 39692.7.diff​ makes those changes. I've updated the unit tests to make use of the new function as oppose to using the private function and manipulating theme mods and options.

I've also added updated the test runner to test more ​scenarios, using three methods to switch themes: WP-CLI, themes admin screen, and the Customizer: ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations/blob/master/tests/nav-menus/runner.php

Using headless Chrome to script the activation of a theme via the Customizer and admin screen:

• ​switch-theme-via-admin.js
• ​switch-theme-via-customizer.js

This is good for to you commit from my view, though per above there may be some tweaks to the group slugs.

[westonruter]

FYI: The Travis build for the branch is ✅: ​https://travis-ci.org/xwp/wordpress-develop/builds/262099992

[obenland]

In 41237:

Map nav menu locations on theme switch

This will send nav menu locations through three levels of mapping:

1. If both themes have only one location, that gets mapped.
2. If both themes have locations with the same slug, they get mapped.
3. Locations that (even partially) match slugs from a similar kind of menu location will get mapped.

Menu locations are mapped for Live Previews in the Customizer and during theme switches.

Props westonruter, obenland, welcher, melchoyce.
Fixes #39692.

[obenland]

In 41801:

Menus: Add social as tertiary mapping group

Allows mapping of all social menus that include the term social in their slug.

See #39692.

[obenland]

In 41810:

Customizer: Account for legacy menu data

Checks for menu location data from when a previewed theme was previously active.

See #39692.

[obenland]

In 41995:

Customize: Allow previewed menus to be customized

Fixes a bug where menu assignements couldn't be changed when previewing a theme.
Also removes an unnecessary call to menu mapping after a theme switch from the customizer and makes sure the locations option is always written, for good measure.

Props westonruter.
See #39692.

[obenland]

In 42026:

Menus: Add menu-$i slugs to mapping groups

Helps to future proof the feature.
Also orders slugs by popularity to optimize mapping time.

See #39692.