Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Action and Filter Recommendations #63

Open
ronalfy opened this issue Apr 5, 2020 · 8 comments
Open

Action and Filter Recommendations #63

ronalfy opened this issue Apr 5, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@ronalfy
Copy link

ronalfy commented Apr 5, 2020

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 pbiron added the enhancement New feature or request label Apr 5, 2020
@pbiron
Copy link
Contributor

pbiron commented Apr 5, 2020

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 site_option_{$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
Copy link
Author

ronalfy commented Apr 5, 2020

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
Copy link
Contributor

pbiron commented Apr 5, 2020

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
Copy link
Member

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
Copy link
Contributor

pbiron commented Apr 15, 2020

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_user_option_{$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
Copy link
Member

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
Copy link
Contributor

pbiron commented Apr 15, 2020

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
Copy link
Contributor

azaozz commented May 7, 2020

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants