Fix missing assignment of widgets on theme switch

[melchoyce]

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

Related: #19912, which focuses on one specific solution — I'd like to see us step back from that and think about other ways to approach this problem.

Related: #39692, for the same problem with nav menus.

Related: #27404, Widget Customizer: Allow adding inactive widgets to widget areas

[melchoyce]

Also related: #39692

[westonruter]

If we don't use the concept of a widget group (#19912), what's an alternative to what core currently does in trying to guess a sidebar to re-assign the widgets to? Add a user step to ask where they'd like widgets (and nav menus) to appear as an interstitial when doing a theme switch? That doesn't seem ideal either.

[SergeyBiryukov]

Switching themes will cause widgets to be "lost" in theme switch.

This was previously fixed in #17979 for 3.3, sounds like there was a regression somewhere.

[melchoyce]

Replying to westonruter:

If we don't use the concept of a widget group (#19912), what's an alternative to what core currently does in trying to guess a sidebar to re-assign the widgets to? Add a user step to ask where they'd like widgets (and nav menus) to appear as an interstitial when doing a theme switch? That doesn't seem ideal either.

Some ideas:

1. Throw all the widgets into whatever widget area there is without asking (and additionally make some design enhancements to make switching widget area easier)
2. Look for similar widget area IDs/names to map to

It would also help to introduce the inactive widgets area into the Customizer.

[melchoyce]

Replying to SergeyBiryukov:

Switching themes will cause widgets to be "lost" in theme switch.

This was previously fixed in #17979 for 3.3, sounds like there was a regression somewhere.

