Opened 11 years ago
Closed 11 years ago
#9952 closed defect (bug) (fixed)
Unregistering default widgets is not backwards compatible
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.8 | Priority: | high |
Severity: | normal | Version: | 2.8 |
Component: | Widgets | Keywords: | widgets |
Focuses: | Cc: | ||
PR Number: |
Description
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.
Attachments (5)
Change History (25)
#3
@
11 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.
#4
@
11 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.
#5
@
11 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.
#6
@
11 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.
#7
follow-up:
↓ 8
@
11 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.
#8
in reply to:
↑ 7
@
11 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) ) 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.
#9
@
11 years ago
Replacing default widgets is commonplace. We have to keep supporting that, I think.
#10
@
11 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.
#11
@
11 years ago
add_action( 'widgets_init', array( &$this, '_register_widgets' ), 100 );
Duh, I forgot about this. I think I even wrote that. :-)
#12
@
11 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.
#13
@
11 years ago
But that won't play nicely with widgets using the new API that are added via plugin. Grrr.
#14
@
11 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?
#15
@
11 years ago
err, sorry. was meaning:
do_action('_widgets_init'); ... do_action('widgets_init');
but I'm sure you got the point.
#16
@
11 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.
#17
@
11 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.
#18
@
11 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.
Probably invalid. I may be wrong, but I believe the name of the registered widget has changed. As in, to WP_Widget_Pages.