New widget interface moves the wrong inaccessible widgets when switching themes

[Denis-de-Bernardy]

In the previous implementation of widgets, one could switch themes without too much hassle. Widgets in sidebars used by the old theme would get ignored.

The new implementation seems to randomly import widgets.

I've two test sites. One was installed with my theme, with multitudes of sidebars: sidebar-1, sidebar-2, top_sidebar, bottom_sidebar, and more. Added widgets in them, then switched to Kubrik.

On the other verbatim WP test site (same DB, home and siteurl overridden by WP_HOME_URL etc), widgets seem to change every now and then whenever I try adding a new one in Kubrik's sidebar.

[Denis-de-Bernardy]

Just a second ago, for instance, the text widget that was in my top_sidebar ended up replacing the pages widget I had in sidebar-1, for no obvious reason. I'll try to find a means to consistently replicate the issue if needed. (seems quite random)

[Denis-de-Bernardy]

load the dump into a trunk install, browse dashboard.

var_dump(get_option('sidebars_widgets'));

array(12) {
  ["the_entry"]=>
  array(6) {
    [0]=>
    string(12) "entry_header"
    [1]=>
    string(13) "entry_content"
    [2]=>
    string(10) "entry_tags"
    [3]=>
    string(16) "entry_categories"
    [4]=>
    string(22) "bookmark_me-1239359401"
    [5]=>
    string(14) "entry_comments"
  }
  ["the_header"]=>
  array(4) {
    [0]=>
    string(6) "header"
    [1]=>
    string(6) "navbar"
    [2]=>
    string(12) "header_boxes"
    [3]=>
    string(14) "text-402142561"
  }
  ["before_the_entries"]=>
  array(1) {
    [0]=>
    string(15) "archives_header"
  }
  ["after_the_entries"]=>
  array(1) {
    [0]=>
    string(15) "next_prev_posts"
  }
  ["the_footer"]=>
  array(2) {
    [0]=>
    string(12) "footer_boxes"
    [1]=>
    string(6) "footer"
  }
  ["sidebar-1"]=>
  array(4) {
    [0]=>
    string(18) "nav_menu-402143571"
    [1]=>
    string(22) "fuzzy-widget-402143751"
    [2]=>
    string(20) "categories-402145421"
    [3]=>
    string(18) "archives-402145422"
  }
  ["sidebar-2"]=>
  array(2) {
    [0]=>
    string(22) "subscribe_me-406661311"
    [1]=>
    string(18) "calendar-402143881"
  }
  ["inline_widgets"]=>
  array(4) {
    [0]=>
    string(28) "newsletter_widget-1239359402"
    [1]=>
    string(23) "contact_form-1239359401"
    [2]=>
    string(9) "silo_stub"
    [3]=>
    string(8) "silo_map"
  }
  ["feed_widgets"]=>
  array(0) {
  }
  ["top_sidebar"]=>
  array(1) {
    [0]=>
    string(14) "text-402140811"
  }
  ["bottom_sidebar"]=>
  array(1) {
    [0]=>
    string(14) "text-402141571"
  }
  ["array_version"]=>
  int(3)
}

after browsing widgets:

array(3) {
  ["sidebar-1"]=>
  array(6) {
    [0]=>
    string(12) "entry_header"
    [1]=>
    string(13) "entry_content"
    [2]=>
    string(10) "entry_tags"
    [3]=>
    string(16) "entry_categories"
    [4]=>
    string(22) "bookmark_me-1239359401"
    [5]=>
    string(14) "entry_comments"
  }
  ["wp_inactive_widgets"]=>
  array(28) {
    [0]=>
    string(7) "pages-2"
    [1]=>
    string(10) "calendar-2"
    [2]=>
    string(10) "archives-2"
    [3]=>
    string(18) "archives-402145422"
    [4]=>
    string(7) "links-2"
    [5]=>
    string(6) "meta-2"
    [6]=>
    string(8) "search-2"
    [7]=>
    string(6) "text-2"
    [8]=>
    string(12) "categories-2"
    [9]=>
    string(20) "categories-402145421"
    [10]=>
    string(5) "rss-2"
    [11]=>
    string(11) "tag_cloud-2"
    [12]=>
    string(6) "header"
    [13]=>
    string(6) "navbar"
    [14]=>
    string(12) "header_boxes"
    [15]=>
    string(14) "text-402142561"
    [16]=>
    string(15) "archives_header"
    [17]=>
    string(15) "next_prev_posts"
    [18]=>
    string(12) "footer_boxes"
    [19]=>
    string(6) "footer"
    [20]=>
    string(22) "subscribe_me-406661311"
    [21]=>
    string(18) "calendar-402143881"
    [22]=>
    string(28) "newsletter_widget-1239359402"
    [23]=>
    string(23) "contact_form-1239359401"
    [24]=>
    string(9) "silo_stub"
    [25]=>
    string(8) "silo_map"
    [26]=>
    string(14) "text-402140811"
    [27]=>
    string(14) "text-402141571"
  }
  ["array_version"]=>
  int(3)
}