If you go from many widget areas down to one widget area, you still "lose" widgets (in that they're set inactive, which, if you're working in the Customizer, doesn't exist). This ticket probably needs a better description.

[westonruter]

To be worked on in conjunction with #27404.

[celloexpressions]

It seems like #19912 is still the best long term approach here. Wherever possible, widget and menu assignment should be automatic when switching themes. However, providing user control over menus/widget groups that can be assigned to locations is the only clean way to allow users to quickly restore their desired configurations when automatic assignment fails. The UX of menu locations could certainly use improvement, but unifying widgets with menus is a good first step.

This ticket would be a good place to explore additional ways to automate widget assignments, both with and without the concept of widget groups. The widget groups concept is very powerful in that it will facilitate separating the majority of widget information from specific instances on specific themes - like with menus, actual widgets could be more logically stored in posts with only the widget group being stored in a theme-specific option (theme_mod). In situations where a site may switch between different themes seasonally or for other reasons (even, say, going back and forth with child themes), changes made to a widget in one theme don't sync with similar widget instances in other themes currently.

[folletto]

It seems like #19912 is still the best long term approach here.

I back @westonruter's comment as that ticket being more about the technical consolidation as separate from the UI, as menu locations is one of the most confusing interface concepts ever. I tried to explore alternatives on .com, but it didn't work very well. Unless we have a solid solution, I wouldn't introduce UI for it.

Regardless, I agree with you:

This ticket would be a good place to explore additional ways to automate widget assignments, both with and without the concept of widget groups.

As re-assignment is a big win regardless of the decision on the other ticket, I think groups could play into this as a way to achieve solid switching.

I'd use a logic similar as Mel outlined in #39692:

If your old theme only has one menu and your new theme only has one menu, just map them automatically
If your old theme and new theme share a similar IDs or names, map them automatically
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.

Another way to say it: always associate the maximum number of existing items to the most likely location.
Since here we deal with individual widget and not menus, it's probably going to be trickier.

The extra considerations I'll do for both are:

1. We need to make sure that when a user switches back to a previous theme the items relocate to exactly the same location. I'm not sure how this can be done, but it's very important as it's expected.
2. In case of a switch to a theme with less locations, there should be a good default so ideally the user has as little as possible extra work to do.
3. Previous widget configurations should be able to be retained in a way to be explored.

We need to explore how to make "dropped" widgets accessible to not make people lose the configuration they made.

One idea: as a different theme might have different areas, an idea would be to introduce a section inside the "Add widget" panel that lists previous configurations. In that way one can "Add" a pre-configured widget (or delete that pre-configuration), without introducing an additional concept of "an inactive widget area".

Incidentally providing an API to pre-configured widget could be a feature for multi-site installation too where there are some pre-configured templates provided at network level. But this escalates complexity, I'm throwing it in just as a potential vision.

[melchoyce]

Replying to folletto:

The extra considerations I'll do for both are:

1. We need to make sure that when a user switches back to a previous theme the items relocate to exactly the same location. I'm not sure how this can be done, but it's very important as it's expected.
2. In case of a switch to a theme with less locations, there should be a good default so ideally the user has as little as possible extra work to do.
3. Previous widget configurations should be able to be retained in a way to be explored.

Agreed 👍

We need to explore how to make "dropped" widgets accessible to not make people lose the configuration they made.

One idea: as a different theme might have different areas, an idea would be to introduce a section inside the "Add widget" panel that lists previous configurations. In that way one can "Add" a pre-configured widget (or delete that pre-configuration), without introducing an additional concept of "an inactive widget area".

Any time or interest in exploring a design for this?

[westonruter]

Comment on #27404 (Widget Customizer: Allow adding inactive widgets to widget areas):

I'm going to suggest that widgets most commonly get moved to inactive widget area as the result of a theme switch (#39693). It would be very useful if widgets that were made inactive as part of a theme switch could be grouped in a way to easily batch-add them to a sidebar in the new theme. By grouping them I don't mean widget persistent groups (#19912), but rather transient groupings that essentially are pulling the grouping of widgets from another theme's widget-sidebar assignments. Being able to easily and intuitively re-assign widgets from the previous theme's sidebars to the new theme's sidebars, maintaining their ordering, would be a huge.

If the old theme has 3 sidebars whereas the new theme as 2 sidebars, note that this could mean that 2 groupings of inactive widgets could be added to one sidebar on the new theme. Two additional questions then come to mind: should the best-guess re-assignment of widgets from the old sidebar's theme to a sidebar in the new theme be eliminated unless the sidebar IDs are exact matches (e.g. sidebar-1)? If not, then it is likely that a re-assignment of widgets in the new theme will be wrong and need to be re-assigned. In that case, there should have to be a way to bulk move all widgets from one sidebar to another.

It may be that this theme-switch grouping is out of scope for this ticket and should instead be addressed as part of #39693.

[folletto]

Customizer widgets "Presets" area i2

[folletto]

Ok I uploaded a concept on how we could show the "inactive" widgets in a way that is fairly seamless. The goal here is exclusively to provide access to widgets that were dropped during a theme switch (or any other reason) with the additional restriction of having minimal impact.

Features of the "Presets" i2 concept:

1. Inactive widgets have a special separated area in the "Add a widget" panel.
2. They are searchable as the other widgets, content included if possible
3. They can be removed, and the removal has an extra trick here: one can position the mouse in the top-right X and keep clicking, removing all the inactive widgets easily in sequence (we probably need a form of undo for the ideal experience).
4. The second line of the widget instead of showing the details shows when they were removed, thus giving a simple indication of how old a widget is.
5. I avoided subtitle labels for the sections, but we can introduce them if we want. The current state might still work well enough.

That's all. It's an approach "low key" enough to be effective, but at the same time very visible as it's inside the main "Add" flow: a user that "lost" a widget will get there, even just because they have to re-add them.

[lukecavanagh]

@folletto

How would this look in WP Admin?

[folletto]

I wasn't considering any change to the wp-admin interface as it already shows an "Inactive widgets" area. Maybe I'm missing something? :)

[melchoyce]

My only concern is, does it become unwieldy if you had a bunch of widgets? But I think we can figure that out via testing, and with better widget switching in #39693, that might not even be a big issue. Let's try it.

Worth including what theme you were using it with, or nah?

Edit: this is #39693. 😑 For some reason I thought we were in #27404. Want to move the mockup over to that ticket, @folletto?

[folletto]

I didn't know there was a more specific ticket. So this is just about the re-assignment logic the other instead is about what happens when the reassignment logic can't find a spot? Ok.

Cross posted there.

[edge22]

#40009 was marked as a duplicate.

[melchoyce]

If your old theme and new theme share a similar IDs or names, map them automatically

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

Most common widget area names seem to be:

1. Sidebar (with Sidebar1, Sidebar2, etc being right down the line)
2. Main (with a Main Sidebar being a very popular second)
3. [Left|Right] Sidebar

Bottom, footer, header, and custom were clustered together in the list of widget areas. Many more are "Footer [something]."

Surprisingly, the data seemed to indicate that Left Sidebar was more common than Right Sidebar. I don't know if we should map Left to Sidebar, etc., and Right with Sidebar2, based on that, or go with my gut and do Sidebar --> Right, Sidebar 2 --> Left. Let's try the latter option and then test a bunch of themes.

I think, using this info and some research of my own, we could assume:

• Sidebar, Sidebar 1, Main, Main Sidebar, Primary, Primary Sidebar, Right Sidebar, and First can all be mapped to each other
• Sidebar 2, Left Sidebar, and Second can be mapped to each other
• Footer and Bottom can be mapped to each other
• Header and Top can be mapped to each other

And if always, if there's an exact match, it should map to that widget area. Or, if your old theme only has one widget area and your new theme only has one widget area, just map them automatically.

[westonruter]

Given the data gathered from themes, perhaps we can use this as an opportunity to codify recommendations in the theme handbook for how sidebars should be named? If a theme author knows that the way they name their sidebars is important, they can re-use the sidebar ids that are recognized in core for mapping and easy the switching into their theme.

[westonruter]

A very helpful way for hosts to provide support on this issue would be to share some statistics on the widget sidebars for the sites the host. WordPress stores the widget configurations for the sidebars in a PHP-serialized sidebars_widgets option in the DB that looks like:

<?php
array(
    'sidebar-1' => array(
        'search-2',
        'meta-2',
    ),
    'sidebar-2' => array(
        'archives-5',
        'calendar-10',
    ),
    'wp_inactive_widgets' => array(
        'text-3',
        'recent-comments-3',
    )
)

A host could run a script that iterates over each WP site, grabs out the sidebars_widgets option, and then collect stats on:

1. Logs the sidebar IDs for the site (the array keys)
2. Counts the number of widgets in each sidebar (the count for each nested array).

Then given that a script has done:

<?php
$sidebars_widgets = get_option( 'sidebars_widgets', array() );
if ( empty( $sidebars_widgets['array_version'] ) || 3 !== $sidebars_widgets['array_version'] ) {
    return;
}
unset( $sidebars_widgets['array_version'] );

Then, if we can get stats on:

1. The most commonly used sidebar IDs (array_keys( $sidebars_widgets ))
2. The number of sidebars registered (count( $sidebars_widgets ))
3. Counts the number of widgets in each sidebar (array_map( 'count', $sidebars_widgets ) )) and perhaps a sum of them (call_user_func_array( 'array_sum', array_map( 'count', $sidebars_widgets ) ) )).

