Make WordPress Core

Opened 22 months ago

Closed 15 months ago

Last modified 15 months ago

#56170 closed defect (bug) (fixed)

Bulk delete media attachments doesn't pass integer values to wp_delete_attachment

Reported by: eherman24's profile eherman24 Owned by: joedolson's profile joedolson
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: administration Cc:


When selecting multiple images in the media table on /wp-admin/upload.php and using the bulk delete dropdown to delete images passes a string value to wp_delete_attachment(). This could cause some issues for methods hooked into into deleted_post.

The documentation for the deleted_post hook indicates the first parameter is an integer.

do_action( 'deleted_post', int $postid, WP_Post $post );

To confirm that the $postid value is a string I ran the following test code to hook into deleted_post and update an option in the database based on the value of $postid.

add_action( 'deleted_post', function( $post_id ) {
        update_option( 'test_post_integer_value', is_int( $post_id ) );
}, PHP_INT_MAX, 2 );

Deleting a single image, using the 'Delete Permanently' button on either the media list table or the media grid will update the test_post_integer_value option value to 1 (true).

Switching over to the media list table view, and using the checkboxes to select 1 or more images, and then using the bulk delete dropdown to delete the images results in a false value. The option value is empty in the database.

So it's worth noting this only happens using the bulk delete actions dropdown on the media list table. Even when selecting a single image.

Attachments (3)

typecast-postid-bulk-delete-attachments.diff (679 bytes) - added by eherman24 22 months ago.
typecast-postid-bulk-delete-attachments-1.diff (473 bytes) - added by eherman24 22 months ago.
Updated diff.
56170.1.diff (1.4 KB) - added by joedolson 15 months ago.
Updated patch with alternate method

Download all attachments as: .zip

Change History (15)

#1 @eherman24
22 months ago

I added an updated patch, the first one included an unnecessary typecast of $post_id_del to current_user_can()

#2 @eherman24
22 months ago

This was on PHP 8.0.20

#3 @desrosj
19 months ago

  • Version trunk deleted

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

18 months ago

#5 @antpb
18 months ago

  • Milestone changed from Awaiting Review to 6.2

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

15 months ago

#7 @joedolson
15 months ago

  • Owner set to joedolson
  • Status changed from new to reviewing

15 months ago

Updated patch with alternate method

#8 @joedolson
15 months ago

  • Keywords 2nd-opinion removed
  • Status changed from reviewing to accepted

The provided patch fixes the immediate issue, but doesn't cover the same issue with wp_trash_post and wp_untrash_post earlier in the same function, which also expect int arguments.

Alternate version of patch ensures that $post_ids is an array of integers from the beginning of the function, simplifying the code by making the (array) and (int) casting unnecessary.

Version 0, edited 15 months ago by joedolson (next)

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

15 months ago

Ensure that post IDs passed to wp_trash_post, wp_untrash_post, and wp_delete_attachment are always of type int.

Trac ticket:

#10 @joedolson
15 months ago

  • Keywords commit added

#11 @joedolson
15 months ago

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

In 55183:

Media: Ensure media IDs are type int in bulk editing.

Cast all array values to integers using array_map so functions like wp_delete_attachment receiving the expected variable type.

Props eherman24, joedolson.
Fixes #56170.

@joedolson commented on PR #3957:

15 months ago

Fixed in r55183

Note: See TracTickets for help on using tickets.