Make WordPress Core

Opened 3 years ago

Closed 17 months ago

Last modified 17 months ago

#54309 closed defect (bug) (fixed)

The "auto-updates enabled / disabled" filter links go missing

Reported by: nekojonez's profile NekoJonez Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.5
Component: Plugins Keywords: has-patch has-testing-info has-screenshots commit
Focuses: ui, administration Cc:

Description (last modified by johnbillion)

On the plugin page, when you click on "required" or "drop-in" plugins, the two filter links for "Auto-updates enabled" and "Auto-updates disabled" don't appear.

Without the links:
https://ibb.co/qmhCTcJ

With the links:
https://ibb.co/Tt9S8Bf

Attachments (1)

61a5470be356579bc5bd67cf19a85d42.gif (656.3 KB) - added by audrasjb 17 months ago.
Issue fixed

Download all attachments as: .zip

Change History (25)

#1 @NekoJonez
3 years ago

  • Component changed from General to Plugins
  • Focuses ui administration added

#2 @pbiron
3 years ago

Thanx for the ticket @NekoJonez.

The reason the Enable/Disable auto-updates links don't appear for Must-Use and Drop-in plugins is that those plugins never receive auto-updates...so having the links wouldn't make sense.

#3 follow-up: @costdev
22 months ago

I believe the ticket description refers to the status links in the list table, rather than the plugin row toggle links.

The list table should indeed still show the Auto-updates enabled and Auto-updates Disabled links.

#4 @costdev
22 months ago

#57456 was marked as a duplicate.

#5 @costdev
22 months ago

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

Moving to 6.2 for consideration and assigning @audrasjb who took ownership of the duplicate mentioned above.

#6 @costdev
22 months ago

Looks like this may be intended behaviour.

  • Ref 1 - The auto-updates-enabled and auto-updates-disabled are only added to the array when $this->show_autoupdates is truthy.
  • Ref 2 - Plugin data is only added to these keys if $this->show_autoupdates is truthy.
  • Ref 3 - The $totals array is populated.
  • Ref 4 - The totals are looped, which won't include the auto-update statuses.
  • Ref 5 - $this->show_autoupdates is specifically tied to mustuse and dropin statuses.

If so, I'd suggest we change this behaviour. These navigation items should show regardless of the currently viewed status.

However, as shown by the references above, the protected show_autoupdates property is coupled to the list table's navigation and the Enable/Disable auto-updates links that show in plugin rows. Extender sub-classes may use this, and may want to hide the list table navigation for Auto-updates. As always, we should be careful not to introduce an unintended BC break.

That said, I can't see any relevant usage in this search result.

@audrasjb what are your thoughts on this?

Last edited 22 months ago by costdev (previous) (diff)

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


21 months ago

#8 @costdev
21 months ago

  • Keywords needs-patch added
  • Milestone changed from 6.2 to Future Release

As this ticket doesn't have a patch yet and the issue was not introduced in 6.2 (and may be expected behaviour), I'm moving this to Future Release for investigation, and adding needs-patch.

#9 @johnbillion
21 months ago

  • Description modified (diff)
  • Summary changed from The "auto-updates enabled / disabled" buttons go missing to The "auto-updates enabled / disabled" filter links go missing
  • Version set to 5.5

#10 in reply to: ↑ 3 @pbiron
21 months ago

Replying to costdev:

I believe the ticket description refers to the status links in the list table, rather than the plugin row toggle links.

The list table should indeed still show the Auto-updates enabled and Auto-updates Disabled links.

The reasoning from #comment:3 still applies. Since auto-updates can't be enabled/disabled for plugins in either of these views, we specifically excluded outputting the view links.

I don't remember of there was an issue created in the repo for the feature plugin that would document that, maybe @audrasjb will remember.

#11 follow-ups: @pbiron
21 months ago

After a little conversation with @costdev and little bit of investigation of the code he references in #comment:6, I now believe that while we specifically intended to hide the auto-update views when the the current view is Must Use or Drop-in that decision was incorrect...and this is, in fact, a bug.

