WordPress / wp-autoupdates

Feature plugin building a UI for opting-in to plugin, theme, and core auto-updates.
https://wordpress.org/plugins/wp-autoupdates/
GNU General Public License v2.0
56 stars 18 forks source link

Action and Filter Recommendations #63

Open ronalfy opened 4 years ago

ronalfy commented 4 years ago

This is a proposal for actions and filters that can be included in the auto-update featured plugin.

The goal is to allow third-party update manager plugins to still be able to offer services and compatibility with the featured plugin.

Filter when retrieving an option.

Goal: Allow third-party update manager plugins to reflect correctly in the plugin/theme area which plugins are under auto-update.

Example:

$wp_auto_update_themes = get_site_option( 'wp_auto_update_themes', array() );
/**
 * Retrieve auto-update enabled themes.
 *
 * Retrieve auto-update enabled themes..
 *
 * @since 1.0.0
 *
 * @param array  $wp_auto_update_themes Array of auto-enable themes.
 */
$wp_auto_update_themes = apply_filters( 'wp_auto_update_themes_enabled', $wp_auto_update_themes );

Action Before Saving Option

Goal: Allow third-parties to update their own plugin/theme list of updated plugins/themes.

Example:

/**
 * Allow others to hook into saving action.
 *
 * Allow third-party plugin/theme data retrieval.
 *
 * @since 1.0.0
 *
 * @param array $new_autoupdated_plugins List of auto-updated plugins.
 */
do_action( 'wp_auto_update_plugins_save_pre', $new_autoupdated_plugins );
update_site_option( 'wp_auto_update_plugins', $new_autoupdated_plugins );

I'm sure there will be more filters/actions as testing continues, but these basics should help third-parties.

pbiron commented 4 years ago

Thanx for the issue @ronalfy.

The proposed new action makes sense (and I suppose we'd also want a similar one for themes, right?).

As for the proposed new filter (again, we'd want the same for themes, right?), can you explain how it would be any different than using core's existing siteoption{$option}?

That is, how would hooking into your proposed wp_auto_update_themes_enabled filter be any different than hooking into core's site_option_wp_auto_update_themes?

ronalfy commented 4 years ago

Good point @pbiron. I guess it would make it more explicit. I always found it a bit hacky IMO to hook into filters (options) that way, but if that's the preferred approach, we can skip it.

pbiron commented 4 years ago

I guess it would make it more explicit.

I figured that was your thinking. I'm not against the idea, that was just my initial thought upon reading the issue for the first time.

Of course, having a separate filter provides a level of abstraction, so that plugin authors don't need to know that the plugins/themes that will auto-update is stored in a site option. So, there might be some benefit to the additional filter.

Be sure to raise this question during the next meeting (in case I forget).

TimothyBJacobs commented 4 years ago

Are there any other places in core where we have these more explicit actions? I agree with @pbiron that I think using core's existing filters and options makes sense.

pbiron commented 4 years ago

Are there any other places in core where we have these more explicit actions?

One came to mind, although it's not quite the same. It has to do with whether the admin bar shows on the front-end.

In that case, there is a call to get_user_option( "show_admin_bar_front", $user ), which applies get_useroption{$option} (just as get_site_option() applies site_option_{$option}). The value returned by that filter is then run through the show_admin_bar filter.

So, the show_admin_bar filter is pretty close to the new filters asked for in this issue, in that one need not know that the default value passed to it comes from a user option.

TimothyBJacobs commented 4 years ago

Interesting find. I think part of the reasoning for that is because of the show_admin_bar() function and $show_admin_bar global. If those are set, then the user's preference isn't taken into account. So the user option filter wouldn't be sufficient to override showing/hiding the admin bar.

Another example could be the get_bloginfo filters, but those are only applied when using the display filter, which is not the default.

There are also the filters for site_url and home_url, but the url value there is transformed first by the function, and additional context is passed to the filters.

Searching through the code base for get_option calls, I think the closest example would be the get_stylesheet and get_template functions.

For the actions, I think a similar example would be the personal_options_update/edit_user_profile_update and user_profile_update_errors actions. Though in that case, I believe the expected use case is for plugins to save their custom form fields. I don't think keeping records in sync would be a good use for that action, particularly because any modifications to the user object via wp_insert_user calls would not be seen, so inconsistencies would occur.

I think a similar logic applies here. If a 3rd party wants to maintain a copy of the list of plugins with auto-updates enabled in a separate location, using the pre hook here wouldn't be safe, since any direct modifications to the option wouldn't be tracked.


So from my perspective, I think adding the action would make sense if we wanted to provide a way for plugin authors to save additional custom data when the auto update list is updated. With the current UI, save is triggered by a link, I don't see how plugin authors would be able to meaningfully hook in there.

For filters, I think if we meaningfully transformed the values, or were able to provide some additional context about how the list of auto updates were being used, and it would make sense for a plugin to alter results based on that context, then adding filters on the return value would make more sense to me.

Just my 2 cents though.

pbiron commented 4 years ago

Interesting find.

As in the ancient curse: May you live in interesting times :-)

I think part of the reasoning for that is because of the show_admin_bar() function and $show_admin_bar global.

Exactly. I bumped into that example recently while trying to figure why the admin bar was showing when it shouldn't, even tho I'd hooked into both get_user_option_show_admin_bar_front and show_admin_bar...only to find some other plugin was calling show_admin_bar() :-(

azaozz commented 4 years ago

Thinking that for the stated purpose, the site_option_{$option} https://developer.wordpress.org/reference/hooks/site_option_option/ and the update_site_option_{$option}, https://developer.wordpress.org/reference/hooks/update_site_option_option/ filters are sufficient. Also see https://developer.wordpress.org/?s=site_option_%7B%24option%7D.

Not sure if new/more specific filters are needed.