Expected: sidebars_widgets remains untouched. And internally, WP manages an wp_inactive_widgets sidebar with those or something like that. Or it should at least move widgets around in the correct sidebar if one exists with the same name.

[Denis-de-Bernardy]

test data to reproduce the issue

[Denis-de-Bernardy]

login details for the sql dump are admin/admin

[Denis-de-Bernardy]

Related: #6161

[azaozz]

This is actually one of the main requested improvements: widgets don't disappear after switching from theme with many sidebars to theme with less sidebars.

If we keep the old behaviour (as you suggest) all widgets from sidebars 2 - 12 above would be in "limbo", they are still configured but cannot be displayed and cannot be moved as the sidebars are gone.

Until now the users had to empty the sidebars by hand before switching themes, losing the widgets' settings in the process, now all widgets that would be inaccessible are moved to Inactive Widgets.

The only bug or rather enhancement that is needed is a better explanation in the widgets screen help.

[Denis-de-Bernardy]

Oh, don't get me wrong, I fully agree the current interface is a major improvement. just...

Please check the two dumps above. The first one contains widgets in a sidebar-1. The second one contains the widgets from the first sidebar (the_entry) into sidebar-1. Something's very wrong here.

At the very least, it should put the widgets in the correct locations when sidebar names are shared. Most themes use sidebar-1, sidebar-2, and frequently share similar names for the top, bottom, left and right sidebars (top_sidebar or top-sidebar, etc.). There is no need to try to be smart by tossing widgets from similarly named sidebars into their new equivalents, but it should at least keep sidebars whose names are exactly the same.

The other point I find strange is, why is the sidebars_widgets option edited even before any changes are made? It seems like there is no need to do this unless widgets are moved around, no?

[azaozz]

The function that does this is retrieve_widgets() currently in wp-admin/widgets.php. It needs to run after the theme has been changed and may need some improvements (patches welcome).

May also need to be relocated and a hook after_theme_change or similar created so it would run on the next page load after a theme change even without visiting the widgets settings screen.

[Denis-de-Bernardy]

Ok. Thanks for the tip on where to look. You'll have a patch by tomorrow.

I could use your feedback before moving forward. My current intention is to proceed as follows:

• During the conversion, I'd ensure that sidebars with similar names are left unchanged.
• Until the widgets page is loaded and a widget is moved, the actual option would get converted by a filter on all page loads, but not saved. That way, theme preview works too. And you can activate a theme and look at its options screen before things become permanent.

If you feel any of the above is wrong or think of anything else that might be needed, just let me know.

[azaozz]

Replying to Denis-de-Bernardy:

• During the conversion, I'd ensure that sidebars with similar names are left unchanged.

There's actually a loop that checks for identical names, perhaps see why it doesn't pick your old "sidebar-1" from above.

• Until the widgets page is loaded and a widget is moved, the actual option would get converted by a filter on all page loads, but not saved. That way, theme preview works too. And you can activate a theme and look at its options screen before things become permanent.

Yes, moving a widget will overwrite $sidebars_widgets with the new data. The problem is with the front-end. Can keep converting it only in memory on each page load but that will have to happen for the front-end too so widgets are displayed. It's not a lot of code but wanted to avoid adding more runtime functions (also the part where it looks for lost multi-widgets settings will have to look only in the options cache to avoid more db queries).