I'll do a patch this evening.

#12 in reply to: ↑ 11 @audrasjb
21 months ago

Replying to pbiron:

I now believe that while we specifically intended to hide the auto-update views when the the current view is Must Use or Drop-in that decision was incorrect...and this is, in fact, a bug.

Good point, this is what probably happened, and I agree with you that this decision was not the best.

#13 in reply to: ↑ 11 @pbiron
21 months ago

Replying to pbiron:

I'll do a patch this evening.

I have a patch prepared, but am waiting for someone else to give it an initial test first (to make sure it doesn't introduce any regressions). Will do a PR tomorrow.

This ticket was mentioned in PR #4100 on WordPress/wordpress-develop by @pbiron.


21 months ago
#14

  • Keywords has-patch added; needs-patch removed

#15 @pbiron
21 months ago

Simple testing instructions for the PR:

  1. Apply the PR
  2. Ensure you have at least 1 Must Use plugin and 1 Drop-in plugin installed
  3. Navigate to Plugins > Installed Plugins > Must use (wp-admin/plugins.php?plugin_status=mustuse)
  4. Navigate to Plugins > Installed Plugins > Drop-in (wp-admin/plugins.php?plugin_status=dropin)

Verify that the Auto-updates Enabled/Disabled views still appear in steps 3&4.

@costdev will shortly be adding more detailed testing (that check for BC) and he'll also be providing me with some unit tests (at which time I'll update the PR with those)

Colin is much better at writing testing instructions than I am ;-)

#16 @costdev
21 months ago

  • Keywords has-testing-info needs-testing added

Testing Instructions

Please note
These testing instructions are long to ensure that the patch works as expected and doesn't introduce any backward compatibility breaks. Despite being long, they have been written as step-by-step instructions to, hopefully, make them easier to follow.

