Make WordPress Core

Opened 16 years ago

Closed 16 years ago

Last modified 15 years ago

#15432 closed defect (bug) (fixed)

media_handle_sideload() is not returning right value

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

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

Download all attachments as: .zip

Change History (11)

#1 @joostdevalk
16 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
16 years ago

Same patch now with documentation fixed too

#2 @scribu
16 years ago

  • Milestone Awaiting Review3.1

#3 @scribu
16 years ago

  • Resolutionfixed
  • Status newclosed

#4 @nacin
16 years ago

  • Resolution fixed
  • Status closedreopened

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

#5 @joostdevalk
16 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
16 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
16 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
16 years ago

  • Resolutionfixed
  • Status reopenedclosed

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