Widgets not displayed if no $instance data is set

[johnnyb]

Widgets fail to render when $instance is empty. This is a change from pre-4.3 versions of WP.

Background: I often create single-use widgets that do one thing, so they don't need a title or anything. These widgets are created with empty form() and update() methods. After upgrading to 4.3 these widgets all stopped rendering.

I'm attaching a simple plugin with 2 widgets, "Bad Widget" and "Good Widget." On a stock install of 4.3 running the Twenty Fifteen theme Bad Widget fails to render.

Expected resolution: The pre-4.3 behaviour, where widgets would render even if there was no configuration for that particular widget instance.

[johnnyb]

Demo of a working, and non-working, widget.

[GregRoss]

I have a similar problem, I tracked it down to changeset 32602. In widgets.php, the display_callback function used to use:

if ( array_key_exists( $this->number, $instance ) ) {

but after the change it is now:

if ( isset( $instances[ $this->number ] ) ) {

This breaks if the instance data doesn't exist OR has been set to null (for example if you don't provide a return value in your widget's update override function because your storing the data somewhere else).

[dd32]

Moving for review, Seems that change in r32602 needs to be reverted.

@westonruter was there any specific reason for that change, other than it not being rather obvious why it was set like that?

[westonruter]

Replying to dd32:

Moving for review, Seems that change in r32602 needs to be reverted.

@westonruter was there any specific reason for that change, other than it not being rather obvious why it was set like that?

The reason for the change from array_key_exists() to using isset() was that I had found that array_key_exists() was not compatible with ArrayObject/ArrayIterator instances. Or actually, I think I had specifically found this for array_keys() and I assumed that it was also true for array_key_exists(). However, in actually testing it seems that using array_key_exists() works just fine: ​https://3v4l.org/vIFPF

And with testing, yes, it is specifically that array_keys() has this problem: ​https://3v4l.org/bdoq5

So yes, it looks like we can indeed go back to using array_key_exists() since it is compatible with both intrinsic arrays and array objects.

@johnnyb Nevertheless, I think it is a plugin error to return anything but an array from the update callback. If you look at WP_Widget::update_callback() in core, it calls the update method and is expecting its return value as being an array, which it then passes into the widget_update_callback filter. If there are other plugins filtering this value, then they would be generating PHP warnings/notices when encountering a null value. The default base WP_Widget::update() method is defined as returning an array as well, so all subclasses should also have methods that return arrays.

[GregRoss]

Replying to westonruter:

@johnnyb Nevertheless, I think it is a plugin error to return anything but an array from the update callback. If you look at WP_Widget::update_callback() in core, it calls the update method and is expecting its return value as being an array, which it then passes into the widget_update_callback filter. If there are other plugins filtering this value, then they would be generating PHP warnings/notices when encountering a null value. The default base WP_Widget::update() method is defined as returning an array as well, so all subclasses should also have methods that return arrays.

That documentation looks to be incorrect as if you look at the WP_Widget::update() inline documentation it specifically says you can return false from the call to indicate the settings should not be changed.

So any plugin filtering the value cannot assume it's an array and needs to handle Boolean as well.

I don't disagree that returning a null value is probably incorrect as per the intent of the functions, but it obvious does happen (I've already fixed my plugin to return an empty array instead).

Going back to the old behaviour seems to make the most sense as it doesn't have any downside by the looks of it.

[westonruter]

@GregRoss Good point on returning false. Perhaps then what we need to do is make this change to Core:

  /**
   * Filter a widget's settings before saving.
   *
   * Returning false will effectively short-circuit the widget's ability
   * to update settings.
   *
   * @since 2.8.0
   *
-  * @param array     $instance     The current widget instance's settings.
+  * @param array|false|null     $instance     The current widget instance's settings
   * @param array     $new_instance Array of new widget settings.
   * @param array     $old_instance Array of old widget settings.
   * @param WP_Widget $this         The current widget instance.
   */
  $instance = apply_filters( 'widget_update_callback', $instance, $new_instance, $old_instance, $this );
- if ( false !== $instance ) {
+ if ( false !== $instance && null !== $instance ) {
        $all_instances[$number] = $instance;
  }

Or perhaps instead of checking specifically for false or null, it should just check if is_array( $instance ).

[GregRoss]

@westonruter why limit it to arrays? The old code was type agnostic, false or *any* type of data worked. If the plugin author wanted to store the config in an object or a string, that was fine.

Simply reverting to array_key_exists() and then updating the inline docs to reflect the fact that *any* data type (including no data, aka null) may be returned from the update() callback and be passed in to the filter.

After all null could be a valid configuration set for a widget (aka use defaults).

[westonruter]

@GregRoss humm, Core is really expecting each widget instance to be represented as an array. If an update callback returned an object or a string, then I expect that a plugin could blow up when they try to amend the $instance array with additional data in the widget_update_callback filter (e.g. Jetpack's ​Widget Conditions). This filter is defined as passing around arrays, as is the WP_Widget::update() method defined as returning an array.

That being said, the change for this ticket can probably just be returning to array_key_exists(), but at the same time I think it is perilous to try returning anything other than an array (or false) from an update method.

[GregRoss]

@westonruter I don't disagree that it's dangerous, just that it worked previously :)

I agree reverting to array_key_exists() makes the most sense and restores the previous behaviour.

[westonruter]

OK, 33442.diff​ reverts the r32602 change from using array_key_exists() to using isset(), which turns out to not be necessary (although I still think that plugins should be told they're doing_it_wrong if their update() methods return null, or anything other than false/array/ArrayIterator).

[westonruter]

In 33696:

Widgets: Switch back to using array_key_exists() instead of isset() for widget instance existence check.

Reverts unnecessary change in [32602] since array_key_exists() does actually work with ArrayIterator objects.

See #32474.
Fixes #33442.

[westonruter]

#33489 was marked as a duplicate.

[netweb]

Reopening for 4.3 branch, r33696 needs committing to the 4.3 branch

[westonruter]

In 33721:

Widgets: Switch back to using array_key_exists() instead of isset() for widget instance existence check.

Reverts unnecessary change in [32602] since array_key_exists() does actually work with ArrayIterator objects.

Merges [33696] to the 4.3 branch.
See #32474.
Fixes #33442 for the 4.3 branch.