Opened 9 years ago
Closed 9 years ago
#33442 closed defect (bug) (fixed)
Widgets not displayed if no $instance data is set
Reported by: | johnnyb | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.3.1 | Priority: | normal |
Severity: | normal | Version: | 4.3 |
Component: | Widgets | Keywords: | has-patch commit fixed-major |
Focuses: | Cc: |
Description
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.
Attachments (2)
Change History (17)
#1
@
9 years ago
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).
#2
follow-up:
↓ 3
@
9 years ago
- Milestone changed from Awaiting Review to 4.3.1
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?
#3
in reply to:
↑ 2
;
follow-up:
↓ 4
@
9 years ago
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.
#4
in reply to:
↑ 3
@
9 years ago
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 atWP_Widget::update_callback()
in core, it calls theupdate
method and is expecting its return value as being an array, which it then passes into thewidget_update_callback
filter. If there are other plugins filtering this value, then they would be generating PHP warnings/notices when encountering anull
value. The default baseWP_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.
#5
@
9 years ago
@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 )
.
#6
@
9 years ago
@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).
#7
follow-up:
↓ 8
@
9 years ago
@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.
#8
in reply to:
↑ 7
@
9 years ago
@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.
#9
@
9 years ago
- Keywords has-patch added
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
).
#11
@
9 years ago
- Owner set to westonruter
- Resolution set to fixed
- Status changed from new to closed
In 33696:
Demo of a working, and non-working, widget.