WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 9 months ago

#50808 closed defect (bug) (fixed)

Plugins screen - The view 'Auto-updates Enabled' also counts not supported plugins

Reported by: knutsp Owned by: audrasjb
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: Upgrade/Install Keywords: has-patch dev-reviewed commit
Focuses: Cc:

Description

On plugins.php screen, and network/plugins.php, right above the table, the usual view links, one of the links reads "Auto-updates Enabled" and has an item count.

This count seems to include all plugins that does not explicitly have Auto-updates Disabled. This includes/counts also those plugins with no (auto) update supported.

Expected: The count should reflect only those specifically set to be have Auto-updates Enabled.

In other words: (All) - ( Enabled + Disabled ) = (the number of plugins with no support)

Since Enabled is the default, it probably has to calculate this by rearranging the above formula, based on an actual, internal count of plugins with no support.

For network/themes.php I can't see these views. Is the lack of them an oversight?

Attachments (4)

50808.diff (1.0 KB) - added by audrasjb 12 months ago.
Upgrade/Install: Add plugins that doesn’t support auto-updates to filters count
patch-1.png (686.4 KB) - added by audrasjb 12 months ago.
Before the patch: plugins that doesn't support auto-updates were counted in the wrong filter
patch-2.png (684.9 KB) - added by audrasjb 12 months ago.
After the patch: plugins that doesn't support auto-updates are counted in the good filter
patch-3.png (120.5 KB) - added by audrasjb 12 months ago.
After the patch: filters works fine \o/

Download all attachments as: .zip

Change History (26)

#1 @knutsp
12 months ago

For network/themes.php I can't see these views. Is the lack of them an oversight?

Sorry. My multisites does not run trunk/5.5 yet, just the feature plugin, so I guess it has the same issue.

#2 @audrasjb
12 months ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to audrasjb
  • Status changed from new to assigned

THanks for spotting this @knutsp, working on a patch right now.
Milestoning to 5.5 as this regression introduced in RC1 wasn't here during Beta cycle.

It will be up to the Release Lead to keep it in the milestone or not.

@audrasjb
12 months ago

Upgrade/Install: Add plugins that doesn’t support auto-updates to filters count

@audrasjb
12 months ago

Before the patch: plugins that doesn't support auto-updates were counted in the wrong filter

@audrasjb
12 months ago

After the patch: plugins that doesn't support auto-updates are counted in the good filter

@audrasjb
12 months ago

After the patch: filters works fine \o/

#3 @audrasjb
12 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

50808.diff fixes the issue.

Pinging @pbiron, @desrosj, @whyisjake, @dd32 for peer review as they are aware of the background of the patch that introduced the issue.

No string changes. Hope to see this regression resolved during the RC cycle.

#4 @pbiron
12 months ago

