Unregistering default widgets is not backwards compatible

[mtdewvirus]

Themes and plugins use wp_unregister_sidebar_widget to remove default widgets and then register their own with register_sidebar_widget. Default widgets have been updated to use WP_Widget, so they do not get unregistered.

As an example...

function sapphire_widgets_init() {
        register_sidebars(1, array(
                'before_widget' => '<div id="%1$s" class="widget %2$s">',
                'after_widget' => '</div>'
        ));
        function widget_sapphire_pages($args) {
                extract( $args );
                $options = get_option( 'widget_pages' );

                $title = empty( $options['title'] ) ? __( 'Pages' ) : apply_filters('widget_title', $options['title']);
                $sortby = empty( $options['sortby'] ) ? 'menu_order' : $options['sortby'];
                $exclude = empty( $options['exclude'] ) ? '' : $options['exclude'];

                if ( $sortby == 'menu_order' ) {
                        $sortby = 'menu_order, post_title';
                }

                $out = wp_list_pages( array('title_li' => '', 'echo' => 0, 'sort_column' => $sortby, 'exclude' => $exclude) );

                echo $before_widget;
                echo $before_title . $title . $after_title;
        ?>
                <ul>
                        <li class="page_item"><a href="<?php bloginfo('url'); ?>">Home</a></li>
                        <?php echo $out; ?>
                </ul>
        <?php
                echo $after_widget;
        }

        unregister_sidebar_widget('pages');
        unregister_widget_control('pages');
        wp_register_sidebar_widget('pages', __('Pages'), 'widget_sapphire_pages', null, 'pages');
        wp_register_widget_control('pages', __('Pages'), 'wp_widget_pages_control' );
}
add_action('widgets_init', 'sapphire_widgets_init');

When an action runs this on widgets_init the default Pages widget is not removed. The Widgets screen will display both Pages widgets but only one of them will work because both the default widget and the new widget are using the same id.

[Denis-de-Bernardy]

Probably invalid. I may be wrong, but I believe the name of the registered widget has changed. As in, to WP_Widget_Pages.

[ryan]

Could map names for default widget so as to preserve back compat. Or peek at the id for all registered WP_Widget widgets and see if a widget matches.

[Denis-de-Bernardy]

Then again, the names already changed (a very messy business, I'd add), when widgets went from single-widget to multi-widget since 2.0. Imo, we should close this one as wontfix. It's just "yet another widget name change". It's a necessary evil, and probably the last.

[azaozz]

We can either skip registering the new multi-widget when there's a registered widget with the same id or unregister the old style widget while registering the new one.

Seems most of the widgets that replace a default widget use the same option when storing the settings (not compatible with the new multi-widgets) and may use directly some functions that were part of the default widgets before and don't exist now. So perhaps it's better to unregister single widgets that try to replace a default widget.

[ryan]

Patch builds upon yours. It adds the ability to pass an id to WP_Widget_Factory::unregister() and calls unregister_widget() from the unregister cases in wp_register_widget_control() and wp_register_sidebar_widget(). Not yet tested, and phpdoc needs updated to reflect that ids and widget classes can be passed.

[ryan]

9952.2.diff tested and working for me. It reveals another problem in the example above. sapphire tries to re-register the pages widget with the no longer existing wp_widget_pages_control() callback. A survey of wp.com themes show that this is the only theme there that does this. Hopefully it's not too common.

[azaozz]

Replying to ryan:

... It reveals another problem in the example above. sapphire tries to re-register the pages widget with the no longer existing wp_widget_pages_control() callback.

Yes, exactly. Found couple of other widgets that do the same. That's why I thought it is better to not register single widgets that are replacing default widgets.

We are registering default widgets with hook priority of 100, so if we want to keep these widgets, we could do something like:

function _register_widgets() {
	global $wp_registered_widgets;
 	$keys = array_keys($this->widgets);
	$registered = array_keys($wp_registered_widgets);

	foreach ( $keys as $key ) {
		// don't register new widget if old, single widget with the same id is already registered
		if ( in_array($this->widgets[$key]->id_base, $registered, true) )
			continue;

 		$this->widgets[$key]->_register();
	}	
}

We should probably do an is_callable check on registering a widget and skip any widget that fails, making sure to unregister it from all globals.

[ryan]

Replacing default widgets is commonplace. We have to keep supporting that, I think.

[ryan]

Default widgets are registered before the widgets_init action is called, so they should be registered before widgets added by plugins and themes. My 9952.2.diff allowed the sapphire theme to unregister the default pages widget and register a replacement.

[ryan]

add_action( 'widgets_init', array( &$this, '_register_widgets' ), 100 );

Duh, I forgot about this. I think I even wrote that. :-)

[ryan]

In 2.7, all default widgets are registered before widgets_init fires. Perhaps we should call WP_Widget_Factory::_register_widgets from wp_widgets_init() right before the widgets_init action.

[ryan]

But that won't play nicely with widgets using the new API that are added via plugin. Grrr.

[Denis-de-Bernardy]

maybe add, much like with the admin_menu:

add_action('_widgets_init', array( &$this, '_register_widgets'));

and then:

add_action('widgets_init', array( &$this, '_register_widgets'));

and add a check to verify it's not already registered?

[Denis-de-Bernardy]

err, sorry. was meaning:

do_action('_widgets_init');

...

do_action('widgets_init');

but I'm sure you got the point.

[azaozz]

The current order seems to be working well, we don't need to init a default widget if a replacement is already active and in 9952.2.diff we have a way for an external widget to unregister a default one if the external loads later. Still testing different variations.

[ryan]

Yeah, the current order works fine, as you say. 9952.3.diff is 9952.2.diff without 9952.patch. I don't think we'll need 9952.patch in practice, but it probably doesn't hurt.

[azaozz]

9952.4 is about the same as 9952.3 but it doesn't allow registering a widget that calls one of the old default widgets callbacks (not existing any more). Then it checks for already registered widget with the same id before registering new widget. This would stop registering widgets with conflicting id's too.

[ryan]

Looks good to me.

[ryan]

[11484]