WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#50280 closed enhancement (fixed)

Enable auto-updates shows for plugins with no support

Reported by: elrae Owned by: audrasjb
Milestone: 5.5 Priority: normal
Severity: normal Version: trunk
Component: Upgrade/Install Keywords: has-patch dev-feedback needs-dev-note commit
Focuses: docs, administration Cc:

Description

I installed / activated the WordPress Auto-updates plugin (Version 0.8.1), and it shows "Enable auto-updates" and allows me to click that and enable auto-updates for ANY plugin. There are some custom written plugins I have in there that have no auto-update capability, so those plugins will never be updated. This could provide a false sense of security to users. There should be some opt-in mechanism for plugin authors to specify their plugin is even capable of auto-updates (same with themes).

Attachments (6)

50280.diff (4.9 KB) - added by audrasjb 5 weeks ago.
First workaround for plugins screen and multisite themes screen
Capture d’écran 2020-06-02 à 23.54.35.png (182.9 KB) - added by audrasjb 5 weeks ago.
Works fine on plugins screen (see comment below)
50280.1.diff (6.8 KB) - added by azaozz 5 weeks ago.
50280.2.diff (6.8 KB) - added by azaozz 5 weeks ago.
50280.3.diff (7.0 KB) - added by azaozz 4 weeks ago.
50280.4.diff (7.1 KB) - added by azaozz 3 weeks ago.

Download all attachments as: .zip

Change History (25)

#1 @pbiron
5 weeks ago

Thanx for the ticket.

I'll answer in relation to how this will work in core once 5.5.0 is released (or right now if you're using the WordPress Beta Tester set to "Bleeding Edge"), although the same applies to the feature plugin.

I'm not sure how this feature would provide "false sense of security". All that enabling auto-updates does is that if an update becomes available it tells the auto-updater to apply that update automatically. If no updates ever become available, then nothing will happen.

There has been some discussion during the development of the feature plugin about adding hooks that would allow the Enable/Disable auto-updates to be replaced for specific plugins/themes (e.g., with plain text such as "Auto-updates controlled by Plugin X"). For some of that discussion, see the Interoperability between existing and new filters and constants issue on the feature plugin repo.

Unfortunately, it's not an easy problem to solve in the general case. But this ticket is certainly a good place to continue that discussion from the feature plugin, for how it might be solved in core.

#2 @SergeyBiryukov
5 weeks ago

  • Version set to trunk

Related: #32101, #23318, #45058, #14179.

@audrasjb
5 weeks ago

First workaround for plugins screen and multisite themes screen

@audrasjb
5 weeks ago

Works fine on plugins screen (see comment below)

#3 follow-up: @audrasjb
5 weeks ago

  • Component changed from Plugins to Upgrade/Install
  • Focuses docs added
  • Keywords has-patch dev-feedback needs-testing added
  • Milestone changed from Awaiting Review to 5.5
  • Owner set to audrasjb
  • Status changed from new to accepted
  • Type changed from defect (bug) to enhancement

Moving to milestone 5.5 as per today’s core-auto-updates meeting.
We agreed to provide a filter to allow plugin authors and WordPress developers to filter the content of the auto-updates column.

In 50280.diff, I started with Plugins screen and Multisite Themes screen which are pretty similar, just to validate the filter proposal (name, content of the filter, overall approach).

I tested those filters using the following code:

<?php
function wporg_auto_update_plugin_column( $plugin_auto_update_content, $plugin_file, $plugin_data, $status ) {
        if ( 'hello.php' === $plugin_file ) {
                $plugin_auto_update_content = array( '<p>This plugin doesn’t need auto-updates.' );
        }
        return $plugin_auto_update_content;
}
add_filter( 'plugin_auto_update_content', 'wporg_auto_update_plugin_column', 10, 4 );

It works fine. See the above screenshot for reference.

Ping @azaozz for review.
I’ll add a comment later about the single site Themes screen, I have to test my approach of this screen before sending a full patch.

