Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#19629 new enhancement

return option for media_sideload_image

Reported by: slbmeh Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 3.3
Component: Media Keywords: has-patch 3.7-early
Focuses: Cc:


An optional parameter to return the attachment id opposed to the html would be beneficial to those attempting to make use of this functionality outside of press this.

Ticket: #11993 has an enhancement suggestion to allow adding thumbnails from URL. I have written a solution for this enhancement, and the result duplicates the functionality of media_sideload_image() setting the post thumbnail opposed to returning the image html markup.

Attachments (9)

media_sideload_image.diff (1.1 KB) - added by slbmeh 3 years ago.
19629.diff (1.1 KB) - added by kawauso 3 years ago.
Use $return_html instead
wp-sideload-media-id-ak.diff (2.8 KB) - added by alexkingorg 3 years ago.
abstract out the sideloading of the image so the ID can be accessed
wp-sideload-media-id-ak.2.diff (2.8 KB) - added by alexkingorg 3 years ago.
Better abstracted function name
19629.2.diff (2.8 KB) - added by nacin 3 years ago.
19629.3.diff (2.8 KB) - added by trepmal 21 months ago.
19629.4.diff (5.1 KB) - added by mattheu 14 months ago.
19629.5.diff (8.1 KB) - added by mattheu 11 months ago.
Refresh + Unit Tests
19629.6.diff (8.0 KB) - added by mattheu 8 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 @mfields3 years ago

  • Cc michael@… added

comment:2 @SergeyBiryukov3 years ago

  • Keywords has-patch added

comment:3 @kawauso3 years ago

media_sideload_image.diff reuses the existing variable $html which would lead to HTML always being returned. Also the variable type given in the PHPDoc is wrong.

Last edited 3 years ago by kawauso (previous) (diff)

@kawauso3 years ago

Use $return_html instead

comment:4 @nacin3 years ago

Related, #15432?

@alexkingorg3 years ago

abstract out the sideloading of the image so the ID can be accessed

comment:5 follow-up: @alexkingorg3 years ago

Here is an alternative implementation. Instead of the additional arg to change the return, refactor the function into two functions. One returns the ID (more efficient, since if you just need the ID it doesn't need to call wp_get_attachment_url(), etc.), the other calls this function to get the ID and returns the HTML (maintains current behavior).

Without this change, it is difficult to implement the following functionality while utilizing core functions:

  1. sideload an image
  2. set the image as the featured images for the post

comment:6 in reply to: ↑ 5 @SergeyBiryukov3 years ago

Replying to alexkingorg:

Without this change, it is difficult to implement the following functionality while utilizing core functions:

  1. sideload an image
  2. set the image as the featured images for the post

I had to reimplement media_sideload_image() a couple of times for this exact use case.

The approach in wp-sideload-media-id-ak.diff makes sense to me.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

@alexkingorg3 years ago

Better abstracted function name

comment:7 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.5

+1 to wp_sideload_image().

comment:8 @DrewAPicture3 years ago

  • Cc xoodrew@… added

comment:9 @evansolomon3 years ago

  • Cc evan@… added

comment:10 @trepmal3 years ago

  • Cc trepmal@… added

comment:11 follow-up: @georgestephanis3 years ago

I like it, but I would think we'd want to call the variable $url instead of $file to avoid confusion? Happy to write a patch revision if anyone else agrees.

comment:12 in reply to: ↑ 11 @DrewAPicture3 years ago

Replying to georgestephanis:

I like it, but I would think we'd want to call the variable $url instead of $file to avoid confusion?

I think $file was used for consistency with media_sideload_image().

comment:13 @nacin3 years ago

Some explanation of the existing stack:

  • wp_handle_upload() is the underlying function that handles an upload of any file in WordPress, writing it to disk, etc.
  • wp_handle_sideload() is the equivalent underlying function.
  • media_handle_upload() takes $_POST/$_FILES and passes it to wp_upload_image().
  • media_handle_sideload() is the equivalent function, taking a $_FILES array and passing it to wp_sideload_image().

Then, there is media_sideload_image(), which is more or less a glorified wrapper for media_handle_sideload():

  • It accepts a URL, which then goes to download_url(), builds a $_FILES-like array of local files, then passes it along.
  • Only works on images.
  • Returns an <img> tag.

Now, media_handle_upload()'s first argument is $file_id, which is the key to the corresponding file in $_FILES. But media_handle_sideload() takes the full array. This is because it isn't actually using $_FILES, instead just taking an argument that looks like it.

Since media_handle_sideload() takes an array currently, we can actually get away with allowing a URL to be passed instead. The benefits here are many fold:

  • We don't need to introduce a new function, which would cause imbalance to the current stack.
  • media_handle_sideload() ends up taking a saner argument of a URL to sideload.
  • The existing function already works on any kind of file, not just images.
  • The existing function already returns an ID.

We're not prevented, of course, from adding new highest-level functions like wp_upload_media() (or wp_upload_file()) and wp_sideload_media() in the future.

Doing this will probably mean a deprecation of media_sideload_image() in favor of media_handle_sideload(). Its single usage in Press This could become a call to media_handle_sideload() followed by a call to wp_get_attachment_url(), as it does now.

@nacin3 years ago

comment:14 @nacin3 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

Suggested patch attached. Since this is not critical and there exist workarounds (namely, using download_url() and media_handle_sideload() manually), moving to 3.6. Happy to resolve this early.

@trepmal21 months ago

comment:15 @trepmal21 months ago

  • Keywords 3.7-early added; 3.6-early removed

Updated nacin's patch to escape question mark in regex

comment:16 @mattheu14 months ago

+1 on this - would be super useful. I like Nacins suggestion is good and I tested 19629.3.diff by trepmal which still works well.

I have updated the patch to deprecate media_sideload_image, and updated the press this function make use of media_handle_sideload directly.

I am also using wp_get_attachment_image rather than outputting its own custom HTML - it seems nicer to me as it adds correct classes and width/height attributes.

Was flagged for 3.6-early, then 3.7-early. Any other concerns blocking this ticket?

@mattheu14 months ago

comment:17 @ircbot14 months ago

This ticket was mentioned in IRC in #wordpress-dev by Matth_eu. View the logs.

@mattheu11 months ago

Refresh + Unit Tests

comment:18 @mattheu11 months ago

Updated this to merge cleanly and add unit tests for media_handle_sideload.

Tests for calling with either file url string or file array and when passing URL that is 404.

Also fixed potential error by passing value returned by media_handle_sideload directly to wp_get_attachment_image

@mattheu8 months ago

comment:19 @mattheu8 months ago

Refreshed patch to merge cleanly

Note: See TracTickets for help on using tickets.