Widgets Disappear

[hakre]

The widgets page displays all available (registered) widgets. They can appear in three sections: Within the list of available widgets, within the list of Inactive Widgets and in the list of the sidebars.

The logic where such a widget is displayed is somehow broken. I can not precise this but what I experience on certain sites is the fact that widgets disappear. Somehow the available widgets list gets incomplete missing some widgets that are neither part of any sidebar nor incative.

I created a plugin to dig into the details and even did make a look on the widget listing code. I strongly suggest that the widget listing code is completely refactored because it's pretty akward. There are bugs in there that lead to dataloss, which is a strong signal that the code is overall not working properly.

I experienced it now with at least three sites otherwise I would not have taken the time to report it.

Please asign this ticket to the coder who is taking care for the widgets.

[hakre]

Additional info: The widget is available in wp_widget_factory but is not part of $wp_registered_widget_controls.

[hakre]

Looks like dataloss occurs because the widget logic is not properly implemented with it's functions. Mostly this is related to not properly sanitze data which leads to dataloss. I do not have any intentions to provide a patch, because this is so deep in core, a core-dev must take care of. As I already suggested I think this should be done by the original implementor.

Because the code is class based, this could benefit from the switch to php5.

In case you experience such problems on your blog and you do not want to wait until this is patched feel free to contact me, I'm here to help.

[hakre]

Additionally, having #9175 in core just helps tremendously with sugs bugs.

[azaozz]

Yes, the widgets API is doing a lot of things at the same time: implementing the new (class based) methods, staying backwards compatible, converting old settings when a widget has been upgraded to use the new methods, taking into account broken/badly coded widgets and trying to prevent them from breaking the rest.

Replying to hakre:

The widget is available in wp_widget_factory but is not part of $wp_registered_widget_controls.

So, the widget fails to initialize properly? Is that a specific widget or it can happen with any? If it's a specific one, can you attach the widget code here. Also will need the database dump of the widget's option(s).

[hakre]

Well, just take a look in the source: Does the widget takes care of the option data in any case? No it does not (well it does take care offering an interface for widget data to be accessed but that's all), fallback only works in one decent case.

I would assume that such errors were creeping in while integrating the multi widget functionality and then being lucky the momement it worked the first time. So I fully can understand your comment that there is much code that contains duplication and is very hard to read. That what I meant when I was suggesting to refactor the code at first sight.

[azaozz]

I'm not sure I follow... The new API stores and retrieves widget settings automatically. Or do you mean it doesn't try to convert all possible old settings for all existing widgets?

Before 2.8 widgets were saving their data as they see fit. It's besides the point to try to convert all these settings in core. When upgraded, the widgets can include couple of lines of code to read their own old settings and pass them to the new API.

If by refactoring you mean to throw out the support for old widgets, that would probably happen soon after 2.9 is released to give plugin authors plenty of time to migrate their code.

[hakre]

What I mean is that even there is old data (wrong formatted data) you still can register those widgets, even with the old data. That prevents from having widgets not displayed. So this must not be a compability issue, just make shure that at least all widgets are registered that are part of the factory. Currently this is not the case and I think this is the concrete bug.

Refactoring indeed is a good idea (I already suggested it) but for fixing that bug, it's not a need. The widgets _register function just only needs to take a bit more to register the widget in any case. Otherwise it will disappear.

[hakre]

Fix

[hakre]

The provided patch does catch the case that the assumption that a widgets settings must be multi-widget based only because of the case it's an array. This will ensure that a widget that is to be registered will be registered.

[hakre]

Same patch as patch-file (this is because I'm testing the XMLRPC interface as well, sorry for the fuzz)

[hakre]

Small code improvement this time.

[azaozz]

Replying to hakre:
Looking at your patch it seems the error it that some keys in the widget option are not numeric (would have been easier with db dump). If that's the case we should also check the data on saving. Can you verify that 10954-3.patch fixes it.

[scribu]

Related: #11160

[Denis-de-Bernardy]

Replying to azaozz:

Replying to hakre:
Looking at your patch it seems the error it that some keys in the widget option are not numeric (would have been easier with db dump). If that's the case we should also check the data on saving. Can you verify that 10954-3.patch fixes it.

I've seen occurrences of this too, and checking on save probably is the wrong approach. the core issue is when the option ends up looking like one or both of:

array('_multiwidget' => 1);

array('_multiwidget' => 1, 'foo' => 'bar');

you can then have a sidebar that "contains" a widget based on the sidebars_widgets option, but the widget's option got corrupted in a way or another -- during an upgrade, or a race condition in the widgets screen; no idea why.

whatever the cause, the widgets screen ends up not displaying the widget at all in the second case (not even in the available widgets), and there is no means to fix the issue short of deleting the option in the database.

10954-3.patch makes the widget re-appear in the available widgets as expected.

[azaozz]

Replying to Denis-de-Bernardy:

I've seen occurrences of this too, and checking on save probably is the wrong approach. the core issue is when the option ends up looking like one or both of:
array('_multiwidget' => 1);
array('_multiwidget' => 1, 'foo' => 'bar');

The first is a valid empty widget option, the '_multiwidget' => 1 gets unset when retrieving it. Filtering the non-numeric keys on save prevents the second one.

whatever the cause, the widgets screen ends up not displaying the widget at all in the second case (not even in the available widgets), and there is no means to fix the issue short of deleting the option in the database.

Fixing that is the purpose of this patch. Setting to needs-testing again as it's very late in the dev. cycle and this would need to be tested more (preferably with different themes too) if we want it in 2.9. Would be good to have perhaps 3-4 more "tested" responses.

[Denis-de-Bernardy]

I tested this in my own theme and with the default theme, and I honestly fail to see how things may differ with other themes.

1. force an:

update_option('widget_whatever', array('_multiwidget' => 1, 'foo' => 'bar'));

2. note that whichever widget it was no longer appears in the sidebar, nor shows in the available widgets list.

3. with the patch applies, it shows in the available widgets once again and things continue to work as they should.

Please highlight what could possibly need more testing to get this checked in?

[hakre]

I do not understand why everything is made so complicated to just fix it. In the current code, the first if checking for is numeric might not be the holy grail but a patch only needs to take care in times it fails. that ensures that at least every widget in the factory is displayed on screen. Fixed. That was what my patch is doing.

There isn't even much testing because a) not much has been changed and b) there is no complicated logic changes.

I know that this whole area has big deficies and I'm pretty sure that a refactored component needs a lot of testing but this is not what my patch was about. It was just about fixing.

And please don't touch a users data. As azzoz and others already pointed out, we just do not know what is saved in that option because it's an array.

To make the data structure / stored data more robust, we might consider about changing from numerical indezies to prefixed ones like:

$optiondata = array(
  'wp_bogusoptions' => 'MY SUPERPLUGIN in 2.5 MADE IT, YEAH!',
  'wp_widget-data-0' => array(/* widget options for widget number #1 */),
  'wp_widget-data-1' => array(/* widget options for widget number #2 */),
  'wp_widget-data-2' => array(/* widget options for widget number #1 */),
  'widgetpluginsname' => array(/* some plugin did store stuff here for this widget-type */),
)

Just a suggestion. In case there are no options we must still use the fallback as it is intended right now with the current check for numerical values only.

[azaozz]

(In [12249]) Always register all widgets, props hakre, fixes #10954

[azaozz]

Replying to hakre:

I do not understand why everything is made so complicated...

The problem is that a lot of plugins and themes add widgets and many use various hacks or access the low level code directly, so the chance of something breaking is high with every change.