Interoperability between existing and new filters and constants

[azaozz]

Seems this has been discussed at different times/places but maybe a good idea to gather it in one place.

What are the expected uses of the combination of: WP_DISABLE_PLUGINS_AUTO_UPDATE constant, and wp_plugins_auto_update_enabled filter, and the theme's equivalent WP_DISABLE_THEMES_AUTO_UPDATE and wp_themes_auto_update_enabled.

Also, as far as I see the auto_update_plugin (old/existing) filter overrides both WP_DISABLE_PLUGINS_AUTO_UPDATE and wp_plugins_auto_update_enabled making the UI "wrong". Same for auto_update_theme.

[pbiron]

Also, as far as I see the auto_update_plugin overrides both WP_DISABLE_PLUGINS_AUTO_UPDATE and wp_plugins_auto_update_enabled making the UI "wrong". Same for auto_update_theme.

I'll answer this one first, as I think doing so will make answering your other question easier.

The way the feature plugin works is precisely by hooking into auto_update_{plugin|theme} and checking whether:

• the plugin/theme to be auto-updated is in the site option that stores the plugins/themes for which auto-updates are enabled
• whether plugin/theme auto-updates are allowed, in general

If both of those checks are true, then it returns true from auto_update_{plugin|theme}; otherwise it returns the value passed to that filter.

The 2nd check actually consults the WP_DISABLE_{PLUGINS|THEMES}_AUTO_UPDATE constants and the wp_{plugins|themes}_auto_update_enabled filters. As far as the feature plugin is concerned, auto_update_{plugin|theme} does not override the constants/filters...it actually uses them; so I'm sure how the UI is "wrong".

I'm currently working on the patch for the merge into core and my current thinking is to do the above checks in directly in WP_Automatic_Updater::should_update() as part of setting the "default" value passed to the filter. An alternative would be to have core hook into auto_update_{plugin|theme} with a priority < 10 so that other plugins hooking into it with the default priority of 10 would "win". I would appreciate your thoughts on that question.

What are the expected uses of the combination of: WP_DISABLE_PLUGINS_AUTO_UPDATE constant, and wp_plugins_auto_update_enabled filter, and the theme's equivalent WP_DISABLE_THEMES_AUTO_UPDATE and wp_themes_auto_update_enabled.

In addition to the use above, the constants and new filters also control whether the UI appears at all. That is, if WP_DISABLE_{PLUGINS|THEMES}_AUTO_UPDATE is defined and false or wp_{plugins|themes}_auto_update_enabled returns false, then the UI is for enabling/disable auto-updates for specific plugins/themes is not shown.

The reason the constants filters are check even when the UI isn't displayed is to avoid the case where:

• at time T, the site owner enables the UI and sets plugin X to auto-update
• at time T + 1, the site owner disables the UI thinking that will disable auto-updates, even for those things they previously wanted to auto-update

Does that explanation help?

[pbiron]

I should note that I have not tested (not sure if others have) the case where the Updates API returns a true value in the autoupdate field of it's response (i.e., a "forced security pushes from the WordPress security team" in the wording of the support page you link to). But I think the "right thing" will happen.

[azaozz]

As far as the feature plugin is concerned, autoupdate{plugin|theme} does not override the constants/filters...it actually uses them; so I'm sure how the UI is "wrong".

Sorry, should have explained that better :)

With the plugin enabled, add add_filter( 'auto_update_plugin', '__return_true', 11 ); to another plugin (this can also be set for individual themes/plugins). Now all plugins should always be auto-updated. However the UI still shows some plugins as not set to auto-update and lets the user to enable/disable auto-updates. The same happens with add_filter( 'auto_update_plugin', '__return_false', 11 );, etc.

[azaozz]

The question about WP_DISABLE_PLUGINS_AUTO_UPDATE constant, and wp_plugins_auto_update_enabled filter is about the expected usage.

Both the filter and the constant do the same thing, with the filter overriding the constant. What is the use case to warrant that? It seems either the filter or the constant would be enough (with strong preference for the filter as it is "the WP way of doing things").