The wp_inactive_widgets sidebar is also a special case which will help with designing #27404.

[Ipstenu]

Oh I see why you asked me about DH.

A host could run a script that iterates over each WP site, grabs out the sidebars_widgets option, and then collect stats on:

Sure we could. But that runs into some SERIOUS privacy concerns, and my knee-jerk reaction is "I am absolutely not giving you customer data like that."

To explain why the search from the theme slurper is kind of junk, it's basically that people format code in different ways.

Example:

themes/aaron/functions.php:46:          register_nav_menus( array(
themes/abacus/functions.php:77:         register_nav_menu( 'top', __( 'Top Menu', 'abacus' ) );

And if I hop into the aaron theme:

                register_nav_menus( array(
                        'header' => __( 'Primary Menu', 'aaron' ),
                        'social' => __( 'Social Menu', 'aaron' ),
                ) );

Scanning for both of those can be a bit tricky. Getting all that data and outputting it into something useful and readable is doubly so.

Two options for you to download and search for it yourself though:

​https://github.com/aaronjorbin/WordPress-Theme-Directory-Slurper
​https://github.com/Ipstenu/WordPress-Theme-Directory-Slurper

Mine's a fork of the current plugin slurper.

[fjarrett]

@Ipstenu Correct me if I'm misreading what @westonruter is asking for, but such stats would be delivered in aggregate and not personally-identifiable. I think it's important for hosts to try to provide anonymous/aggregated trend data back to the community when they can, similar to ​https://www.godaddy.com/garage/wordpress-hot-100/

[Ipstenu]

Listing what plugins are installed is somewhat of a different order of magnitude. And while I laud GoDaddy for doing that, I personally find it right on the edge of things where I'm violating privileged information. Like disclosing how many users, on average, people have on a site is different. @mikeschroder may have a different opinion of course, but like I said, I'd have to ask and my (PERSONAL) knee-jerk reaction is "No."

If all you care about is the themes hosted on .org, then someone can grab the slurper and figure out a better search than I did. The data is right there for y'all to search and you don't need me for that. I happened to have some of it handy when Mel asked is all.

[westonruter]

Replying to Ipstenu:

If all you care about is the themes hosted on .org, then someone can grab the slurper and figure out a better search than I did. The data is right there for y'all to search and you don't need me for that. I happened to have some of it handy when Mel asked is all.

Thanks, I'll try to figure out something to grab the sidebar IDs. And thanks for doing that initial getting the initial rundown of the sidebar Names. That gave us the initial direction we needed.

[westonruter]

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-widget-sidebars

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

The most common number of sidebars is 1. Half as common is to have 2, with a little bit less common to have 0 sidebars. Here are the top 10 sidebar counts:

Sidebar Count	Theme Quantity
1	1308
2	748
0	612
4	444
5	377
3	365
6	236
7	129
8	90

Plotted:

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

Sidebar ID	Count
sidebar-1	2234
sidebar-2	810
sidebar-3	416
sidebar-4	299
sidebar	234
footer-1	233
footer-2	224
footer-3	218
sidebar-5	203
primary-widget-area	162
footer-4	134
first-footer-widget-area	132
second-footer-widget-area	131
third-footer-widget-area	124
sidebar-6	120
fourth-footer-widget-area	97
secondary-widget-area	96
right-sidebar	80
sidebar-widget-area	76
footer	70
footer-widget-area	70
primary-sidebar	69
sidebar-left	68
footer-right	66
footer-left	65
sidebar-right	60
sidebar-7	56
footer-sidebar	52
primary	51

There is a degree of normalization that can be done for these, such as replacing “fourth” with 4, removing hyphens, removing “sidebar” and “widget area” and so on.

[westonruter]

The function that will be the focus of the work here is retrieve_widgets(): ​https://developer.wordpress.org/reference/functions/retrieve_widgets/

[alexvorn2]

My idea is to store widgets settings into theme_mod_[theme_name] option, see my ticket: https://core.trac.wordpress.org/ticket/40067

[westonruter]

Replying to alexvorn2:

My idea is to store widgets settings into theme_mod_[theme_name] option, see my ticket: https://core.trac.wordpress.org/ticket/40067

Beyond what I commented on that ticket, I don't see how this will help the situation. It seems like your proposing creating theme-specific sets of widgets, where each theme has its own parallel universe of widgets. The problem were are trying to solve here is how to teleport widgets between the theme multiverse, not introduce the need to maintain a multiverse of widget doppelgangers. 😉

[alexvorn2]

Replying to westonruter:

Beyond what I commented on that ticket, I don't see how this will help the situation. It seems like your proposing creating theme-specific sets of widgets, where each theme has its own parallel universe of widgets. The problem were are trying to solve here is how to teleport widgets between the theme multiverse, not introduce the need to maintain a multiverse of widget doppelgangers.

If you look in the recent default theme: "Twenty Seventeen" you can find out that this theme have theme-specific sets of widgets settings, so this is not something new, you can check by yourself when you make a theme preview of this theme... a you find that some widgets are loaded with text in the specific sidebars.

I, of course like this idea and I am looking further in this direction as it need a lot of improvements.

[westonruter]

Replying to alexvorn2:

If you look in the recent default theme: "Twenty Seventeen" you can find out that this theme have theme-specific sets of widgets settings, so this is not something new, you can check by yourself when you make a theme preview of this theme... a you find that some widgets are loaded with text in the specific sidebars.

These starter content widgets are only applied to Twenty Seventeen in the case of a site being fresh, without any content on it. These are not intended to be used when a site already has content. They should not overwrite existing widgets that someone may already have in another theme when switching to Twenty Seventeen. Any re-use of the starter content widgets would have to be opt-in, per #38624.

[westonruter]

I've created a new repo to contain a parent theme and eventually child themes that exhibit all of the test scenarios: ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations

[jbpaul17]

Punting to 4.8.1 per today's bug scrub.

[westonruter]

See also #40177.

[westonruter]

#19846 was marked as a duplicate.

[westonruter]

See also #19846 which suggested that an is_primary arg be added to registered sidebars. I think the ideas being explored here in this ticket are better because they account for multiple widget areas, not just the primary one.

[westonruter]

@melchoyce If we improve the widget-sidebar assignments on theme switch, I think it is important to note that it would only improve the situation when switching to a brand new theme that hasn't been active before. Once a theme is active, it gets its own copy of which widgets are in which sidebars (stored in the sidebars_widgets theme mod, and used in retrieve_widgets()).

Improving the logic for switching will not improve the user experience when switching to a theme that was previously active. If I was on theme A and had widgets 1,2,3 in a sidebar “Foo” and then I switch to a new theme B, where widgets 1,2,3 get added to “Bar”, and I add widget 4 to that sidebar as well… if I switch back to theme A then widget 4 will be be absent from “Foo” in the re-activated theme. I can imagine this being really confusing for users, and really confounding for users who try to preview a theme switch via the Customizer. The only way I can think of to permanently that issue is to implement widget groups (#19912), but that then introduces another layer of abstraction which I understand confuses users in the nav menus system.

All of this to say, it feels like delivering this without also implementing #27404 (Allow adding inactive widgets to widget areas) will be an incomplete solution.

[alexvorn2]

Replying to westonruter:
Maybe you would like to reconsider my idea - https://core.trac.wordpress.org/ticket/40067
I don't see other straight solution to the widgets problem.

[westonruter]

Replying to alexvorn2:

Maybe you would like to reconsider my idea - https://core.trac.wordpress.org/ticket/40067
I don't see other straight solution to the widgets problem.

Thanks, but I don't think so. As I just noted there now, requiring a user to re-create all of their widgets every time they switch to a new theme just isn't going to go over well.

[alexvorn2]

Replying to westonruter:

Replying to alexvorn2:
Thanks, but I don't think so. As I just noted there now, requiring a user to re-create all of their widgets every time they switch to a new theme just isn't going to go over well.

When switching to a new theme we can insert default widgets with their setting OR to take from the previous theme. So the user will not be required to re-create all of their widgets.

[westonruter]

Replying to alexvorn2:

When switching to a new theme we can insert default widgets with their setting OR to take from the previous theme. So the user will not be required to re-create all of their widgets.

Taking widgets from the previous theme is the problem being addressed in this ticket. If there is to be an option (and UI) to reset widgets to a theme's defaults, then this should be tackled in #38624.

[alexvorn2]

Replying to westonruter:

Replying to alexvorn2:

When switching to a new theme we can insert default widgets with their setting OR to take from the previous theme. So the user will not be required to re-create all of their widgets.

Taking widgets from the previous theme is the problem being addressed in this ticket. If there is to be an option (and UI) to reset widgets to a theme's defaults, then this should be tackled in #38624.

Yeah but everything (in my idea) is different compared to what is now and will make everything less confusing.

The new approach I would like to see like this:

1. If you switch to new theme for the first time - you will get widgets from previous theme.
2. If you switch back to the old theme - you get old widgets from that theme not from this new theme

So it is not the same.

Now what we have and it makes everyone confusing and frustrated:

1. If you change/edit/remove widgets and you switch back to the old theme - you can not get the old widgets for that old theme.
2. After a theme switch you get all other theme widgets in the "Inactive Widgets" section in the widgets.php page and if you remove them you will not have them anymore if you switch back to the old theme.

[obenland]

I wrote tests for the existing version of retreive_widgets(), in an effort to understand the function better and hopefully maintain backwards compatibility with future changes. Not sure if I achieved 100% code coverage, but it should be pretty close.

It seems like the function has four(!) different contexts from which it's called from and behaves differently based on that context. I'll dive a bit more into the history of that function to get a better understanding, but for now the tests should at least give a little better footing to build on.

[obenland]

@azaozz @melchoyce @westonruter What do you think about moving away from the concept of orphaned widgets, and just adding any orphaned ones to the inactive sidebar?

[obenland]

Added a first pass in 39693.diff​. It still needs some more unit tests around the mapping part.

[alexvorn2]

Replying to obenland:

Added a first pass in 39693.diff​. It still needs some more unit tests around the mapping part.

Thanks for the patch, I think my other ticket is related to this ticket... #40177
Maybe the patch will fix both tickets.

[obenland]

I just tested it and it doesn't look like it fixed $40177. I'll try to write a test case for it and see if I can cover that too.

[obenland]

@alexvorn2 Could you test with 39693.2.diff​ and see if you can still replicate #40177?

[alexvorn2]

Replying to obenland:

@alexvorn2 Could you test with 39693.2.diff​ and see if you can still replicate #40177?

I get some warnings after theme install:

Warning: array_merge() expects at least 1 parameter, 0 given in D:\Dropbox\htdocs\wordpresslastmu\wp-includes\widgets.php on line 1149

Warning: array_diff(): Argument #2 is not an array in D:\Dropbox\htdocs\wordpresslastmu\wp-includes\widgets.php on line 1150

Warning: Invalid argument supplied for foreach() in D:\Dropbox\htdocs\wordpresslastmu\wp-includes\widgets.php on line 1152

Warning: array_merge(): Argument #1 is not an array in D:\Dropbox\htdocs\wordpresslastmu\wp-includes\widgets.php on line 1159

[alexvorn2]

Strange the warning come from nowhere...

Maybe I patched twice...

Can't replicate the warnings, today first time patched a file, for WordPress.

I tested twice and it seems to work nice for me. So after making all steps in the #40177 ticked all file changes seems to work and I get the right widget in the sidebar I was looking for.

[obenland]

Replying to alexvorn2:

I get some warnings after theme install:

Hm, I hoped I had those fixed. You said you can't reproduce them anymore?

[alexvorn2]

Replying to obenland:

Replying to alexvorn2:

I get some warnings after theme install:

Hm, I hoped I had those fixed. You said you can't reproduce them anymore?

I can! It seems when there a not widgets in any sidebars - warnings occur.
How to replicate:

1. Remove all widgets in every theme.
2. Activate another theme.

[alexvorn2]

Replying to obenland:
Can you please tell me what you think about my idea that was closed by westonruter: #40067

[obenland]

Thanks! I'll have it fixed in the next version of the patch

[obenland]

Updated tests for original retrieve_widgets().

[alexvorn2]

Thanks for update.

I tested and what I can say it works better than it was before when after a theme switch all widgets move to Inactive Widgets sidebar.

Does it mean we can get rid out of it? I have a ticket for this too and it was closed again by westonruter in #40219 .

[obenland]

Replying to alexvorn2:

Does it mean we can get rid out of it?

I don't think so. I feel like there should be a place for widgets that have active settings but are currently not shown.

[alexvorn2]

Replying to obenland:

Replying to alexvorn2:

Does it mean we can get rid out of it?

I don't think so. I feel like there should be a place for widgets that have active settings but are currently not shown.

Yeah! but how will these widgets show in the Inactive Widgets sidebar now with this new patch?

Or you mean the widgets that are currently in the Inactive Widgets widget area so after WordPress update they will show there..., but after a theme switch they will not be there anymore, right?

[obenland]

There is still a possibility for widgets to end up there if we can't match one sidebar up to another, it's just less likely. Also users might decide to "park" a widget there, regardless of a theme switch.

[obenland]

In 41266:

Widgets: Add tests for retrieve_widgets().

Helps with maintaining back compat when making changes in the future.

See #39693.

[alexvorn2]

OK, then I will close my ticket - #40177.

[alexvorn2]

#40177 was marked as a duplicate.

[timmydcrawford]

Tested out 39693.3.diff​ with a switch from 2017 -> 2014 and back. Sidebar on 2017 mapped to Primary on 2014, which felt absolutely right. Footer 1 mapped to Content Sidebar, which seemed a bit interesting because Footer 2 was subsequently mapped to Footer on the new theme. But, all widgets were carried over which was fab.

Switching back to 2017 also led to all widget areas being restored to their prior spots.

Moving to 2012 theme, which has the one sidebar, registered the logical 'Sidebar' from 2017. Switching back again restored all previous widgets, even the ones un-used in 2012 ( footer1/2 ).

In the code, a few items:

In _wp_map_sidebars() should we be validating that the value of $old_sidebars_widgets passed in is an array before iterating over it in widgets.php#L1186?

On line 994 in the tests, this line struck me as a bit odd $expected_sidebars = $prev_theme_sidebars; any reason to not just assert against $prev_theme_sidebars?

[obenland]

Thanks for testing @timmydcrawford! I'll update the patch to reflect your recommendations

[melchoyce]

Apparently a duplicate of #17979, but we're going to keep this open since that one's super old. :)

[alexvorn2]

Replying to melchoyce:

Apparently a duplicate of #17979, but we're going to keep this open since that one's super old. :)

As i heard it was fixed once, but the problem occurred again, so it is another fix.

[azaozz]

Tested 39693.4.diff. All seems to be working well, just like in comment 75. After switching back and forth through several themes most widgets still go to appropriate sidebars.

The quintuple nested loop looks a bit scary, but all arrays are quite short so it doesn't slow down.

[westonruter]

Fix minor phpcs issues: ​https://github.com/xwp/wordpress-develop/pull/251/commits/206f97d

[westonruter]

Before committing this, I want to add tests for theme switching with widgets as I did for nav menus in ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations

This will make sure we can have a reliable way to test all the scenarios and check for regressions going forward.

[obenland]

Are these tests related to the patch that goes in? It doesn't seem like they run from within /tests?

[westonruter]

No, they won't be part of the patch. They'll just be double confirmation that the patch works across the various scenarios.

[westonruter]

I've found that the mapping logic isn't working when activating a theme via WP-CLI. Looking into why.

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/251/files/206f97d..2abe308

[westonruter]

In 39693.6.diff​:

• Figured out why the mapping logic didn't appear to be working with WP-CLI. It actually just appeared to not work for the initial time you visit the frontend, but then after reloading the widgets would be in the sidebar as expected. The issue is just that the global variable containing the sidebars widgets wasn't getting reset after the sidebars widgets were updated. See ​0cf8d0c.
• I was testing with a theme that had sidebars sidebar-1 and sidebar-2 and then another theme that had sidebars primary and footer. I was expecting when switching that they would get mapped correspondingly, but they were not. So I included sidebar-1 and sidebar-2 in the primary and footer groups respectively. See ​2abe308.
• Renamed _wp_map_sidebars() to wp_map_sidebars_widgets() for better parity with wp_map_nav_menu_locations().

Some outstanding things that I see that need to be addressed:

• The lost widgets logic appears to be blowing a way old single widgets that lack widget numbers. I think ​that logic could probably be removed?
• I was working on ​scenarios for testing the various sidebar ​theme permutations, and I found a ​scenario where from a theme with 3 populated sidebars to a theme with 2 sidebars and then switching back to the theme with 3 sidebars results in the one sidebar that was not initially mapped being empty after having switched back. This seems bad to me. I think this is part of the orphaned sidebar question.

PR where I'm reviewing the patch and amending the patch and running Travis tests: ​https://github.com/xwp/wordpress-develop/pull/251

[alexvorn2]

Replying to westonruter:

In 39693.6.diff​:

• I was working on ​scenarios for testing the various sidebar ​theme permutations, and I found a ​scenario where from a theme with 3 populated sidebars to a theme with 2 sidebars and then switching back to the theme with 3 sidebars results in the one sidebar that was not initially mapped being empty after having switched back. This seems bad to me. I think this is part of the orphaned sidebar question.

Some widgets show in a Orphaned Sidebars because the number of sidebars in the current theme is fewer than in the old theme, so the solution I think is that we can move the widgets to the Inactive Widgets sidebar as for now. Seeing multiple Orphaned Sidebars seems not OK for me.

[westonruter]

Showing widgets from the orphaned sidebars among the widgets in the “inactive widgets” sidebar would be fine. But what should be restored, I believe, is widgets from those orphaned sidebars to be restored to their former widget areas when switching from a theme with 3 areas to 2 areas and back to 3 again.

[obenland]

I was testing with a theme that had sidebars sidebar-1 and sidebar-2 and then another theme that had sidebars primary and footer. I was expecting when switching that they would get mapped correspondingly, but they were not. So I included sidebar-1 and sidebar-2 in the primary and footer groups respectively.

That kind of expectation would probably be good to cover in a unit test. FWIW, the addition of sidebar-1 should be redundant because we're looking for *sidebar* with the next slug.

The lost widgets logic appears to be blowing a way old single widgets that lack widget numbers. I think that logic could probably be removed?

Good catch! Unfortunately the link doesn't seem to work?

I was working on scenarios for testing the various sidebar theme permutations, and I found a scenario where from a theme with 3 populated sidebars to a theme with 2 sidebars and then switching back to the theme with 3 sidebars results in the one sidebar that was not initially mapped being empty after having switched back. This seems bad to me. I think this is part of the orphaned sidebar question.

What we could do is to check wp_inactive_widgets for the ids in that third sidebar. Overall however I think the expectation that they map right back to where they were is only reasonable for immediate switch backs, yes?

[westonruter]

Replying to obenland:

That kind of expectation would probably be good to cover in a unit test. FWIW, the addition of sidebar-1 should be redundant because we're looking for *sidebar* with the next slug.

OK, good point. Yes, I agree it should be included in the unit test. Here are the scenarios I was testing with: ​https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations/blob/master/tests/widget-sidebars/scenarios.json Don't expect to recognize this file format, it's ad hoc. But you can see which theme was switched from and which was switched to, and which sidebars were populated.

Good catch! Unfortunately the link doesn't seem to work?

Apologies. I messed up the URL. Fixed in the comment above, and here it is again: ​https://github.com/xwp/wordpress-develop/blob/206f97d/src/wp-includes/widgets.php#L1152-L1158

If you want an old single widget to test with, you can use this one: ​https://gist.github.com/westonruter/7141599

What we could do is to check wp_inactive_widgets for the ids in that third sidebar. Overall however I think the expectation that they map right back to where they were is only reasonable for immediate switch backs, yes?

I was thinking that when switching from a 2-sidebar theme back to a 3-sidebar theme, that WP should try to restore each widget that was in the original 3rd sidebar unless the widget was moved to one of the other 2 sidebars when the 2-sidebar theme was active. So yeah, basically it would remove any widget IDs from the 3rd sidebar being restored to the 3-sidebar theme if those widget IDs now exist among the widgets in the 2-sidebar theme.

(Don't feel compelled to amend the PR I opened with additional commits. I was using it for testing and review and my own work.)

[obenland]

The lost widgets logic appears to be blowing a way old single widgets that lack widget numbers. I think that logic could probably be removed?

The purpose of that piece of code is to keep a default set of widgets in the sidebar. So it should probably just be enhanced to account for widgets without numbers

[obenland]

39693.7.diff​ adds test test_retreive_widgets_with_single_widget to cover single widgets that were lost, fixed by the is_numeric() check in line 1169 in wp-includes/widgets.php.

[obenland]

39693.8.diff​ merges orphaned widgets into wp_inactive_widgets and makes the necessary unit test changes.

[obenland]

39693.9.diff​ adds a basic way to support restoring old sidebar settings. Needs testing and polish, but it should be a step in the right direction.

[obenland]

@westonruter 39693.10.diff​ is ready for you. I'm inclined to commit that soon and iterate fro there as necessary

[melchoyce]

Replying to obenland:

@westonruter 39693.10.diff​ is ready for you. I'm inclined to commit that soon and iterate fro there as necessary

+1, let's get patches in early.

[westonruter]

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

[westonruter]

@obenland I can confirm that the test case I had of switching from a populated 3-sidebar theme to 2-sidebar theme and then back to 3-sidebar theme results in all of the sidebars being restored as expected. So the issue I identified in comment 88 is fixed. 🙌

[xkon]

@obenland , @westonruter , after a long night of going back and forth on over 60 themes both non & commercial I can say this.

First of all, as you informed me yesterday it depends on how the placeholders are named so some times it was confusing as from a sidebar the widgets where ending up on a footer etc ( TA-DA! feeling ) as the theme developers where playing trick on the code side as well x_X . So that won't be counted at all or counted as correct since that's what the code is supposed to do :)

All switches back and forth having where working as expected as @westonruter said above as well. So that's goody good.

Most of the time even if the placeholders where a lot more in 1 of the 2 test-combined themes when switching most of the widgets would end up in a similar position as the previous and surely return back to the correct one when reverting back.

No idea what else to check but I guess in an overall viewing with serious clicking of activates and refreshes it seems to be working as expected. Not that obvious in some cases as I mentioned but at least working. :D

Hopefully this is a little bit helpful as well as on my watch it was working as expected at least from what I understand.

[obenland]

In 41555:

Widgets: Improved sidebar mapping on theme switch

Builds on efforts brought forward in #17979.

This will send sidebars through three levels of mapping:

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

Finally, if the theme has previously been active and we have a record of its
sidebar configuration then, any unmapped sidebar will be restored to its
previous state.

Props westonruter, obenland, alexvorn2, timmydcrawford.
See #39693.

[obenland]

39693.11.diff​ fixes a bug, where orphaned widgets that were merged into the inactive sidebar would be overwritten with an empty array.

[bpayton]

Replying to obenland:

39693.11.diff​ fixes a bug, where orphaned widgets that were merged into the inactive sidebar would be overwritten with an empty array.

This looks good to me.

I have a couple of questions about the test, possibly due to being new to WordPress.

1. Where do the 6 default widgets come from in the following comment? Based on my reading of the test, there are 2 specified for wp_inactive_widgets. Where do the other 4 come from?

// 6 default widgets + 1 orphaned calendar widget = 7.

2. How do we know to look for recent-comments-2 in the wp_inactive_widgets array?

[poena]

Hi
I did not see the recent update so I have only tested 39693.10 (on 4.9-alpha-40870).

I did a lot of switching back and forward, mostly with parent themes.
One thing I noticed was that sometimes the individual widgets were duplicated in the same widget area.
I also think it might be hard for new users to see how to make the inactive widgets show again.

When trying to reproduce the duplicate widget problem, I wrote down the latest test:

Fresh install
Go to customizer and save the starter content widgets in Twenty Seventeen.
Change to Twenty Sixteen, add new widgets to any of the widget areas.
Change back to Twenty Seventeen. The widgets displays correctly.
Change to Twenty Thirteen. The starter content text widgets from Twenty Seventeen are now duplicated in the "main" widget area.
Change back to Twenty seventeen. The widgets are no longer duplicated.
Installed a random theme from the latest themes list, (stripes) the widgets displayed correctly.
Installed a random child theme from the latest themes list (katlan), the widgets displayed correctly.
Changed back to Twenty Seventeen. Only one widget area is showing widgets on the front and in the customizer preview (the blog sidebar).
All the widgets are present in the customizer sidebar, but the the widgets that are not showing on the page preview are greyed out. I assumed that this meant they are inactive.
I checked if perhaps I could press the customizer save button in order to restore the greyed out widgets, but the button already says saved, so that did not work.
I tried to see if I could expand the widgets themselves and see if there was a save or "restore" option there, but I could only see links for "Remove" and "Done"
Clicking the Done link did not help me restore my widget, it only collapsed it.
In this widget area there was only one widget. Clicking "Reorder" and then "Done" also did not restore my widget.
In order to make the widget display in the widget area in the page preview, I had to add a new widget.
Once I added another widget, the previously greyed out widget was no longer greyed out.

[obenland]

Replying to bpayton:

Love those questions! Researching the exact answers for those helped me too in exactly understanding how it works.

// 6 default widgets + 1 orphaned calendar widget = 7.
Where do the 6 default widgets come from in the following comment? Based on my reading of the test, there are 2 specified for wp_inactive_widgets. Where do the other 4 come from?

When a site gets first installed, WP adds dummy info for these six widgets.
When we run wp_widgets_init() at the top of each test, WP looks for existing settings and assigns IDs based on that, and starts with 1 if it can't find any.
Finally, in retreive_widgets(), it compares the shown widgets with all registered widgets, and weeds out all widgets that are not those six default ones, or one that a user registered.

How do we know to look for recent-comments-2 in the wp_inactive_widgets array?

Recent Comments is one of the six default widgets, so it should always be there. BUT! That's not what the test should be testing for. It should make sure 'calendar-1' is in the inactive sidebar, since we're testing to make sure orphaned widgets get folded into that!

This is a left over from an earlier version of the test and I missed updating that together with the setup. Good catch!

[obenland]

Thanks for posting your test results @poena! Do you think there's any way to port your scenario over into a phpunit test?

[bpayton]

Replying to obenland:

Love those questions! Researching the exact answers for those helped me too in exactly understanding how it works.

Thanks for the helpful explanation.

This is a left over from an earlier version of the test and I missed updating that together with the setup. Good catch!

That's great. I'll keep asking these kinds of questions then. The latest patch looks good.

[westonruter]

@obenland I'm able to reproduce PHP warnings when switching between Twenty Seventeen and Twenty Fifteen, see conversation at ​https://wordpress.slack.com/archives/C0381N237/p1506163588000022

Here are the warnings and where they happen:

Warning: Invalid argument supplied for foreach() in /srv/www/wordpress-develop/public_html/src/wp-includes/widgets.php on line 1318

Line in question:

// ...for every widget we're trying to revive.
foreach ( $old_widgets as $key => $widget_id )

Also:

Warning: array_merge(): Argument #1 is not an array in /srv/www/wordpress-develop/public_html/src/wp-includes/widgets.php on line 1156

At:

// Find hidden/lost multi-widget instances.
$shown_widgets = call_user_func_array( 'array_merge', $sidebars_widgets );

And on the next line:

Warning: array_diff(): Argument #2 is not an array in /srv/www/wordpress-develop/public_html/src/wp-includes/widgets.php on line 1157

At:

$lost_widgets  = array_diff( $registered_widgets_ids, $shown_widgets );

And two lines below:

Warning: Invalid argument supplied for foreach() in /srv/www/wordpress-develop/public_html/src/wp-includes/widgets.php on line 1159

foreach ( $lost_widgets as $key => $widget_id ) {

And lastly:

Warning: array_merge(): Argument #1 is not an array in /srv/www/wordpress-develop/public_html/src/wp-includes/widgets.php on line 1167

At:

$sidebars_widgets['wp_inactive_widgets'] = array_merge( $lost_widgets, (array) $sidebars_widgets['wp_inactive_widgets'] );

[obenland]

In 41594:

Widgets: Merge orphaned widgets on theme switch

Merges orphaned widgets into the inactive sidebar when switching themes.

Props bpayton.
See #39693.

[obenland]

@afercia @westonruter 39693.13.diff​ restores the previous format of the sidebars_widgets theme mod. In testing it will create one more warning, but subsequent theme switches should go through smoothly.

[poena]

I have never written one so if you stil need more phpunit tests it is faster if someone else writes it while I continue to read the documentation :)

[obenland]

@afercia @westonruter Have you had a chance to take a look at my latest patch?

[westonruter]

@obenland Restoring the old data format for the sidebars_widgets theme mod seems right to me. I tried switching themes with the patch applied and I got no warnings. Should get committed for wider testing.

[obenland]

In 41673:

Widgets: Revert format of sidebars_widgets.

Restores the data format of the sidebars_widgets theme mod to its pre [41555]
format. It can trigger PHP warnings after switching themes if that mod has not
been removed previously.

Fixes #39693.

[westonruter]

For the 3.9.1 headaches with widgets and theme switching, see #27897. We need to avoid this happening again.

[alexvorn2]

Replying to westonruter:

For the 3.9.1 headaches with widgets and theme switching, see #27897. We need to avoid this happening again.

Theme preview action is buggy so we need to fix that too: please take a look to this ticket #40221, It could be related to #27897.

[obenland]

Replying to westonruter:

For the 3.9.1 headaches with widgets and theme switching, see #27897. We need to avoid this happening again.

I can't reproduce the bug outlined in that ticket's description. If you have any concrete bugs that I should be aware of I'd be happy to add more tests.

Theme preview action is buggy so we need to fix that too: please take a look to this ticket #40221, It could be related to #27897.

@alexvorn2 Have commits from this ticket introduced any bugs? It looks like #40221 was last updated 7 months ago.

[alexvorn2]

Replying to obenland:

@alexvorn2 Have commits from this ticket introduced any bugs? It looks like #40221 was last updated 7 months ago.

I don't think so.
It was not updated because that ticket was not getting any attention or interest. I think that should be a simple fix, maybe you would like to take part and check how to fix the problem.