Make WordPress Core

Opened 8 years ago

Closed 4 years ago

Last modified 3 years ago

#39589 closed defect (bug) (fixed)

PHP Notice if nothing selected on custom bulk action in Media Library.

Reported by: gitlost's profile gitlost Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch has-testing-info commit
Focuses: Cc:

Description

If you add a custom bulk action to the Media Library:

<?php
add_filter( 'bulk_actions-upload', function( $actions ) {
        $actions['foo'] = 'Foo';
        return $actions;
} );

Then if nothing selected when you apply "Foo" (and you have WP_DEBUG on) you'll get a

PHP Notice:  Undefined variable: post_ids in .../wp-admin/upload.php on line 168

Attachments (3)

39589.patch (713 bytes) - added by achbed 8 years ago.
39589_b.patch (633 bytes) - added by achbed 8 years ago.
Revised based on feedback
39589_c.patch (468 bytes) - added by wpgurudev 4 years ago.
Define variable

Download all attachments as: .zip

Change History (21)

@achbed
8 years ago

#1 @achbed
8 years ago

  • Keywords has-patch needs-testing needs-docs added

This is a first pass at fixing this. This version does change behavior in two key ways.

1) It always runs the filter, even for those cases where there is nothing selected. In this case, it passes an empty array for $post_ids. This allows the filter to put up an appropriate error message.

2) It also checks the filter return value. If the filter returns false, null, or empty string, it will use the previously defined $location instead.

This behavior change may be unexpected to some. We should discuss these changes to make sure these are appropriate.

This ticket was mentioned in Slack in #core-media by achbed. View the logs.


8 years ago

#3 @gitlost
8 years ago

Looking at [38647] none of the other handle_bulk_actions filters checks the return so perhaps it's best to leave it as is and just do the isset() check (which should be before the comment). Also I don't think it constitutes a change of behaviour as it always runs now - adding the check just saves one having to do something similar in the handler...

@achbed
8 years ago

Revised based on feedback

#4 @achbed
8 years ago

Patch is revised to not check the return value, but blindly assign to $location, matching behavior of non-filtered handlers.

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


4 years ago

#6 @Boniu91
4 years ago

The issue is still present on:
NGINX
PHP 7.4
WP 5.7.2

@wpgurudev
4 years ago

Define variable

#7 @wpgurudev
4 years ago

This issue is happening because $post_ids variable is never getting defined when no attachments are selected.

We can define the variable as blank array just to ensure this issue would not occur and later modify it.

Adding patch here.

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


4 years ago

#9 @hellofromTonya
4 years ago

  • Keywords has-testing-info added

Steps to reproduce and test

  • Add a file to wp-content/mu-plugins/ and copy paste the supplied Foo code:
    <?php
    
    add_filter( 'bulk_actions-upload', function( $actions ) {
            $actions['foo'] = 'Foo';
            return $actions;
    } );
    
    
  • In wp-config.php add define( 'WP_DEBUG', true );
  • In the admin, go to Media
  • In Bulk actions dropdown, select Foo
  • Click Apply button
  • Check for the PHP Notice
    • Note, if you have WP_DEBUG_LOG set to true, the notice will appear in the debug.log.

#10 @hellofromTonya
4 years ago

  • Owner set to hellofromTonya
  • Status changed from new to accepted

#11 @hellofromTonya
4 years ago

  • Keywords needs-testing removed

Testing Result

Env:

  • WP: 5.6.4, 5.7.2, and latest 5.8-alpha
  • PHP: 7.3 to 8.0
  • Browser: Chrome 91.0.4472.77 and Firefox 89.0
  • Localhost: Local
  • OS: MacOS
  • Web server: NGINX

Before Applying Patch

Reproduced ✅

Results in debug.log:

[03-Jun-2021 12:59:20 UTC] PHP Notice:  Undefined variable: post_ids in ../Local/wpcore/app/public/wp-admin/upload.php on line 204
[03-Jun-2021 12:59:20 UTC] PHP Stack trace:
[03-Jun-2021 12:59:20 UTC] PHP   1. {main}() ../Local/wpcore/app/public/wp-admin/upload.php:0

After Applying the Patch

Results: no PHP Notice ✅

#12 @hellofromTonya
4 years ago

  • Keywords commit added; needs-docs removed

Reviewed 39589_c.patch.

  • I agree with @wpgurudev. The root cause is the variable is not defined with no attachments.
  • Initializing the $post_ids variable before the conditional overrides is a clean and understandable approach ✅
  • Tests pass (there are no unit tests for uploading)

Marking the patch ready for commit.

Removing needs-docs as this patch is a bug fix but is not changing functionality.

#13 @hellofromTonya
4 years ago

  • Milestone changed from Awaiting Review to 5.8

Moving into the milestone.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

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


4 years ago

#17 @whyisjake
4 years ago

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

In 51111:

Media: Ensure that post_id variable is initiated ahead of usage.

This prevents warnings when using custom bulk actions in the media library.

Props gitlost, achbed, Boniu91, wpgurudev, hellofromTonya.

Fixes #39589.

#18 @SergeyBiryukov
3 years ago

In 51163:

Quick/Bulk Edit: Ensure that $post_ids variable is initialized ahead of usage.

This brings some consistency between similar fragments of wp-admin/edit.php and wp-admin/upload.php.

Follow-up to [51111], [51161].

See #39589, #53411.

Note: See TracTickets for help on using tickets.