[Denis-de-Bernardy]

I'm currently nailing it down to this:

	// Assign to each unmatched registered sidebar the first available orphan
	while ( ( $sidebar = array_shift( $sidebars ) ) && $widgets = array_shift( $sidebars_widgets ) )
		$_sidebars_widgets[ $sidebar ] = $widgets;

Apart from the fact that it does not work as the comment suggests it shows, it is invalid to do so. I've a plugin called Feed Widgets, for instance, which allows to put widgets after each post in a feed. If left empty, and were the above code work, it may end up tossing widgets into there, without the user being aware of it.

I take it that it's there to make the likes of converting from sidebar-1, sidebar-2 to left-sidebar, right-sidebar automatic. But this can be so potentially bug prone that I believe we're better off not trying to do so at all, and letting the user configure his widgets as needed upon changing themes.

[Denis-de-Bernardy]

sidebars_widgets dump after the current patch:

array(3) {
  ["sidebar-1"]=>
  array(2) {
    [0]=>
    string(20) "categories-402145421"
    [1]=>
    string(18) "archives-402145422"
  }
  ["wp_inactive_widgets"]=>
  array(14) {
    [0]=>
    string(7) "pages-2"
    [1]=>
    string(10) "calendar-2"
    [2]=>
    string(10) "archives-2"
    [3]=>
    string(7) "links-2"
    [4]=>
    string(6) "meta-2"
    [5]=>
    string(8) "search-2"
    [6]=>
    string(6) "text-2"
    [7]=>
    string(12) "categories-2"
    [8]=>
    string(5) "rss-2"
    [9]=>
    string(11) "tag_cloud-2"
    [10]=>
    string(14) "text-402142561"
    [11]=>
    string(18) "calendar-402143881"
    [12]=>
    string(14) "text-402140811"
    [13]=>
    string(14) "text-402141571"
  }
  ["array_version"]=>
  int(3)
}

[Denis-de-Bernardy]

As things are with the patch I just uploaded, it's harmless on the front end's standpoint:

• When previewing a new theme (and upon activating one), he may get empty sidebars, which is a lesser evil as compared to widgets being randomly moved around.
• Upon switching his theme, the user will thus want to visit the widgets screen, where his old sidebars_widgets option gets overwritten (as previously was the case).
• No extra missed cache hit or overhead were added, per your request.

[Denis-de-Bernardy]

Side note: I took the liberty to remove the setting of array_version, since it's set to its latest value in wp_set_sidebars_widgets(). In case it changes in the future...

[Denis-de-Bernardy]

The second patch I've uploaded is optional.

It's a fix to the wp_convert_widget_settings function. It prevents unused, single widgets from getting listed twice (once in the available widgets, and once in the inactive widgets) during the 2.8 upgrade process.

Whether this is desirable is probable. As in, if I'm not using the tag cloud widget, there doesn't seem to be any valid reason to clutter my inactive widgets with one in addition to the available tag cloud widget. But it could also be argued that there might be a pre-configured, "saved" one somewhere.

[Denis-de-Bernardy]

I'm itching to make a third patch after these two are committed, to make sure that single widgets never make it into the inactive widgets list. It once happened to me (i.e. to have them in the inactive widgets list, and again in a sidebar), and it's a huge mess when it does.

[Denis-de-Bernardy]

please get this into wp 2.8 asap. it's breaking two of my plugins (inline widgets, feed widgets), which add sidebars. I can think of workarounds using the plugins themselves, but they're just awkward.

see also #9703, which is even more severe.

[azaozz]

(In [11161]) Do not import sidebars by index after switching themes, props Denis-de-Bernardy, see #9695

[azaozz]

Don't think the other patch is necessary. The bug showing single widgets twice when they are in Inactive Widgets was elsewhere and is now fixed.

[Denis-de-Bernardy]

Closing as fixed then. will re-open if I see it happening again. Thanks for having committed this.

I'd appreciate that you took care of #9703, too. Two of my widgets' interfaces are rendered completely dysfunctional until it gets fixed. :-(