Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#9952 closed defect (bug) (fixed)

Unregistering default widgets is not backwards compatible

Reported by: mtdewvirus Owned by: azaozz
Milestone: 2.8 Priority: high
Severity: normal Version: 2.8
Component: Widgets Keywords: widgets
Focuses: Cc:


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;
                        <li class="page_item"><a href="<?php bloginfo('url'); ?>">Home</a></li>
                        <?php echo $out; ?>
                echo $after_widget;

        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.

Attachments (5)

9952.patch (716 bytes) - added by azaozz 6 years ago.
9952.diff (1.4 KB) - added by ryan 6 years ago.
9952.2.diff (1.4 KB) - added by ryan 6 years ago.
9952.3.diff (977 bytes) - added by ryan 6 years ago.
9952.4.patch (5.0 KB) - added by azaozz 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 @ryan6 years ago

  • Priority changed from normal to high

comment:2 @Denis-de-Bernardy6 years ago

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

comment:3 @ryan6 years ago

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.

comment:4 @Denis-de-Bernardy6 years ago

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.

comment:5 @azaozz6 years ago

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.

@azaozz6 years ago

@ryan6 years ago

comment:6 @ryan6 years ago

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.

@ryan6 years ago

comment:7 follow-up: @ryan6 years ago

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.

comment:8 in reply to: ↑ 7 @azaozz6 years ago

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) )


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.

comment:9 @ryan6 years ago

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

comment:10 @ryan6 years ago

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.

comment:11 @ryan6 years ago

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

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

comment:12 @ryan6 years ago

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.

comment:13 @ryan6 years ago

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

comment:14 @Denis-de-Bernardy6 years ago

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?

comment:15 @Denis-de-Bernardy6 years ago

err, sorry. was meaning:




but I'm sure you got the point.

comment:16 @azaozz6 years ago

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.

@ryan6 years ago

comment:17 @ryan6 years ago

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.

@azaozz6 years ago

comment:18 @azaozz6 years ago

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.

comment:19 @ryan6 years ago

Looks good to me.

comment:20 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.