Make WordPress Core

Opened 6 years ago

Closed 18 months ago

Last modified 17 months ago

#44990 closed defect (bug) (fixed)

Unhandle post_ids variable in custom bulk actions

Reported by: gaupoit's profile gaupoit Owned by:
Milestone: 5.8 Priority: normal
Severity: minor Version: 4.9.8
Component: Quick/Bulk Edit Keywords: reporter-feedback close
Focuses: Cc:

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 (12)

#1 @garrett-eclipse
6 years 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
6 years 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 6 years ago by garrett-eclipse (previous) (diff)

#3 in reply to: ↑ 2 @garrett-eclipse
6 years 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
5 years 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.

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


3 years ago

#6 @joedolson
3 years ago

  • Component changed from Media to Quick/Bulk Edit

#7 @antpb
3 years ago

This ticket looks to have been added to the Media component but it seems this is best fit for Quick/Bulk Edit. Moving along to get proper visibility on this!

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


2 years ago

#9 @marybaum
2 years ago

In the scrub: @davidb says, " I'll track down the commits here and see if my hunch is correct. I'll ping the reporter to see if it's still an issue."

#10 follow-up: @davidbaumwald
2 years ago

  • Keywords reporter-feedback close added; dev-feedback 2nd-opinion removed

@gaupoit It looks like this was resolved in r51111. I'm going to flag this as a close candidate, but would like your feedback about whether the issue is fixed for you in the latest version of WordPress.

#11 in reply to: ↑ 10 @hellofromTonya
18 months ago

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

Re-reading the ticket and specifically @gaupoit comment here:

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.

I agree with @davidbaumwald that [51111] should have resolved the reported issue. Why? It initialized $post_ids to an empty array before the code goes into the switch() of where @gaupoit noted the issue.

I think this ticket can be marked as fixed (by #39589 / [51111]). Given that it's been 10 months with no reporter feedback, thinking it's safe to close the ticket as resolved.

If however this problem persists, @gaupoit please reopen the ticket and provide more information to help contributors further investigate.

#12 @SergeyBiryukov
17 months ago

  • Milestone changed from Awaiting Review to 5.8
Note: See TracTickets for help on using tickets.