Make WordPress Core

Opened 11 months ago

Closed 4 months ago

Last modified 4 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:

Description

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.

https://developer.wordpress.org/reference/hooks/deleted_post/

<?php
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.

<?php
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 11 months ago.
typecast-postid-bulk-delete-attachments-1.diff (473 bytes) - added by eherman24 11 months ago.
Updated diff.
56170.1.diff (1.4 KB) - added by joedolson 4 months ago.
Updated patch with alternate method

Download all attachments as: .zip

Change History (15)

#1 @eherman24
11 months ago

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

#2 @eherman24
11 months ago

This was on PHP 8.0.20

#3 @desrosj
8 months ago

  • Version trunk deleted

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


7 months ago

#5 @antpb
7 months ago

  • Milestone changed from Awaiting Review to 6.2

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


4 months ago

#7 @joedolson
4 months ago

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

@joedolson
4 months ago

Updated patch with alternate method

#8 @joedolson
4 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 code block, which also expect int arguments.

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

Last edited 4 months ago by joedolson (previous) (diff)

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


4 months ago
#9

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

Trac ticket: https://core.trac.wordpress.org/ticket/56170

#10 @joedolson
4 months ago

  • Keywords commit added

#11 @joedolson
4 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:


4 months ago
#12

Fixed in r55183

Note: See TracTickets for help on using tickets.