Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50280 closed enhancement (fixed)

Enable auto-updates shows for plugins with no support

Reported by: elrae's profile elrae Owned by: audrasjb's profile audrasjb
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-dev-note
Focuses: docs, administration, multisite 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 (7)

50280.diff (4.9 KB) - added by audrasjb 4 years 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 4 years ago.
Works fine on plugins screen (see comment below)
50280.1.diff (6.8 KB) - added by azaozz 4 years ago.
50280.2.diff (6.8 KB) - added by azaozz 4 years ago.
50280.3.diff (7.0 KB) - added by azaozz 4 years ago.
50280.4.diff (7.1 KB) - added by azaozz 4 years ago.
50280.5.diff (1.7 KB) - added by whyisjake 4 years ago.

Download all attachments as: .zip

Change History (79)

#1 @pbiron
4 years 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
4 years ago

  • Version set to trunk

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

@audrasjb
4 years ago

First workaround for plugins screen and multisite themes screen

@audrasjb
4 years ago

Works fine on plugins screen (see comment below)

#3 follow-up: @audrasjb
4 years 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
4 years ago

#4 in reply to: ↑ 3 @azaozz
4 years 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
4 years 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
4 years 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
4 years 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 4 years ago by pbiron (previous) (diff)

@azaozz
4 years ago

#8 in reply to: ↑ 7 @azaozz
4 years 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 years ago

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

@azaozz
4 years ago

#10 in reply to: ↑ 9 @azaozz
4 years 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 years 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 years 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 years ago

  • Keywords needs-dev-note added

#14 follow-up: @azaozz
4 years 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 years 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
4 years ago

#16 @azaozz
4 years 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
4 years ago

  • Keywords commit added

50280.4.diff looks good.

#18 @azaozz
4 years 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
4 years ago

In 48078:

Fix empty line phpcs error.

See #50280.

#21 @StephenCronin
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I left a comment on the dev note just published, but will raise the concern here too.

I agree that this will raise a false sense of security for plugins/themes outside the repo.

The user can click the Enable Auto-updates link for these plugins/themes and it appears that it's been turned on. I guess it HAS been turned on, but of course, it will never actually update.

Plugin and theme authors can modify the action link as shown above, but there are tens of thousands of plugins and themes out there and not all authors are going to do that.

One of the most common security tips is to keep your plugins and themes updated. Users who 'turn on' auto updates for plugins/themes outside the repo will have a false sense of security, thinking that now they don't need to worry about keeping these plugins/themes up to date. They will stop looking for updates in the appropriate place. This will potentially open them to security threats.

Wouldn't it be safer to only show the Enable Auto-updates link for plugins/themes that have a slug that matches a plugin/theme in the respective repo and display "Auto updates unavailable" for any that don't match?

I'll reopen this to make sure it gets seen - apologies if that's the wrong thing to do!

#22 @whyisjake
4 years ago

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

Thanks for the comment @StephenCronin. Going to close this ticket out since Trac is for the workflow of core work. If you think this issue needs to get more attention, we could open a Trac ticket for the issue, bring it forward in the weekly dev chats, or, like you are doing now, carry the conversation forward on the dev note.

This ticket was mentioned in PR #431 on WordPress/wordpress-develop by dd32.


4 years ago
#23

This PR enhances the Auto-updates section for plugins to mark plugins that do not support automatic updates as not being available.

See https://core.trac.wordpress.org/ticket/50280#comment:23 for my concerns here.

Trac ticket: https://core.trac.wordpress.org/ticket/50280

#24 @dd32
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm going to re-open this, as as @StephenCronin has pointed out this is a very confusing situation for plugins that simply will never update and can be quite a concerning where a plugin that should never be updated shows as receiving updates.
Themes are potentially far more likely to be in that situation, and may give some site owners the misunderstanding that whomever built a custom theme for them is still supporting it.

The original purpose of the ticket still remains true, and the filter added in [48077] while beneficial, isn't actually super useful in these cases as the plugins in question are either:

  • Not going to include the filter, and will never update so never make use of it
  • Not going to be active, and as a result, not able to filter it
  • Be a custom plugin and there's really no reason a custom development agency should need to add an extra filter to disable this functionality, when we can do it automatically.