[azaozz]

the case where the Updates API returns a true value in the autoupdate field of it's response (i.e., a "forced security pushes from the WordPress security team"...)

Good catch :) Yeah, this should always update the plugin/theme but may be harder to implement in the plugin. Perhaps this can be added when merging to core.

[pbiron]

Sorry, should have explained that better :)

With the plugin enabled, add add_filter( 'auto_update_plugin', '__return_true', 11 ); to another plugin (this can also be set for individual themes/plugins). Now all plugins should always be auto-updated. However the UI still shows some plugins as not set to auto-update and lets the user to enable/disable auto-updates. The same happens with add_filter( 'auto_update_plugin', '__return_false', 11 );.

OK, now I get what you mean.

That has been discussed although not in great detail and no decision has been made about it.

For reference see https://github.com/audrasjb/wp-autoupdates/pull/10#issuecomment-598827167 (which is a comment I made on PR that, unfortunately, didn't get migrated from the old repo to the new). Do you have any thoughts on the idea put foreword there?

[pbiron]

The question about WP_DISABLE_PLUGINS_AUTO_UPDATE constant, and wp_plugins_auto_update_enabled filter is about the expected usage.

Both the filter and the constant do the same thing, with the filter overriding the constant. What is the use case to warrant that? It seems either the filter or the constant would be enough (with strong preference for the filter as it is "the WP way of doing things").

It's modeled directly on core's AUTOMATIC_UPDATER_DISABLED constant and automatic_updater_disabled filter.

See WP_Automatic_Updater::is_disabled().

[pbiron]

BTW, these are all great questions! Thanx for the careful consideration!

[knutsp]

his has been on my mind since this project started. The special thing with this featured plugin is that it builds upon functionality that is already in core, with constants and filters.

So when another plugin sets some or all plugins/themes to auto update, it UI should accept that, reflect that, and not offer to enable it or disable it, just say it's enabled or disabled (by a plugin).

That gives four variations in the auto updates column or box (simplified): Enabled (Disable action) Disabled (Enable action) Enabled Disabled

The above should be in place at merge or before target (5.5) release.

Details could be something for Site Health, if necessary to avoid confusion. This can wait.

[azaozz]

The special thing with this featured plugin is that it builds upon functionality that is already in core, with constants and filters.

Right, and it adds another "layer" of constants and filters to the already existing. This makes managing the feature pretty complicated and hard to do.

The main problem is that it adds UI where the user can decide to enable or disable auto-updates, but in some cases that UI doesn't reflect what actually happens. This can "misinform/mislead" the users and make the experience pretty bleak.

At the same time, as mentioned by @pbiron and @seb86 on https://github.com/audrasjb/wp-autoupdates/pull/10, it is not straightforward to fix. I don't see a good way to "check" the auto_update_plugin and auto_update_theme filters when displaying the UI as these filters depend on WP_Automatic_Updater data and may allow/block an update depending on third-party plugins that may be checking other conditions (i.e. today an update may be blocked, tomorrow it may be allowed).

As far as I see there are couple of thing possible to improve this:

• Make the UI "not definitive". Do not say "Auto-updates enabled" when they may not be. This string would probably be better as "Auto-updates enabled by default" or something similar.
• As @knutsp suggests above, add other "states" for autoupdates. Can be something like "Controlled by a plugin", or perhaps "Enabled/Disabled by [plugin-name]". Then another filter specifically for the UI so plugins can hook there.

I understand this will make the UI not-as-clear/somewhat more complex, but is a lot better than giving the wrong information to the users.

[pbiron]

At the same time, as mentioned by @pbiron and @seb86 on audrasjb/wp-autoupdates#10, it is not straightforward to fix. I don't see a good way to "check" the auto_update_plugin and auto_update_theme filters when displaying the UI as these filters depend on WP_Automatic_Updater data and may allow/block an update depending on third-party plugins that may be checking other conditions (i.e. today an update may be blocked, tomorrow it may be allowed).

It might be easier to do that once this is merged into core.

