Optimization in wp_get_sidebars_widgets() corrupts the widgets

[Denis-de-Bernardy]

Found this while testing upgrade scripts. It's not simple to reproduce in a live case, either. (You need to switch from a multi-sidebar theme to another multi-sidebar theme with different sidebars, possibly at the same time you're upgrading from WP 2.7 to WP 2.8.1-beta.)

Basically, the workflow in the non-admin area goes down to this:

1. call wp_get_sidebars_widgets() (e.g. due to dynamic_sidebar())
2. check for $_wp_sidebars_widgets, which isn't set yet
3. set $_wp_sidebars_widgets using get_option()
4. if array_version is not set, run upgrade procedure
5. unset array_version, so as to return a clean sidebars_widgets array
6. call wp_get_sidebars_widgets() again for whatever reason (e.g. you've two sidebars)
7. check for $_wp_sidebars_widgets, which is now set
8. array_version is no longer around, since it was unset, so run the upgrade procedure
9. Corrupt empty sidebars, and turn the site into a train wreck (in my case, ext_sidebar became top_sidebar instead of the sidebar-2 that I was trying to auto-convert)

}}}

I investigated a couple of potential patches.

Setting array_version to 3 in wp_convert_widget_settings is plain wrong.

Merely removing the ref in wp_get_sidebars_widgets() works but it potentially removes the work that wp_convert_widget_settings() is doing in the background.

