WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 months ago

#46872 new defect (bug)

Bulk Actions underneath plugins list perform Update when Deactivate chosen

Reported by: clayray Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.8
Component: Quick/Bulk Edit Keywords: has-patch needs-testing
Focuses: ui, administration Cc:
PR Number:

Description

I have selected a number of plugins to deactivate. Then I choose Deactivate from the dropdown BELOW the plugins list and then click Apply, I am taken to an "Updates" screen where the plugins get updated.
This does not happen if I use the dropdown and apply button ABOVE the plugins list.
(By "trunk" below, presumably the current version 5.1.1 is meant, since 5.1.1 isn't otherwise selectable via the Dropdown)

Attachments (1)

46872.diff (989 bytes) - added by garrett-eclipse 6 months ago.
Initial working patch to apply the doaction[2] string as the name attribute on the submit input. This will allow us to know which button was clicked.

Download all attachments as: .zip

Change History (11)

#1 @subrataemfluence
7 months ago

  • Keywords reporter-feedback added

I did a fresh 5.2 beta install and tried to reproduce the issue by first activating the bundled plugins Akismeta and Hello Dolly and then Bulk deactivating. I could not get into any issue.

If possible please provide the exact steps you are using in order to reproduce the issue.

Thank you!

#2 @SergeyBiryukov
6 months ago

  • Focuses ui administration added

#3 @garrett-eclipse
6 months ago

  • Keywords needs-patch added; reporter-feedback removed

Hi @clayray,

Correct me if my assumption is wrong but was the top select possibly set to 'Update' when you used the bottom one?

I believe I've reproduced your issue in trunk with one clarifying nuance, setting the top Bulk Select action to Update and then going to use the bottom action.

This seems to force the update action on undesireable plugins, even ones which don't have any available updates. Not sure if that overwrites them with what's on wp.org but if so that could replace customized code unnecessarily (not that you should customize plugins directly...).

@subrataemfluence with this nuance in mind can you retest and double confirm for me. Simply add one step prior to your existing tests;

  1. Change the top 'Bulk Action' selector to 'Update'

Testing with other options set in the top Bulk Action selector it seems you can unintentionally trigger all of them. This means you can accidentally trigger Delete action (thankfully there's a confirm page).

#4 @garrett-eclipse
6 months ago

  • Keywords dev-feedback added

Looking into the code it appears this issues comes from the parent WP_List_Table class in wp-admin/includes/class-wp-list-table.php here;
https://github.com/WordPress/WordPress/blob/7a617078fa7674f2062379b961daba412875ab9e/wp-admin/includes/class-wp-list-table.php#L492-L498

<?php
if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
        return $_REQUEST['action'];
}
if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
        return $_REQUEST['action2'];
}

So if the first bulk selector is set it's action takes precedence irregardless of the secondary action.

There's a few avenues we can take to resolve this.

  1. Tie the two selectors together so changes to one apply to the other meaning they'll always submit the same value.
  2. Depending on which Apply is used unset the other bulk action selector so it's value being submitted is -1 allowing the second action to act.
  3. Introduce a hidden input which gets the currently acted upon selectors value mapped to it just before submit.

Adding dev-feedback to get input on the above.

#5 @garrett-eclipse
6 months ago

  • Component changed from Plugins to Quick/Bulk Edit

Moving to 'Quick/Bulk Edit' as the issue pertains to all List Tables and not just Plugins.

#6 @subrataemfluence
6 months ago

@garrett-eclipse I should have been more elaborate when tested. You are right. Your way allowed me to reproduce the issue.

My proposal is that if we can rewrite the current function in the following way, it will solve the issue.

<?php
public function current_action() {
  if ( isset( $_REQUEST['filter_action'] ) && ! empty( $_REQUEST['filter_action'] ) ) {
    return false;
  }
                
  $action1 = '';
  $action2 = '';
                
  if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
    $action1 = $_REQUEST['action'];
  }

  if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
    $action2 = $_REQUEST['action2'];
  }
                
  if( $action1 === $action2 ) {
    return $action1;
  }
                
  return false;
}

However, we need to have a proper AdminNotice, something like

You have selected different actions. Operation cannot be performed.

#7 @subrataemfluence
6 months ago

There is a logical issue in my above code. Please disregard. I am looking into it.

#8 @subrataemfluence
6 months ago

Modified code:

<?php
public function current_action() {
     if ( isset( $_REQUEST['filter_action'] ) && ! empty( $_REQUEST['filter_action'] ) ) {
          return false;
     }
        
     if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
          if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
               if( $_REQUEST['action'] === $_REQUEST['action2'] ) {
                    return $_REQUEST['action'];
               }
                
               return false;
          }
            
          return $_REQUEST['action'];
     }
        
     if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
          return $_REQUEST['action2'];
     }
        
     return false;
}

@garrett-eclipse
6 months ago

Initial working patch to apply the doaction[2] string as the name attribute on the submit input. This will allow us to know which button was clicked.

#9 @garrett-eclipse
6 months ago

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

Thanks @subrataemfluence I appreciate the logic suggestions for for current_action but I feel we can accomplish this even simpler by applying a unique name attribute to the submit inputs as that will then make available to the $_REQUEST object which submit button was actually used.

With that in mind can you test my simple patch 46872.diff.

It should also avoid submitting action1 when action2 isn't present but the doaction2 button is clicked.

Thoughts?

#10 @SergeyBiryukov
6 months ago

  • Version changed from trunk to 2.8

Introduced in [11474]. Related: #28555

Note: See TracTickets for help on using tickets.