At least in the case of Plugins, WordPress knows if a plugin supports automatic updates, the plugins update check API returns all known plugins (It's how the 'Visit plugin site' changes to 'View details'). I believe most 3rd party update scripts also do something to fill in the required no_update transient key.

PR431 implements a basic unavailable warning here, although the UI may want a bit more work.
This can probably be punted to being worked on for 5.5.1, existing commits not withstanding.

Last edited 4 years ago by dd32 (previous) (diff)

#26 @audrasjb
4 years ago

  • Keywords commit removed

Thanks for the PR, @dd32.
Small feedback: I don't think we really need the extra dashicon in the UI.

I'll try to test some use cases later today, like at least various examples of update scripts.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#28 @TimothyBlynJacobs
4 years ago

So this requires 3rd party plugins to add auto-update-supported => true to the response object and the no update object to have their plugin available for auto updates?

#29 follow-up: @pbiron
4 years ago

While I understand the concerns raised by @stephencronin (and others), those concerns are really about updates in general, and not about auto-updates, per se. The presense of the Enable auto-updates link is not a promise that updates will ever be available for a particular plugin/theme: it merely allows site admins to opt-in to auto-updates if an update ever becomes available.

I realize that not all site admins know where all of the plugins installed on a site come from and which ones are/are not hooked into the Updates API (many won't even know what the Updates API is!). For example, they may be new to managing a particular site and someone else, who no longer works on the site may have installed the plugins.

But if a particular plugin is not hooked into the Updates API (either by being hosted in the .org repo, having it's own updater code, or because its updates are handled by yet another plugin such as GitHub Updater) and the unavailablity of updates is a concern, then core should do something to inform admins of that fact...independent of the auto-updates UI!

That is, suppose a site has the auto-updates UI disabled (and hence, all updates are manual), doesn't the fact that site admins regularly get notices to manually update plugins A, B & C give them a "false sense of security" that some day they will also get notices to manually update plugins X, Y & Z, even though, unbeknownst to such site admin, those notices will never be given for plugins X, Y & Z since those plugins are't hooked into the Updates API?

I actually support introducing a mechanism for detecting whether a plugin/theme is hooked into the Updates API, but I do not support tying that mechanism just to auto-updates! We are certainly not going to design and implement such a mechanism before 5.5 ships, and doing something last minute just for auto-updates is going to tie our hands about the eventual solution for updates in general, whether manual or auto.

#30 in reply to: ↑ 29 ; follow-up: @dd32
4 years ago

Replying to TimothyBlynJacobs:

So this requires 3rd party plugins to add auto-update-supported => true to the response object and the no update object to have their plugin available for auto updates?

No, it requires either a) That, or b) to set the appropriate no_update key in their Update Handler which triggers various other things in WordPress.

Replying to pbiron:

While I understand the concerns raised by @stephencronin (and others), those concerns are really about updates in general

I kind of agree with you that a lot of the concerns that people raise, are centered around update-support and not necessarily about Automatic updates.
However, it's IMHO short sighted to disconnect automatic updates from updates in general; they're very much reliant upon each other and tightly tied together.

The presense of the Enable auto-updates link is not a promise that updates will ever be available for a particular plugin/theme: it merely allows site admins to opt-in to auto-updates if an update ever becomes available.

And that's the issue I have here - It's not clear to WordPress users that that's the case.
While we can't be 100% sure that every plugin installed on a users site can be updated, we know which ones we can update, and that's an important distinction to make.

Take the Jetpack Manage interface for example, it makes it clear to the user that automatic updates are simply not viably supported. The option I've presented in the PR however would work with 3rd-party plugin update systems, where Jetpack doesn't.
https://i0.wp.com/cldup.com/Z5PzGv5e-v.png

But if a particular plugin is not hooked into the Updates API (either by being hosted in the .org repo, having it's own updater code, or because its updates are handled by yet another plugin such as GitHub Updater) and the unavailablity of updates is a concern, then core should do something to inform admins of that fact...independent of the auto-updates UI!

Yes, that's an option, but the lack of a different feature in WordPress doesn't mean that this feature should ignore a massive use-case of WordPress. If anything, as presented here, we can partially implement that exact missing feature by simply not enabling the UI for plugins we're not sure if they support updates or not. It's not that feature but rather just a side effect of exactly that.

It's complicated though, because some 3rd-party plugins that DO include an update handler won't be able to receive automatic updates or update notifications in general if they're disabled (Unless it's handled by yet another plugin).

#31 follow-up: @dd32
4 years ago

This morning I've updated PR 431 to take into account some feedback, and to take into account a few more scenario's that I feel are important to cover.

Remove the toggle ui when Automatic updates aren't available for the plugin
The PR originally showed 'Unavailable' but after the feedback, I decided to just remove the content which looks a fair bit cleaner IMHO. That was in a32001eb.

Automatic Updater is disabled
When using the constant AUTOMATIC_UPDATER_DISABLED or filter automatic_updater_disabled (see docs), the UI shouldn't be present to suggest that automatic updates can be enabled. 92ffb1da.
That has the effect of 100% disabling the automatic updates UI, the column isn't shown on the plugins table, etc.
Similar to using the plugins_auto_update_enabled filter which is rather badly and confusingly named (It doesn't enable plugin automatic updates, only toggles the UI to manage it)

Respect the auto_update_plugin filter
When the auto_update_plugin filter is in use (see docs), which we can detect, we should flag in the UI and override it so that it doesn't provide a false value. There's no point saying "Enable updates" if the filter is going to update anyway, and vice versa when a filter is forcibly disabling the updates.
You'll find that in a32001eb & 9363ecab.

That's resulted in this, where:

  • Akismet automatic updates are disabled via the filter
  • Hello Dolly automatic updates are enabled via the filter
  • Jetpack is not affected by a filter, and automatic updates are not enabled (Actually, it does have them enabled as Jetpack Manage is going to update it. Unfortunately it no longer filters the filter, but if this PR lands I'll ensure that happens)
  • Meeting calendar does not support automatic updates.

https://i0.wp.com/cldup.com/SKKhsV-luy.png

edit: I've updated this comment to reference the new commit hashes, and updated the image after I altered strings to consistently use 'auto-update' since that seems to have been the chosen text.

Last edited 4 years ago by dd32 (previous) (diff)

#32 @dd32
4 years ago

Additionally, I want to point out a use-case that could potentially be seen to go against my proposed PR.

Existing plugins which manage automatic updates may want to allow the UI on the plugins/themes UI's to toggle their settings.
For example, Jetpack Manage might automatically disable the autoupdates toggle in their UI when disabled in core, and enable it when enabled in core, etc.

With the changes in the PR, that use-case is a little more difficult, but could be achieved by simply not filtering the auto_update_plugin filter on the plugins.php screen and/or only filtering it on cron requests where it affects updates.

#33 @desrosj
4 years ago

Because there are commits associated with this ticket that will be released with 5.5, the remaining change suggestions and discussion should be moved to a new ticket eventually.

#35 in reply to: ↑ 30 @pbiron
4 years ago

Replying to dd32:

Replying to pbiron:

While I understand the concerns raised by @stephencronin (and others), those concerns are really about updates in general

I kind of agree with you that a lot of the concerns that people raise, are centered around update-support and not necessarily about Automatic updates.
However, it's IMHO short sighted to disconnect automatic updates from updates in general; they're very much reliant upon each other and tightly tied together.

Sorry if I wasn't clear, the thrust of my comment was that the concerns as raised (and the proposed solution in the PR) are "disconnecting automatic updates from updates in general" and should be solved at the level of "updates in general" rather than just for automatic updates.

For example, the PR proposes

$plugin_data = array_merge( (array) $plugin_info->no_update[ $plugin_file ], array( 'auto-update-supported' => true ), $plugin_data );

which I think "disconnects automatic updates from updates in general".

I would be much more comfortable with something more general, like:

$plugin_data = array_merge( (array) $plugin_info->no_update[ $plugin_file ], array( 'update-supported' => true ), $plugin_data );

Making the new key about "updates in general" rather than simply about "auto-updates" would then allow a future version of WP to show an indication in the list table that manual updates are also not supported (the design of such an indication TBD).

#36 in reply to: ↑ 31 ; follow-up: @pbiron
4 years ago

Replying to dd32:

Automatic Updater is disabled
When using the constant AUTOMATIC_UPDATER_DISABLED or filter automatic_updater_disabled (see docs), the UI shouldn't be present to suggest that automatic updates can be enabled. 92ffb1da.
That has the effect of 100% disabling the automatic updates UI, the column isn't shown on the plugins table, etc.
Similar to using the plugins_auto_update_enabled filter which is rather badly and confusingly named (It doesn't enable plugin automatic updates, only toggles the UI to manage it)

This is a very good catch!! and something that escaped us during the development of the feature plugin and in the prep for the core merge.

I would support getting the part of the PR for this into 5.5...if others thing it's not too late (with RC1 to be released in a few hours).

#37 @elrae
4 years ago

I just tested the patch as-is (knowing the param needs to be changed) and it appears to be working perfectly. It shows "enable auto updates" automatically for a plugin that it detected had an update that's custom. and then I added in the necessary array key for a plugin which didn't have an update available and gave it the 'auto-update-supported' => true and enable auto-updates showed up perfectly in my admin.

#38 in reply to: ↑ 36 @pbiron
4 years ago

Replying to pbiron:

Replying to dd32:

Automatic Updater is disabled
When using the constant AUTOMATIC_UPDATER_DISABLED or filter automatic_updater_disabled (see docs), the UI shouldn't be present to suggest that automatic updates can be enabled. 92ffb1da.
That has the effect of 100% disabling the automatic updates UI, the column isn't shown on the plugins table, etc.
Similar to using the plugins_auto_update_enabled filter which is rather badly and confusingly named (It doesn't enable plugin automatic updates, only toggles the UI to manage it)

This is a very good catch!! and something that escaped us during the development of the feature plugin and in the prep for the core merge.

I would support getting the part of the PR for this into 5.5...if others thing it's not too late (with RC1 to be released in a few hours).

Related #50798: I opened a separate ticket (w/ patch) for the point about AUTOMATIC_UPDATER_DISABLED.

#39 @SergeyBiryukov
4 years ago

In 48667:

Upgrade/Install: Disable the plugin/theme auto-updates UI if AUTOMATIC_UPDATER_DISABLED is defined and set as true.

Props pbiron, dd32.
Fixes #50798. See #50280.

#40 @whyisjake
4 years ago

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

In 48669:

Upgrade/Install: Enhance auto-updates to be disabled for plugins that don't suport updates.

This removes the toggle UI when updates aren't available. When plugins use the filter, the UI is updated to show that they are being controlled via code. And then removed entirely when not available.

See #50798.
Fixes #50280.

Props elrae, pbiron, SergeyBiryukov, audrasjb, azaozz, StephenCronin, whyisjake, dd32, TimothyBlynJacobs, desrosj.

#41 @paaljoachim
4 years ago

The comment: "This plugin doesn’t need auto-updates."
can be changed to "This plugin will not be auto updated."

@whyisjake
4 years ago

#42 @whyisjake
4 years ago

@paaljoachim where is that string?

#43 follow-up: @paaljoachim
4 years ago

Hey Jake. I noticed that @audrasjb posted it here. https://core.trac.wordpress.org/ticket/50280#comment:3

Version 0, edited 4 years ago by paaljoachim (next)

#44 in reply to: ↑ 43 @SergeyBiryukov
4 years ago

Replying to paaljoachim:

I noticed that @audrasjb posted it here. https://core.trac.wordpress.org/ticket/50280#comment:3

Just to clarify, that appears to be an example of using the core filters, that string is not actually in core.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#45 @paaljoachim
4 years ago

Ok. Thanks for the feedback, Sergey!

#46 follow-up: @whyisjake
4 years ago

In 48678:

Upgrade/Install: Swap auto-update-supported to update-supported in update checks.

Ensures backwards compatability with external updaters.

See #50280.
Props everyone-in-the-core-updates-channel.

#47 in reply to: ↑ 46 ; follow-up: @afragen
4 years ago

Replying to whyisjake:

In 48678:

Upgrade/Install: Swap auto-update-supported to update-supported in update checks.

Ensures backwards compatability with external updaters.

See #50280.
Props everyone-in-the-core-updates-channel.

For consistency in how the update transient data is designated; currently keys with more than one word use an underscore, not a hyphen, as a separator. new_version and banners_rtl as example.

#48 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address comment:47.

#49 @leemon
4 years ago

I downloaded and installed WP 5.5 RC1 and the "Enable auto-updates" links shows up in themes that are not in the repository/with no auto updates. Is this normal?

#50 @dd32
4 years ago

I downloaded and installed WP 5.5 RC1 and the "Enable auto-updates" links shows up in themes that are not in the repository/with no auto updates. Is this normal?

Correct, the changes made here in the last 24hrs only apply to Plugins, and not Themes.
To implement this for Themes requires a WordPress.org API change (which I'll work on in the next few hours) before it can be seen if it'll work there too.

This ticket was mentioned in PR #441 on WordPress/wordpress-develop by dd32.


4 years ago
#51

Similar to #431 this makes the same changes to Themes.

Trac ticket: https://core.trac.wordpress.org/ticket/50280

#53 @dd32
4 years ago

PR 441 implements the same logic for Themes as added for Plugins.

There's a larger catch for Themes though, the no_update update field wasn't previously available so it's extremely unlikely that existing 3rd-party Theme Updaters set that field.

The result of that is that for 3rd party themes that don't have an updated updater you can expect to only be able to enable auto-updates when the theme has an update actually available.

No new strings added, these are the same as in the previous commit.

#54 in reply to: ↑ 47 @dd32
4 years ago

Replying to afragen:

Replying to whyisjake:

In 48678:

Upgrade/Install: Swap auto-update-supported to update-supported in update checks.

Ensures backwards compatability with external updaters.

See #50280.
Props everyone-in-the-core-updates-channel.

For consistency in how the update transient data is designated; currently keys with more than one word use an underscore, not a hyphen, as a separator. new_version and banners_rtl as example.

I'm not sure that's actually something that needs doing, the parameter is only intended on being used internally by the loop it's in - It seems like this stems from the original misunderstanding of how that flag was being set.

3rd-party updaters should never need to touch it, infact they're probably doing it wrong if they do, they should only need to set the details in response or no_update.

Last edited 4 years ago by dd32 (previous) (diff)

#55 follow-up: @whyisjake
4 years ago

In 48688:

Upgrade/Install: Only show auto-update for themes that support the feature.

Similar to the changes for plugins in [48669], let's only show the UI for themes when updates are supported for that theme.

See #50280.
Props dd32.

#56 @apedog
4 years ago

#50778 was marked as a duplicate.

whyisjake commented on PR #441:


4 years ago
#57

Merged today in #48688. Keeping this open as a backport candidate for 5.5.

#58 @whyisjake
4 years ago

I think that the plan that @dd32 has laid out makes sense to bring to the 5.5 branch and release.

As he said in Slack:

[I]t’s a bug that it’s only set for plugins and not themes. The notion that the plugins changes were going to need to apply to themes as well should’ve been obvious, but last minute things happen.

Ahead of merge, I need to get a second committer to review this strategy.

#59 @whyisjake
4 years ago

  • Keywords commit added

#60 @SergeyBiryukov
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

[48688] looks good to backport to the 5.5 branch.

#61 @whyisjake
4 years ago

In 48698:

Upgrade/Install: Only show auto-update for themes that support the feature.

Similar to the changes for plugins in [48669], let's only show the UI for themes when updates are supported for that theme.

This brings the changes from [48688] to the 5.5 branch.

See #50280.
Props dd32.

#62 @whyisjake
4 years ago

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

whyisjake commented on PR #441:


4 years ago
#63

Backported to 5.5 branch here.

#64 @dennis_f
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It appears that once auto updates are enabled for external plugins (that support updates), the "Disable auto-updates" link is not visible when the plugin is up to date. So basically, the "Disable auto-updates" link is only visible while there is a new version available, but as soon as the latest version is installed, it is not present anymore.

Edit: Never mind, I just found that no_update needs to be set in the transient.

Last edited 4 years ago by dennis_f (previous) (diff)

#65 @SergeyBiryukov
4 years ago

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

#66 in reply to: ↑ 55 @pbiron
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to whyisjake:

In 48688:

Upgrade/Install: Only show auto-update for themes that support the feature.

Similar to the changes for plugins in [48669], let's only show the UI for themes when updates are supported for that theme.

See #50280.
Props dd32.

The above commit did not include the requisite changes to WP_MS_Themes_List_Table. The impact is that in multisite, the Enable/Disable auto-updates links will still appear for externally hosted themes on the Network Admin > Themes screen :-(

I'm looking into what it will take to add the multisite support for this. It's complicated by the fact that plugins use an array (returned by get_plugin_data()) and thus can use array_merge() on the info in the transient, array( 'update-supported => true ) and the plugin_data; whereas themes use an instance of WP_Theme...making the array merge not possible.

I think something can be worked out using the magic __get() and __set() methods of WP_Theme...but still looking into that.

#67 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added; has-patch commit dev-reviewed removed

#68 @audrasjb
4 years ago

  • Milestone changed from 5.5 to 5.5.1

WP 5.5 RC2 is today, let's move that part of the ticket to the next minor release.

#69 @khag7
4 years ago

  • Focuses multisite added

I was confused by this ticket being reopened. I just realized, this is now only an issue for multisite. I'm adding multisite focus if that's helpful to anyone.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

#71 @audrasjb
4 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

As per today's bug scrub, let's close this ticket in favor of #51129.

#72 @audrasjb
4 years ago

  • Milestone changed from 5.5.1 to 5.5
  • Version 5.5 deleted
Note: See TracTickets for help on using tickets.