Setup

  1. Create a new file at wp-content/mu-plugins/test_54309.php with the following content:
    <?php
    
    /**
     * Plugin Name: Must-Use plugin for testing #54309.
     * Description: This shows a Must-Use plugin in Plugins > Installed plugins > Must-Use
     * Author:      WordPress Core Contributors
     * Author URI:  https://make.wordpress.org/core
     * License:     GPLv2 or later
     * Version:     1.0.0
     */
    
  2. Navigate to Plugins > Add New.
  3. Install and activate the User Role Editor, View Admin As and Query Monitor plugins.
  4. Navigate to Plugins > Installed Plugins and click Enable auto-updates beside Query Monitor.
  5. Navigate to Users > User Role Editor.
  6. Select Contributor from the dropdown, add the activate_plugins and update_plugins capabilities, then click Update and Yes to confirm`.
  7. Select Editor from the dropdown, add the activate_plugins capability, then click Update and Yes to confirm.
  8. Select Subscriber from the dropdown, add the update_plugins capability, then click Update and Yes to confirm.

Hints

  • BC = Backward Compatibility (the patch doesn't break previous functionality)
  • 🐞 = A bug to be fixed.
  • ✅ = The expected behaviour.
  • Testing recommendations:
    • First go through these steps without the patch, only paying attention to the "Before the patch" and "Before and after the patch" sections.
    • Apply the patch with this command:
      grunt patch:https://github.com/WordPress/wordpress-develop/pull/4100.diff
      
    • Then go through these steps again, but this time, only pay attention to the "After the patch" and "Before and after the patch" sections.
    • I appreciate that there are a lot of steps. Focus on one step at a time.

Steps to Reproduce or Test

  1. Navigate to the Dashboard.
  2. At the top right of the admin bar, hover over View As and click Contributor.
  3. Navigate to Plugins > Installed Plugins > Must-Use.
    1. Before the patch:
      1. 🐞 The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ (BC) The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
    2. After the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled views should appear above the table.
      2. ✅ (BC) The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
  4. Navigate to Plugins > Installed Plugins > Drop-in.
    1. Before the patch:
      1. 🐞 The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ (BC) The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
    2. After the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled view links should appear above the table.
      2. ✅ (BC) The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.

Steps to test Backward Compatiblity more extensively

  1. Set define( 'AUTOMATIC_UPDATER_DISABLED', true ); in wp-config.php.
  2. Navigate to Plugins > Installed Plugins > Must-Use.
    1. Before and after the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
  3. Navigate to Plugins > Installed Plugins > Drop-in.
    1. Before and after the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
  4. Remove define( 'AUTOMATIC_UPDATER_DISABLED', true ); from wp-config.php.
  5. Navigate to the Dashboard.
  6. At the top right of the admin bar, hover over Viewing as Role: Contributor and click Editor.
  7. Navigate to Plugins > Installed Plugins.
    1. Before and after the patch:
      1. ✅ You should be able to deactivate Query Monitor and re-activate it.
  8. Navigate to Plugins > Installed Plugins > Must-Use.
    1. Before and after the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
  9. Navigate to Plugins > Installed Plugins > Drop-in.
    1. Before and after the patch:
      1. ✅ The Auto-updates Enabled and Auto-updates Disabled view links should not appear above the table.
      2. ✅ The Enable auto-updates and Disable auto-updates links should not appear in the plugin row.
  10. Navigate to the Dashboard.
  11. At the top right of the admin bar, hover over Viewing as Role: Editor and click Subscriber.
    1. Before and after the patch:
      1. ✅ The Plugins menu item should not appear.
  12. Manually navigate to /wp-admin/plugins.php.
    1. Before and after the patch:
      1. ✅ You should receive a message that you cannot access the page. :white_tick:

Steps to clean up your local environment

  1. At the top right of the admin bar, hover over Viewing as Role: Subscriber and select Reset to default.
  2. Navigate to Plugins > Installed plugins. Deactivate and delete all plugins.
  3. Delete the file at wp-content/mu-plugins/test_54309.php.
  4. Run this command to remove the patch:
    git restore .
    

Expected Results

When reproducing a bug:

  • ❌ All "Before the patch" items marked with 🐞 should occur.
  • ✅ All "Before the patch" and "Before and after the patch" items marked with ✅ should occur.

When testing a patch to validate it works as expected:

  • ✅ All "After the patch" and "Before and after the patch" items marked with ✅ should occur.
Last edited 21 months ago by costdev (previous) (diff)

#17 @costdev
21 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4100.diff

Environment

  • Server: Apache (Linux)
  • WordPress: 6.2-beta2-55340-src
  • Browser: Chrome 110.0.0.0
  • OS: Windows 10
  • Theme: Twenty Twenty-One
  • Plugins:
    • Must-Use plugin for testing #54309 1.0.0
    • Query Monitor 3.11.1
    • User Role Editor 4.63.2
    • View Admin As 1.8.8

Actual Results

When reproducing a bug:

  • ❌ All "Before the patch" items marked with 🐞 occurred. Issue reproduced.
  • ✅ All "Before the patch" and "Before and after the patch" items marked with ✅ occurred.

When testing a patch to validate it works as expected:

  • ✅ All "After the patch" and "Before and after the patch" items marked with ✅ occurred.
  • ✅ Testing on a Multisite network showed it's own expected behaviour before and after the patch.
  • ✅ Issue resolved with the patch.
  • ✅ No backward compatibility breaks detected.

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


21 months ago

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


21 months ago

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


17 months ago

#21 @audrasjb
17 months ago

  • Milestone changed from Future Release to 6.3

As per today's bug scrub, moving this to 6.3 and self assigning for final review.

#22 @audrasjb
17 months ago

  • Keywords has-screenshots commit added; needs-testing removed

I tested the proposed patch successfully. Marking for commit.

#23 @audrasjb
17 months ago

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

In 55903:

Plugins: Display Auto-updates filters when the current view is "Must Use" or "Drop-in".

This changeset fixes a bug where the "Auto-updates Enabled/Disabled" filters were not showing when the current view is "Must Use" or "Drop-in".

Props NekoJonez, pbiron, costdev, audrasjb.
Fixes #54309.

Note: See TracTickets for help on using tickets.