WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 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:
PR Number:

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 9 years ago.
Patch
media_handle_sideload.2.patch (1.8 KB) - added by joostdevalk 9 years ago.
Same patch now with documentation fixed too

Download all attachments as: .zip

Change History (11)

#1 @joostdevalk
9 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? :)

@joostdevalk
9 years ago

Same patch now with documentation fixed too

#2 @scribu
9 years ago

  • Milestone changed from Awaiting Review to 3.1

#3 @scribu
9 years ago

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

#4 @nacin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#5 @joostdevalk
9 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.

#6 @scribu
9 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.

#7 @scribu
9 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' .

#8 @nacin
9 years ago

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

#9 @transom
9 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.