WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15432 closed defect (bug) (fixed)

media_handle_sideload() is not returning right value

Reported by: acsnaterse Owned by: joostdevalk
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0.1
Component: Media Keywords: has-patch
Focuses: Cc:

Description

The media_handle_sideload() is not return the right value. Just like the media_handle_upload you would expect the $id as integer return value, but it returns the URL string.

This is happening in the last lines of the function:

// Save the attachment metadata
$id = wp_insert_attachment($attachment, $file, $post_id);
	
if ( !is_wp_error($id) ) {
	wp_update_attachment_metadata( $id, wp_generate_attachment_metadata( $id, $file ) );
	return $url;
}
return $id;

Also where this function is used ( in media_sideload_image() ) it is expecting the ID as return value.

Attachments (2)

media_handle_sideload.patch (1.2 KB) - added by joostdevalk 3 years ago.
Patch
media_handle_sideload.2.patch (1.8 KB) - added by joostdevalk 3 years ago.
Same patch now with documentation fixed too

Download all attachments as: .zip

Change History (11)

joostdevalk3 years ago

Patch

comment:1 joostdevalk3 years ago

  • Keywords has-patch added

Attached patch fixes return and makes sure the single function in core that uses media_handle_sideload work properly again. Seriously, if you write this:

 // do the validation and storage stuff
$id = media_handle_sideload($file_array, $post_id, @$desc);
$src = $id;

You have to know you're doing something wrong, right? :)

joostdevalk3 years ago

Same patch now with documentation fixed too

comment:2 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:3 scribu3 years ago

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

comment:4 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is an API change. Not convinced it is simply a bug fix.

comment:5 joostdevalk3 years ago

Well it is an API change: it returns $id instead of $url, where $url was the $fileurl? that the function took as an input before, so it might change some things. The one instance where that return value was used in WP has been fixed with the same patch too, if you think that's not enough, let's see how we should approach this.

comment:6 scribu3 years ago

Indeed, if a plugin was using media_handle_sideload(), it would probably break.

Fortunately, it isn't a very well known function, so I think we can get away with fixing it before it becomes better known.

I'll do a grep on my local checkout of the plugins on WP.org later today.

comment:7 scribu3 years ago

Grepped through 4855 plugins and couldn't find a single call to media_handle_sideload.

This is the command I used:

grep --exclude-dir=.svn --include=*.php -r 'media_handle_sideload' .

comment:8 nacin3 years ago

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

comment:9 transom3 years ago

Thanks for the regression bug (grumble) but I will submit a enhancement for media_sideload_image shortly. This is handy function that is really handy for imports.

Note: See TracTickets for help on using tickets.