I'm close to being done with a first pass on the patch for the core merge. Once I'm pretty sure that patch is solid, I plan to experiment with some version of the approach I mentioned in that old PR to see how feasible it is. I'll be sure to report back here how that goes.

[pbiron]

I experimented with this a little bit over the weekend.

Where the content of the Automatic Updates column is output, we can do:

$item = (object) array_filter(
    $plugin_data,
    function( $key ) {
        // this array represents the standard fields returned by the Updates API.
        return in_array( $key, array( 'banners', 'banners_rtl', 'compatibility', 'icons', 'id', 'new_version', 'package', 'plugin', 'requires_php', 'slug', 'tested', 'url' ), true );
    },
    ARRAY_FILTER_USE_KEY
);
if ( ! isset( $item->new_version ) ) {
    // no update is available right now, so use the current version.
    $item->new_version = $plugin_data['Version'];
}

$update = apply_filters( 'auto_update_plugin', null, $item );
if ( null !== $update ) {
    // we can't tell which plugin is controlling the auto-update.
    echo  $update ? __( 'Auto-updates enabled by a plugin.' ) : __( 'Auto-updates disabled by a plugin.' );
} else {
// output the enable/disable links as is currently done.
}

@azaozz I was wrong yesterday when I told you on Slack that some of the info that would be returned by the Updates API were an update available is not in the results from get_plugin_data(): all of the standard info returned by the API is there. What the above would not get is any custom properties that a plugin added to the response from the API.

Using the above would still not 100% guarantee that what is displayed in the UI is what will happen in the future.

For example, any plugin hooking into auto_update_plugin that made their auto-update decision for a given plugin on $item->new_version or $item->some_custom_property_injected_into_the_API_response might return a different value from their callback when WP_Automatic_Updater::should_update() applies the filter.

Make the UI "not definitive". Do not say "Auto-updates enabled" when they may not be. This string would probably be better as "Auto-updates enabled by default" or something similar.

I think (with the addition of the above code) that is a reasonable way to "market" this.

[afragen]

Should we simply disable the display of the autoupdate column in the respective list table if auto_update_plugin or auto_update_theme is true?

Just trying to simplify some of the Slack discussion.

[pbiron]

Not sure that'd be the right thing to do, since those filters get passed info about a specific plugin/theme update. So, plugins that hook into them could (but are not required to) make the return value of their callback depend on the specific plugin/theme update.

Of course, a plugin that wanted to "take over" the UI could certain do add_filter( 'wp_{plugins|themes}_auto_update_enabled', '__return_false' ).

Which reminds me: given the slack discussion re: naming conventions, are the names of those filters clear enough that what they do is control whether the UI is displayed (and not whether auto-updates will happen)?

[afragen]

The naming would indicate more that the autoupdates happen and don’t convey that it’s only about the display.

[pbiron]

That's what I think as well. The names of those filters should probably change to something like wp_{plugins|themes}_auto_update_ui_enabled (maybe without the wp_ prefix when merged into core?).

But as @azaozz said on Slack, naming is hard! I'm totally open to suggestions!

As for the additional new filters we talked about on Slack during today's meeting, the intention of those was to control what gets displayed in the Automatic Updates column for a specific plugin/theme (in place of the current Enable/Disable links, if I was following the discussion correctly).

So, again, looking for more suggestions on what to name those additional new filters so that it properly conveys the intention.

I now have a couple of questions:

1. what information should be passed to those new filters? A that point, we have
   • the plugin/theme name and slug ($plugin_file in the case of plugins)
   • the $plugin_data and $theme object
   • the response from the Update API (if any), i.e., the entry from the update_{plugins|themes} transients
2. what is the callback expected to return?
   • a string with the name of the plugin that is controlling auto-updates for that plugin/theme (already translated, which would them be included within a string that we provide)
   • a complete string that we display as is (escaped, of course :-)

For number 1, my initial thought is that passing slug, $plugin_data/$theme object and API response (null if no update available), in that order would be best.

For number 2, my initial thought is that the return should be just the controlling plugin name