Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#36941 closed defect (bug) (invalid)

media_sideload_image doesn't unlink tempfile on success

Reported by: ruudjoyo's profile ruud@… Owned by: dd32's profile dd32
Milestone: Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Hi,

related to #34772 I was looking for more examples of not unlinking the tempfile. I think I found one in the media_sideload_image() method.

after media_handle_sideload() is called successfully (line 875 in media.php), the $file_array[tmp_name] is only unlinked after a WP_ERROR is thrown, but not in the normal successful case at all.

Attachments (1)

media.patch (669 bytes) - added by ruud@… 8 years ago.
Patch proposal for moving the unlink directly after the sideload call

Download all attachments as: .zip

Change History (7)

@ruud@…
8 years ago

Patch proposal for moving the unlink directly after the sideload call

#1 @ruud@…
8 years ago

  • Keywords has-patch dev-feedback added

#2 @ruud@…
8 years ago

related to #36942 and #36943

#3 @ocean90
8 years ago

  • Version trunk deleted

#4 @dd32
8 years ago

  • Keywords needs-testing added; dev-feedback removed
  • Milestone changed from Awaiting Review to 4.6
  • Owner set to dd32
  • Status changed from new to accepted

This patch looks correct at first glance, but I'll need to test that removing the temporary file here is not going to affect behaviour incorrectly somewhere.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#6 @ocean90
8 years ago

  • Keywords needs-testing removed
  • Milestone 4.6 deleted
  • Resolution set to invalid
  • Status changed from accepted to closed

media_handle_sideload() passes the file array to wp_handle_sideload() which uses _wp_handle_upload(). _wp_handle_upload() (re)moves the temporary file on success, see https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php?rev=38015&marks=373,376#L370.

That's why media_handle_sideload() only needs to handle the failure case.

Note: See TracTickets for help on using tickets.