WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#10013 closed defect (bug) (fixed)

poorly documented widget args are confusing

Reported by: Denis-de-Bernardy Owned by: azaozz
Milestone: 2.8 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: dev-feedback
Focuses: Cc:

Description

See attached patch. looking into the other two useful hooks.

Attachments (4)

10013.diff (666 bytes) - added by Denis-de-Bernardy 5 years ago.
10013-display-update.diff (2.3 KB) - added by Denis-de-Bernardy 5 years ago.
10013.2.diff (2.5 KB) - added by Denis-de-Bernardy 5 years ago.
10013.3.diff (2.7 KB) - added by Denis-de-Bernardy 5 years ago.
slightly different approach for the noform argument

Download all attachments as: .zip

Change History (13)

Denis-de-Bernardy5 years ago

comment:1 Denis-de-Bernardy5 years ago

dunno if this has any impact, but... _get_form_callback() returns null, always, if the form is overridden.

comment:2 follow-up: Denis-de-Bernardy5 years ago

  • Severity changed from major to blocker

The widget_update_callback and widget_form_callback hooks are missing a callback.

Their main purpose is to override the normal callback, but the thing is, it's often used to manage widget contexts. In this case, it's necessary to know which sidebar we're in. On occasion, the context is irrelevant (inline widgets used in posts, widgets appended to feed contents) and should be ignored outright.

comment:3 Denis-de-Bernardy5 years ago

missing an argument even.

comment:4 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback added

this line:

			if ( 'noform' !== $return )
				do_action_ref_array( 'in_widget_form', array(&$this) ); // add extra fields in the widget form

is very questionable. it means you cannot add extra fields to a form that has no fields in the first place.

comment:5 Denis-de-Bernardy5 years ago

widget_update_callback hook has inconsistent args.

$instance = apply_filters('widget_update_callback', $instance, $new_instance, $this);

should be something like:

$instance = apply_filters('widget_update_callback', $instance, $new_instance, $old_instance, $this);

else you don't have the same args as in the update() method for WP_Widgets

comment:6 in reply to: ↑ 2 ; follow-up: azaozz5 years ago

  • Priority changed from high to normal
  • Severity changed from blocker to normal

Replying to Denis-de-Bernardy:

The widget_update_callback and widget_form_callback hooks are missing a callback.

Their main purpose is to override the normal callback ...

No, overriding any of the widget's callbacks would usually break the widget. Their purpose is to allow post-processing of the new settings when saving or adding form fields to the existing widget form. Also the sidebar ID is usually available in the POST request.

this line: if ( 'noform' !== $return ) ... is very questionable. it means you cannot add extra fields to a form that has no fields in the first place.

Yes, you cannot add form fields to a widget that doesn't have a form... This is only used to show (or hide) the Save button on the widgets screen.

widget_update_callback hook has inconsistent args.

Again this is not meant to allow replacing of the callback, it is there to allow extra processing of the widget's settings before saving them.

I'm actually wondering how to discourage plugins that access widgets internal functions directly. Each widget is like a small (or not so small) plugin and is meant to be a separate entity that uses the API. Using (or changing) internal widget functionality is not a good practice.

Of course this affects mostly the default widgets. Perhaps can mark them as "Private" as they may change from version to version. If a plugin wants to use default widget functionality, it's a lot better to copy the code, change it and register it as a separate widget. That will also ensure future compatibility. It can even unregister the default widget although that would mean less options for the users.

Changing to normal severity only for the if ( 'noform' !== $return ) part. Reworking this might help a few plugins.

comment:7 in reply to: ↑ 6 Denis-de-Bernardy5 years ago

Replying to azaozz:

this line: if ( 'noform' !== $return ) ... is very questionable. it means you cannot add extra fields to a form that has no fields in the first place.

Yes, you cannot add form fields to a widget that doesn't have a form... This is only used to show (or hide) the Save button on the widgets screen.

widget_update_callback hook has inconsistent args.

Again this is not meant to allow replacing of the callback, it is there to allow extra processing of the widget's settings before saving them.

Here's what I used to do, and what I'm looking into doing instead, so you get a better picture... The plugin adds context handling to widgets.

Until now, I'd change the two/three wp_registered_widgets arrays directly, and replace their callbacks with my own. In my display callback, I'd check the context, and proceed with the widget's normal callback if it was valid. In the form callback, I'd output the widget's callback, then my own. In the update callback, I'd grab my own variables, and pass it on to the widget.

It works fine until users start dropping widgets. The contexts array where I store it all then grows, and grows, and grows. It would seem a lot more elegant to leave the internal arrays untouched, and used the three new filters to do the same, storing the contexts in the widget's option directly.

I'm actually wondering how to discourage plugins that access widgets internal functions directly. Each widget is like a small (or not so small) plugin and is meant to be a separate entity that uses the API. Using (or changing) internal widget functionality is not a good practice.

That would depend on what your plugin tries to do... ;-) Frankly, I fail to see the point in these hooks, if they don't allow to do what I'm using them for.

Anyway, this one:

$instance = apply_filters('widget_update_callback', $instance, $new_instance, $this);

Should almost certainly be:

$instance = apply_filters('widget_update_callback', $instance, $new_instance, $old_instance, $this);

... so as to allow to save options added to the form.

And this one:

if ( 'noform' !== $return )
	do_action_ref_array( 'in_widget_form', array(&$this) ); // add extra fields in the widget form

Should be something like:

// add extra fields in the widget form, unset return if you add any to a widget that has none
do_action_ref_array( 'in_widget_form', array(&$this, &$return, $sidebar_args) );

comment:8 Denis-de-Bernardy5 years ago

in_widget_form could use the $instance stuff as an argument too...

Denis-de-Bernardy5 years ago

Denis-de-Bernardy5 years ago

slightly different approach for the noform argument

comment:9 azaozz5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [11510]) Improve widgets hooks, props Denis-de-Bernardy, fixes #10013

Note: See TracTickets for help on using tickets.