Checking for $_wp_sidebars_widgets before setting a default array_version seemed wrong: on a very old site, array_version is not set indeed (I've yet to test how it behaves, but figured it was better to just fix the workflow instead).

The last option was to fix the workflow. The attached patch does just that.

[azaozz]

To work around this we can add back the array_version when $_wp_sidebars_widgets is not empty https://core.trac.wordpress.org/browser/trunk/wp-includes/widgets.php#L970

Perhaps we can remove the conversion part from wp_get_sidebars_widgets() for 2.9. It seems it was used in WordPress 2.2 to convert previous widgets set by the Sidebar Widgets plugin (​http://wordpress.org/extend/plugins/widgets/) that was added to the core in 2.2. So the conversion is not needed unless upgrading from WordPress 2.1 or earlier and using the plugin.

[azaozz]

Can you confirm if this fixes it?

[azaozz]

The second patch should speed it up a bit too.

[Denis-de-Bernardy]

odd that I didn't receive any emails on this one. trac seems broken.

haven't tested, but the logic is incorrect.

10300.2.patch prevents the sidebars_widgets filter from getting applied differently from a call to the next. on occasion, you want to do something like this:

if ( !is_active_sidebar('my_sidebar') ) {
  add_filter('sidebars_widgets', 'my_sidebar_autofill');
}

your patch removes that possibility. (mine doesn't)

[Denis-de-Bernardy]

haven't tested 10300.1.patch either, but it seems like, if, for whatnot reason, an old version of the array is passed, the upgrader no longer works.

adding to the ticket, I just had a user whose sidebars were wrecked and mangled after upgrading an old site, and I suspect it's due to the array_version 2 upgrader. could we, instead of assigning non-existing sidebars to new ones, assign unused widgets over into the inactive widgets "sidebar"?

[azaozz]

Replying to Denis-de-Bernardy:

10300.2.patch prevents the sidebars_widgets filter from getting applied differently from a call to the next.

The return $_wp_sidebars_widgets; in wp_get_sidebars_widgets() can be replaced with

return apply_filters('sidebars_widgets', $_wp_sidebars_widgets);

to fix that. The patch makes sure the conversion there is run only once and adds and removes array_version properly.

could we, instead of assigning non-existing sidebars to new ones, assign unused widgets over into the inactive

This is done in wp-admin/widgets.php as it applies to changing themes too. Could perhaps add it here as well. Nevertheless when upgrading from very old version of WordPress the user would need to visit the Widgets screen to properly reset any previously used widgets.

[Denis-de-Bernardy]

Replying to azaozz:

Replying to Denis-de-Bernardy:

10300.2.patch prevents the sidebars_widgets filter from getting applied differently from a call to the next.

The return $_wp_sidebars_widgets; in wp_get_sidebars_widgets() can be replaced with (...)

but then whichever first filter is applied (if any) becomes "sticky" (and potentially tossed into the the db).

then again, we already have that problem anyway, so +1 to the approach.

could we, instead of assigning non-existing sidebars to new ones, assign unused widgets over into the inactive

This is done in wp-admin/widgets.php as it applies to changing themes too. Could perhaps add it here as well. Nevertheless when upgrading from very old version of WordPress the user would need to visit the Widgets screen to properly reset any previously used widgets.

this would be desirable imo.

[azaozz]

As update_option('sidebars_widgets', $sidebars_widgets); is not run from the front-end (unless a plugin or theme does it which is incorrect) the filtered $sidebars_widgets would not get saved in the db. Unfortunately some plugins and themes seem to apply this filter on the back-end too but the changes they make are designed for the front-end.

[Denis-de-Bernardy]

Might be wrong, but your last patch seems to introduce an inconsistency:

wp_convert_widgets -> now returns (and saves) an array without any version.

I continue to think the initial approach I suggested introduces the least possible number of issues, even if it ends up requiring slightly more memory.

[azaozz]

wp_convert_widgets() doesn't return the widgets order array, it uses the option directly when in admin (and the array_version is already there) or sets $GLOBALS['_wp_sidebars_widgets'] without the array_version as it should be.

[Denis-de-Bernardy]

right. but then you go:

$GLOBALS['_wp_sidebars_widgets'] = wp_get_sidebars_widgets(false); // without array version, and with filters applied

...

$sidebars_widgets = &$GLOBALS['_wp_sidebars_widgets'];

...

update_option('sidebars_widgets', $sidebars_widgets); // should be wp_set_sidebars_widgets($sidebars_widgets);, and $sidebars_widgets has filters applied

[azaozz]

Heh, $GLOBALS['_wp_sidebars_widgets'] = wp_get_sidebars_widgets(false); is used if not admin and update_option('sidebars_widgets', $sidebars_widgets); is run only if admin...

[Denis-de-Bernardy]

Mm, right. However, we still have a sticky array around on the front end with, potentially, some filters potentially applied inconsistently.

It still feels like a gaz factory that can blow up if widgets are saved on the front end. But I've an idea in mind that should work without needing to squash the global.

[Denis-de-Bernardy]

I attached a new patch, based on your last one, that ensures that filters don't get applied to $_wp_sidebars_widgets before caching it.

I'll be testing it for the rest of the afternoon.

[Denis-de-Bernardy]

confirming that the themes with two or more sidebars no longer end up with corrupt sidebars when upgrading.

still looking into widgets upgrades, but I'm expecting no issues on that front either.

[Denis-de-Bernardy]

Rewrote my widget upgrade scripts, and it's working like a charm with 10300.2.diff applied. With your own patch (10300.3.patch) applied, the global contains filtered values, and I expect this'll eventually lead to bugs.

	function init() {
		if ( get_option('widget_nav_menu') === false ) {
			foreach ( array(
				'nav_menus' => 'upgrade',
				) as $ops => $method ) {
				if ( get_option($ops) !== false ) {
					$this->alt_option_name = $ops;
					add_filter('option_' . $ops, array(get_class($this), $method));
					break;
				}
			}
		}
	} # init()


	function nav_menu() {
		$widget_ops = array(
			'classname' => 'nav_menu',
			'description' => __("A navigation menu", 'nav-menus'),
			);
		$control_ops = array(
			'width' => 330,
			);
		
		$this->init();
		$this->WP_Widget('nav_menu', __('Nav Menu', 'nav-menus'), $widget_ops, $control_ops);
	} # nav_menu()


	function upgrade($ops) {
		$widget_contexts = class_exists('widget_contexts')
			? get_option('widget_contexts')
			: false;
		
		unset($ops['header']);
		unset($ops['footer']);
		
		foreach ( $ops as $k => $o ) {
			if ( isset($widget_contexts['nav_menu-' . $k]) ) {
				$ops[$k]['widget_contexts'] = $widget_contexts['nav_menu-' . $k];
			}
		}
		
		$extra = get_option('silo_widgets');
		
		if ( $extra !== false ) {
			foreach ( $extra as $k => $o ) {
				if ( !isset($ops[$k]) ) {
					$ops[$k] = $extra[$k];
					if ( isset($widget_contexts['silo_widget-' . $k]) ) {
						$ops[$k]['widget_contexts'] = $widget_contexts['silo_widget-' . $k];
					}
				} else {
					unset($extra[$k]);
				}
			}
		}
		
		#global $wp_filter, $_wp_sidebars_widgets;
		#$filter_backup = isset($wp_filter['sidebars_widgets']) ? $wp_filter['sidebars_widgets'] : array();
		#unset($wp_filter['sidebars_widgets']);
		#$_wp_sidebars_widgets = array();
		#$sidebars_widgets = wp_get_sidebars_widgets(false);
		#$wp_filter['sidebars_widgets'] = $filter_backup;
		#$_wp_sidebars_widgets = array();

		global $_wp_sidebars_widgets;
		if ( is_admin() ) {
			$sidebars_widgets = get_option('sidebars_widgets', array());
		} else {
			if ( !$_wp_sidebars_widgets )
				wp_get_sidebars_widgets(false);
			$sidebars_widgets =& $_wp_sidebars_widgets;
		}
		
		$keys = array_keys($extra);
		
		foreach ( $sidebars_widgets as $sidebar => $widgets ) {
			if ( !is_array($widgets) )
				continue;
			foreach ( $keys as $k ) {
				$key = array_search("silo_widget-$k", $widgets);
				if ( $key !== false ) {
					$sidebars_widgets[$sidebar][$key] = 'nav_menu-' . $k;
					unset($keys[array_search($k, $keys)]);
				}
			}
		}
		
		#wp_set_sidebars_widgets($sidebars_widgets);
		if ( is_admin() )
			update_option('sidebars_widgets', $sidebars_widgets);
		
		return $ops;
	} # upgrade()

[Denis-de-Bernardy]

deprecate the array version

[Denis-de-Bernardy]

mm, the deprecate diff won't be good, as the sidebars_widgets thingy may end up containing junk (array version 1 is quite different).

[azaozz]

Combined the ideas from couple of the previous patches. Seems to work well when we always use $_wp_sidebars_widgets on the front end and set it before the filter.

[Denis-de-Bernardy]

safe obsolete

[Denis-de-Bernardy]

same as previous without the typo

[azaozz]

(In [11690]) Convert the old style widgets order array only when loading the widgets screen, fixes #10300 for 2.8

[azaozz]

(In [11691]) Convert the old style widgets order array only when loading the widgets screen, fixes #10300 for trunk

[Denis-de-Bernardy]

I'm sorry, but there still are issues. :-(

The patch you committed still has a couple of issues when working on old-style widgets. In essence, the Recent Comments widget will introduce a workflow error, and essentially flush the sidebars during a WP 2.0 to 2.8 upgrade, because it calls wp_get_sidebars_widgets() before all widgets are flushed.

The new patch I just attached does the following:

• ensure that the array_version conversion only occurs once, and only when all widgets are registered
• ensure that the WP_Widget class conversion only occurs when array_version is 3 (which works fine, since it gets called several times on the front end)

I've been testing it since this morning, and building my own upgrade scripts against it. I've yet to complete my own scripts, but so far so good: it's working fine with WP with all versions since 2.0, and I've yet to run into issues related to the slightly changed flow that I need to deal with.

[Denis-de-Bernardy]

same as the previous, refreshed against trunk

[Denis-de-Bernardy]

the other issue I noticed is, while the old widgets are indeed converted properly on the widgets screen (the WP ones anyway), it fails to save the widgets once converted. :-|

the patch I attached fixes this too. merely visiting the dashboard converts and saves the stuff properly.

[azaozz]

There are two different workflows here, each with two cases:

• When upgrading from WordPress 2.2+ (99.9% of the cases?) the widgets' settings are converted from single to multi format for most default widgets. The conversion happens only in memory on the front-end and is saved when the user visits the widgets screen.

• When upgrading from WordPress <= 2.1 two different conversions have to be done and the old widgets plugin has to be disabled (this disables all widgets added by it). Then the settings from the old widgets have to be converted to the newer (in 2.2) array format and then they have to be converted to the new (in 2.8) multi-widget settings format. These steps can be performed reliably when the user visits the widgets screen, so currently WordPress doesn't run these conversions for the front-end and doesn't save the widgets order array from there.

The widgets order is explicitly saved when the user visits the widgets screen either in retrieve_widgets() or when a widget is added/moved/deleted. The latter also resets the order array completely.

Since nothing is saved unless the user visits the widgets screen, plugins have the opportunity to manipulate any settings and/or perform a conversion before that.

[Denis-de-Bernardy]

And the other patch I added ensures they get converted properly in all cases.

Anyway... Whatever approach we decide to keep, I noticed that it didn't actually save the new order in trunk: using the sql dump I gave you by email, I browse the admin area, run the upgrader, visit the widgets screen, and everything looks fine indeed. Deciding to not touch a thing, I visit the site's front page, and things are all wacked again. (Moving just one widget fixes this.)

[Denis-de-Bernardy]

I worked around the remaining WP bugs. Thanks for the fix.

D.