WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 9 months ago

#44990 new defect (bug)

Unhandle post_ids variable in custom bulk actions

Reported by: gaupoit Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 4.9.8
Component: Media Keywords: dev-feedback 2nd-opinion
Focuses: Cc:
PR Number:

Description

This error happened when creating a custom bulk actions by using filter "handle_bulk_actions-" then user don't choose any items to apply that custom bulk action. The reason is in ~/wp-admin/upload.php line 170, the post_ids variable has never defined or handled.

<?php
default:
/** This action is documented in wp-admin/edit-comments.php */
$location = apply_filters( 'handle_bulk_actions-' . get_current_screen()->id, $location, $doaction, $post_ids );

Change History (4)

#1 @garrett-eclipse
13 months ago

  • Keywords ux-feedback added

Thanks @gaupoit welcome to Trac.

This seems to coincide with #45006 to handle Bulk Actions when no items selected.

When reviewing this it appears the trash, untrash and delete actions in the switch case all do isset checks on $post_ids;
https://github.com/WordPress/WordPress/blob/master/wp-admin/upload.php#L149
https://github.com/WordPress/WordPress/blob/master/wp-admin/upload.php#L170
https://github.com/WordPress/WordPress/blob/master/wp-admin/upload.php#L185

So in the interim you can update your custom code to similarly check $post_ids and simply return $location;;

<?php
if ( ! isset( $post_ids ) ) {
    return $location;
}

And for easy reference as the code you referred to is no longer on line 170 in master here's a permalink to 4.9.8;
https://github.com/WordPress/WordPress/blob/4.9.8/wp-admin/upload.php#L170

#2 follow-up: @gaupoit
13 months ago

Thanks, @garrett-eclipse for the reply. I also added the checking for $post_ids you mentioned in my custom code. However, the problem is here https://github.com/WordPress/WordPress/blob/4.9.8/wp-admin/upload.php#L170 the variable $post_ids is never handled.

Last edited 13 months ago by garrett-eclipse (previous) (diff)

#3 in reply to: ↑ 2 @garrett-eclipse
13 months ago

  • Keywords 2nd-opinion added

Thanks @gaupoit, I tweaked your comment to switch from master branch making it a permalink as master often changes.

Replying to gaupoit:

However, the problem is here https://github.com/WordPress/WordPress/blob/4.9.8/wp-admin/upload.php#L170 the variable $post_ids is never handled.

By unhandled I'm assuming you're speaking of the isset check on the $post_ids array which isn't done before firing the action? If I misunderstood please correct me. But with that in mind.

Checking the other actions - https://github.com/WordPress/WordPress/search?q=handle_bulk_actions-&type=Code
Only the /wp-admin/edit.php does a global check before entering the conditional logic;
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/edit.php#L99-L102

Other instances either use the foreach around the switch statements or use an isset within the switch statement before the foreach.

In most cases it seems the check is avoided prior to the action to allow it to fire even when the ids array is empty. I'm unsure if that's intended by design so adding 2nd-opinion. And wanted to flag it's existing use in Plugins;
https://wpdirectory.net/search/01CS1BY2N61SNYPJBRVJXX5N7C
*If any do actions when they receive an empty array then introducing a check in core would avoid the action and result in those actions never firing. This is my only concern with adding a global check, it would force a global redirect location without providing the ability to plugins/etc to override that.

And just for reference, the docblock associated to this action is found here;
https://github.com/WordPress/WordPress/blob/4.9.8/wp-admin/edit-comments.php#L85-L103

#4 @karmatosed
9 months ago

  • Keywords ux-feedback removed

Looking at this, not surer that it needs ux-feedback. If that is the case please add back in. For now, I am removing that keyword.

Note: See TracTickets for help on using tickets.