Just finishing up my "report" and the "raw material" for the new dev-note. When I finish that (and post it to #core-auto-updates) I'll test this patch.

#5 @pbiron
12 months ago

after some investigation, it appears that the problem as reported and demonstrated in the screenshots JB attached here, only happens if a site has auto-updates enabled for some plugins that do not hook into the Updates API before updating to RC1 and/or [48669] was committed, e.g., a site that had been running beta4 or 5.4 w/ the feature plugin.

Both @audrasjb and I have tested that hypothesis and it seems to be the case. Would love to have someone else check whether it is true. To do that:

  • install RC1 on a fresh site, with at least 1 plugin that doesn't hook into the API
  • enable auto-updates for 1 or more plugins that do hook into the API
  • confirm that the plugins that don't hook into the API are included in the count for the Auto-update disabled view and displayed in the list table when that view is selected

or, on a site that had auto-updates enabled for some plugins that don't hook into the API before it was updated to RC1:

  • delete the auto_update_plugins from the DB
  • enable auto-updates for 1 or more plugins that do hook into the API
  • confirm that the plugins that don't hook into the API are included in the count for the Auto-update disabled view (and displayed in the list table when that view is selected)

While the reported behavior is technically a regression, I suspect it will affect very few sites.

But this also raises another question: should plugins that don't hook into the API be counted as having auto-updates disabled?

My initial thought is that they should not be included in either the enabled nor disabled view. If others agree, then 2 more questions come up:

  1. does that need to be fixed for 5.5?
  2. should there be a Auto-updates not supported view?

I think number 2 can certainly be considered a possible enhancement for 5.5.1. Not sure about number 1.

Thoughts?

#6 @dd32
12 months ago

My initial thought is that they should not be included in either the enabled nor disabled view. If others agree, then 2 more questions come up:

My assumption was that plugins that do not support auto-update toggling would show up in the Auto-updates disabled view.
I think of that view as a "What plugins do NOT get automatically updated?" the inverse of "What plugins WILL get automatically updated".

50808.diff gets a +1 from me, logically it looks correct and the screenshots show what I would expect.

#7 @apedog
12 months ago

I'm thinking not-supported is its own category/view.
And any time in the future the view is added is fine. I wouldn't even miss it if it wasn't there at all. It's not a particularly useful view (as opposed to Enabled/Disabled views) but I get how completionists might want it there. In my mind it's just clutter.

#8 @afragen
12 months ago

I'm not sure that unsupported plugins should show up in either Auto-updates enabled or Auto-updates disabled. Using those tabs provides for a methods of toggling the auto-update switch. Including a non-functional plugin in either of those tabs seems confusing.

I think of Auto-updates disabled as those plugins that CAN be auto-updated, but the user chooses not to. In this case the auto-update isn't simply disabled, it's not present and not capable of being enabled.

#9 follow-up: @dd32
12 months ago

I think of Auto-updates disabled as those plugins that CAN be auto-updated, but the user chooses not to. In this case the auto-update isn't simply disabled, it's not present and not capable of being enabled.

I guess it depends highly upon the way you think of the filters. But to use the existing format of the plugins page, we've got Active/Inactive, Recently Active, Update Available, and now Auto-update Enabled/Disabled.

Given the language constructs in use, it doesn't make sense (to me) to split the latter into Auto-Update Enabled/Disabled/Unsupported when all Unsupported are by design Disabled. It should also be pointed out that at this point it's splitting hairs, as the vast majority of users will most likely rarely have an unsupported plugin listed.

Additionally, to use the Active/Inactive case, we count plugins that can't be activated as Inactive, such as those who use Network: true, to say that enabled/disabled is any different from active/inactive when they're both boolean terms seems like it'll introduce more confusion than it'll solve, even if it goes against some purist notions.
https://i0.wp.com/cldup.com/T9aClM1G62.png

#10 in reply to: ↑ 9 @afragen
12 months ago

Objection withdrawn. Let’s move forward.

Replying to dd32:

I think of Auto-updates disabled as those plugins that CAN be auto-updated, but the user chooses not to. In this case the auto-update isn't simply disabled, it's not present and not capable of being enabled.

I guess it depends highly upon the way you think of the filters. But to use the existing format of the plugins page, we've got Active/Inactive, Recently Active, Update Available, and now Auto-update Enabled/Disabled.

Given the language constructs in use, it doesn't make sense (to me) to split the latter into Auto-Update Enabled/Disabled/Unsupported when all Unsupported are by design Disabled. It should also be pointed out that at this point it's splitting hairs, as the vast majority of users will most likely rarely have an unsupported plugin listed.

Additionally, to use the Active/Inactive case, we count plugins that can't be activated as Inactive, such as those who use Network: true, to say that enabled/disabled is any different from active/inactive when they're both boolean terms seems like it'll introduce more confusion than it'll solve, even if it goes against some purist notions.
https://i0.wp.com/cldup.com/T9aClM1G62.png

#11 @pbiron
12 months ago

Thanx for the feedback everyone.

@afragen:

I think of Auto-updates disabled as those plugins that CAN be auto-updated, but the user chooses not to. In this case the auto-update isn't simply disabled, it's not present and not capable of being enabled.

That is my initial thought as well.

@dd32:

Additionally, to use the Active/Inactive case, we count plugins that can't be activated as Inactive

But that point convinces me that the unsupported view is not necessary (at least for 5.5).

I've also thought of another situation where a plugin could end up in get_site_option( 'auto_update_plugins', array() ) even though updates aren't supported: a plugin that is in the .org repo (and hence supports updates) is has auto-updates enabled in the UI and then gets pulled from the repo. so the situation I described as only impacting the few sites that had auto-updates enabled for non-updatable plugins prior to RC1 could actually happen to a larger group of sites.

So, yeah, I think 50808.diff is actually necessary (and works as expected).

This ticket was mentioned in PR #445 on WordPress/wordpress-develop by garyc40.


12 months ago

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

Plugins that filter auto_update_plugin and returns true should be listed in Auto-updates Enabled view instead of Auto-updates Disabled view.

#13 @chriscct7
12 months ago

#50825 was marked as a duplicate.

#14 follow-up: @pbiron
12 months ago

Following up, there has been a discussion on Slack about this.

It looks to me that the change in Dion's PR correctly handles this and the code is easier to read/follow than 50808.diff.

Last edited 12 months ago by whyisjake (previous) (diff)

#16 @whyisjake
12 months ago

  • Keywords commit added

I'm also in favor of the approach found in Dion's PR. Would love to get this landed.

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


12 months ago

#18 in reply to: ↑ 14 @SergeyBiryukov
12 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Replying to pbiron:

It looks to me that the change in Dion's PR correctly handles this and the code is easier to read/follow than 50808.diff.

Same here, the PR looks good to me.

#19 @whyisjake
12 months ago

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

(I neglected to add the #, so this commit doesn't show on the ticket)

WordPress commit r48703

Upgrade/Install: Ensure the proper count of plugins that have auto-update enabled.

As certain plugins may not support updates, this count needs to accurately reflect that in the navbar.

Fixes #50808.
Props knutsp, audrasjb, pbiron, dd32, apedog, afragen, chriscct7, garyc40, whyisjake, SergeyBiryukov.

#20 @whyisjake
12 months ago

WordPress commit r48704

Upgrade/Install: Ensure the proper count of plugins that have auto-update enabled.

As certain plugins may not support updates, this count needs to accurately reflect that in the navbar.

This brings the changes from [48703] to the WordPress 5.5 branch.

Fixes #50808.
Props knutsp, audrasjb, pbiron, dd32, apedog, afragen, chriscct7, garyc40, whyisjake, SergeyBiryukov.

#21 @prbot
12 months ago

whyisjake commented on PR #445:

Fixed in r48703

Note: See TracTickets for help on using tickets.