Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50822 closed defect (bug) (fixed)

Site Health page incorrectly reports plugin auto-update status

Reported by: gwendydd's profile Gwendydd Owned by: whyisjake's profile whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: Site Health Keywords: has-screenshots has-patch commit dev-reviewed
Focuses: Cc:

Description

I am working to make sure a self-hosted plugin works with 5.5's auto-updates. The plugins page correctly reports whether auto-updates are enabled or not, but the Site Health page always says auto-update is disabled.

The attached screenshots show the discrepancy between the auto-update status of Gravity Forms.

Attachments (5)

Image 2020-07-30 at 2.41.00 PM.png (710.6 KB) - added by Gwendydd 4 years ago.
Plugins page showing that auto-updates are enabled for Gravity Forms
Image 2020-07-30 at 2.42.03 PM.png (326.5 KB) - added by Gwendydd 4 years ago.
Site Health page showing auto-updates are not enabled for Gravity Forms
50822.diff (10.3 KB) - added by pbiron 4 years ago.
50822.2.diff (10.9 KB) - added by pbiron 4 years ago.
50822.3.diff (10.2 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (19)

@Gwendydd
4 years ago

Plugins page showing that auto-updates are enabled for Gravity Forms

@Gwendydd
4 years ago

Site Health page showing auto-updates are not enabled for Gravity Forms

#1 @SergeyBiryukov
4 years ago

  • Component changed from Upgrade/Install to Site Health
  • Milestone changed from Awaiting Review to 5.5

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


4 years ago

#3 @pbiron
4 years ago

The descrepency in the screenshots is the result of one part of [48669].

When populating the Automatic Updates column of the list table, auto_update_plugin (which Gravity Forms hooks in to) is applied. That filter is not currently being applied when constructing the strings to display on the Site Health debug screen.

Patch coming shortly that corrects that.

p.s. @Gwendydd welcome to trac! Thankx for noticing this.

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

@pbiron
4 years ago

#4 @pbiron
4 years ago

50822.diff addresses most of this problem and what is a bare minimum for a 5.5 fix.

It does not guarantee that the text in the Automatic Updates column in the list table and that in the Site Health debug screen will agree 100% of time, but it does ensure that those plugins/themes that appear in the Auto-updates enabled/disabled views on the list table will display Auto-updates enabled/disabled appropriately on the Site Health debug screen.

Here's why the patch doesn't guarantee the strings on the 2 screens are the same:

  1. if a plugin/theme does not hook into the Updates API then the list tables display an empty string, e.g., Gravity Forms ActiveCampaign Add-On in Image 2020-07-30 at 2.41.00 PM.png). The rationale for the patch displaying Auto-updates disabled on the debug screen for such plugins/themes is same as given in 50808 comment:9. We can reconsider that in a future version if necessary.
  1. plugins can hook into plugin_auto_update_setting_html to display a completely custom message on the list table, and it's unclear whether such custom messages would be appropriate for the debug screen. For example, @Gwendydd has mentioned that GF might hook into plugin_auto_update_setting_html to display a link to the GF settings screen. That is one of the main use cases for why plugin_auto_update_setting_html was added and is very appropriate for the list table, but likely would not be appropriate for the debug screen. Therefore, the patch here does not apply that filter when constructing the strings for the debug screen. Again, that can be reconsidered in a future version when we have some experience with how plugins in the wild actually use that filter.

As there is a lot of repetition in the site health code, please review 50822.diff very carefully to make sure I didn't inadvertently forget to correct variable names/etc when I copy-pasted code used for active plugins into that used for active theme, etc.

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


4 years ago

@pbiron
4 years ago

#6 @pbiron
4 years ago

  • Keywords has-patch added

Currently, the auto-update related Site Health strings are based on consulting the results of get_plugin_updates() and get_theme_updates(), instead of consulting the site transients directly.

The original patch (50822.diff) followed that same strategy, using response if present and creating a mock item if not, to pass to auto_update_plugin and/or auto_update_theme. That turned out to be too unreliable.

So 50822.2.diff now checks response and no_update from the transients, and only creates a mock item if neither of those is set...which is what the list table does.

Note that 50822.2.diff continues to use the returned value of get_plugin_updates() and get_theme_updates() for everything other can constructing the item to be passed to the auto_update_plugin and auto_update_theme filters. Ultimately, I think those other uses should be rewritten to use the transients directly, but I'm reluctant to do that at the last minute for fear of introducing yet another last minute bug.

#7 @audrasjb
4 years ago

Thanks @Gwendydd for the ticket and @pbiron for the patch.
The conditions added in 50822.diff looks good to me and the checks added in 50822.2.diff make it ready to ship before 5.5 RC2 in my opinion.

Not a committer, and it's just my humble opinion, but I think it's good to go. Thanks.

#8 @Gwendydd
4 years ago

I can confirm that 50822.2.diff fixes the problem!

Gravity Forms will release an update the populates no_update so that the auto-update status is reported correctly.

#9 @whyisjake
4 years ago

  • Keywords commit dev-feedback added

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


4 years ago

#11 @SergeyBiryukov
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

50822.3.diff replaces a couple of DocBlocks with duplicate hook references. Looks good otherwise.

#12 @pbiron
4 years ago

50822.3.diff looks good here.

#13 @whyisjake
4 years ago

  • Owner set to whyisjake
  • Resolution set to fixed
  • Status changed from new to closed

In 48731:

Site Health: Display auto-update properly for plugins that don't support auto-updates.

Properly filter auto_update_plugin when displaying the table.

Fixes #50822.
Props Gwendydd, pbiron, audrasjb, SergeyBiryukov, whyisjake.

#14 @whyisjake
4 years ago

In 48732:

Site Health: Display auto-update properly for plugins that don't support auto-updates.

Properly filter auto_update_plugin when displaying the table.

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

Fixes #50822.
Props Gwendydd, pbiron, audrasjb, SergeyBiryukov, whyisjake.

Note: See TracTickets for help on using tickets.