Cheers,
Jb

@azaozz
5 weeks ago

#4 in reply to: ↑ 3 @azaozz
5 weeks ago

Replying to audrasjb:

Looks good. As discussed in Slack, seems better to filter the HTML as one string, not as array of strings.

In 50280.1.diff:

  • Based on 50280.diff.
  • Filter the auto-update setting HTML as string.
  • Add a filter that allows plugins and themes to override/replace the template for the settings link on the Themes screen.
  • Change the var and filter names to be (perhaps) more descriptive.

#5 follow-up: @audrasjb
5 weeks ago

  • Keywords needs-testing removed

50280.1.diff is a way better than my first patch.
Also, I was unsure about how to handle the Themes screen using the same filter used on Multisite Theme, but it makes sense to use a different one, as this screen is really specific.

👍

#6 in reply to: ↑ 5 ; follow-up: @azaozz
5 weeks ago

Replying to audrasjb:

50280.1.diff is a way better than my first patch.

Thanks! :)

I was unsure about how to handle the Themes screen using the same filter used on Multisite Theme, but it makes sense to use a different one, as this screen is really specific.

Yes, has to have a different filter as the list table needs HTML, but the Themes screen needs a template.

I'm having some second thoughts on output_theme_auto_update_setting_template though. It is the cleaner way of doing this, but that filter acts as both a filter and an action, which is unusual/may be a bit confusing. Also when more than one plugin is using the same filter, the last plugin to hook there wins, in this case the first plugin wins. Perhaps would be better to filter the template code as string after all, similarly to how the HTML filters work.

#7 in reply to: ↑ 6 ; follow-up: @pbiron
5 weeks ago

Replying to azaozz:

I'm having some second thoughts on output_theme_auto_update_setting_template though. It is the cleaner way of doing this, but that filter acts as both a filter and an action, which is unusual/may be a bit confusing. Also when more than one plugin is using the same filter, the last plugin to hook there wins, in this case the first plugin wins. Perhaps would be better to filter the template code as string after all, similarly to how the HTML filters work.

Does this mean that the intention is that those hooking into it are supposed to echo 'xyz'; return true;

If so, then yes, that is likely to cause developer confusion. The DocBlock could be improved to make that more clear, but it is very unusual that a filter directly results in output.

Last edited 5 weeks ago by pbiron (previous) (diff)

@azaozz
5 weeks ago

#8 in reply to: ↑ 7 @azaozz
5 weeks ago

Replying to pbiron:

Does this mean that the intention is that those hooking into it are supposed to echo 'xyz'; return true;

Yes, with 50280.1.diff this would be harder to understand/use.

In 50280.2.diff: same as 50280.1.diff but introduces wp_theme_auto_update_setting_template() that filters and returns the HTML template for the auto-update setting link in the theme overlay. The filter there works as any other HTML filter in WP. Last plugin to use it "wins".

#9 follow-up: @audrasjb
4 weeks ago

Looks great to me, I think this is the best we can offer to plugin authors and developers on that screen.

@azaozz
4 weeks ago

#10 in reply to: ↑ 9 @azaozz
4 weeks ago

In 50280.3.diff: refresh after [47910].

Replying to audrasjb:
Yes, thinking this is ready. @pbiron does it look good on your end?

#11 @pbiron
4 weeks ago

Let me finish up something else I'm in the middle of and then I'll give 50280.3.diff a test

#12 @pbiron
4 weeks ago

Functionally, looks good.

A couple of things, however.

The documentation for theme_auto_update_setting_template should give more "guidance" about what the callback should return (unclear to me right now whether documentation here means the DocBlock, the plugins handbook or a dev-note...certainly the last, but probably the 1st and 2nd as well).

For example, suppose a plugin manages the auto-update setting for 2 different themes. The callback for theme_auto_update_setting_html (the multisite list table) is pretty straightforward:

add_filter(
        'theme_auto_update_setting_html',
        function( $html, $stylesheet, $theme ) {
                if ( in_array( $stylesheet, array( 'twentynineteen', 'twentytwenty' ), true ) ) {
                        return 'this is a test';
                }

                return $html;
        },
        10,
        3
);

It took me a little while to figure out how to do the equivalent for theme_auto_update_setting_template:

  1. the plugin author needs to know that they probably want to test against data.id (where the callback for theme_auto_update_setting_html is explicitly passed $stylesheet...I had to look at the source of wp-admin/js/theme.js to figure out what to test against)
  2. the callback needs to always include the original template in their return value, as an "else" when their test fails (i.e., it's not one of the themes they are managing auto-updates for), otherwise there is no auto-update link for the themes they are not managing

That is, something like:

add_filter(
        'theme_auto_update_setting_template',
        function( $template ) {
                return <<<EOF
<# if ( [ 'twentynineteen', 'twentytwenty' ].includes ( data.id ) ) { #>
<p>this is a test</p>
<# } else { #>
{$template}
<# } #>
EOF;
        }
);

The other thing is that if the callback for theme_auto_update_setting_template returns something that is an "illegal" template (i.e., contains backbone or JS errors) then the Theme Details modal doesn't open for any theme!!, For example,

add_filter(
        'theme_auto_update_setting_template',
        function( $template ) {
// note that I forgot the "#" after the opening "< if..."
                return <<<EOF
< if ( [ 'twentynineteen', 'twentytwenty' ].includes ( data.id ) ) { #>
<p>this is a test</p>
<# } else { #>
{$template}
<# } #>
EOF;
        }
);

Not sure there's anything we can do about that, just pointing out that developers will need to be reminded that the template they return from theme_auto_update_setting_template has "global" consequences, whereas the returned HTML from theme_auto_update_setting_html only affects specific rows in the list table.

#13 @pbiron
4 weeks ago

  • Keywords needs-dev-note added

#14 follow-up: @azaozz
4 weeks ago

should give more "guidance" about what the callback should return
...
Not sure there's anything we can do about that, just pointing out that developers will need to be reminded that the template they return from theme_auto_update_setting_template has "global" consequences

Yeah, would be nice to have some more documentation and have a link in the docblock to it. At the same time the plugin authors are able to see what is being filtered, so thinking an expanded docblock about how js templates work in Backbone is not needed? IMHO it is enough to point out that the filtered string is a js template, there is quite a bit of related code in the source around there that can be copied and/or studied if needed.

#15 in reply to: ↑ 14 @pbiron
4 weeks ago

Replying to azaozz:

Yeah, would be nice to have some more documentation and have a link in the docblock to it. At the same time the plugin authors are able to see what is being filtered, so thinking an expanded docblock about how js templates work in Backbone is not needed? IMHO it is enough to point out that the filtered string is a js template, there is quite a bit of related code in the source around there that can be copied and/or studied if needed.

Agreed that there is no need to explain how templates, per se, work. Something about the data object used by the (default) template would be helpful, tho.

@azaozz
3 weeks ago

#16 @azaozz
3 weeks ago

In 50280.4.diff: add a bit more inline docs pointing to where to get info about the data js object's properties.

#17 @pbiron
3 weeks ago

  • Keywords commit added

50280.4.diff looks good.

#18 @azaozz
3 weeks ago

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

In 48077:

Plugins and Themes Auto-Updates: allow overriding of the HTML for the auto-update setting link. This will let plugins show better/specific information when they are overriding the auto-update settings, for example "Updates are managed by ... plugin".

Introduces: theme_auto_update_setting_html, plugin_auto_update_setting_html, and theme_auto_update_setting_template filters.

Props audrasjb, pbiron, azaozz.
Fixes #50280.

#19 @azaozz
3 weeks ago

In 48078:

Fix empty line phpcs error.

See #50280.

Note: See TracTickets for help on using tickets.