return option for media_sideload_image

[slbmeh]

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.

[kawauso]

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.

[kawauso]

Use $return_html instead

[nacin]

Related, #15432?

[alexkingorg]

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

[alexkingorg]

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

[SergeyBiryukov]

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.

[alexkingorg]

Better abstracted function name

[nacin]

+1 to wp_sideload_image().

[georgestephanis]

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.

[DrewAPicture]

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().

[nacin]

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.

[nacin]

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.

[trepmal]

Updated nacin's patch to escape question mark in regex

[mattheu]

+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?

[mattheu]

Refresh + Unit Tests

[mattheu]

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

[mattheu]

Refreshed patch to merge cleanly

[dd32]

#32695 was marked as a duplicate.

[kraftbj]

Refresh patch and split enhancement+unit tests from deprecating existing function

[kraftbj]

Deprecate media_sideload_image and rewrite 4.2 era Press This

[kraftbj]

Deprecate media_sideload_image and rewrite 4.2 era Press This (missed picking up deprecated.php in 1st attempt)

[kraftbj]

I refreshed the patch and split them, since enhancing one function doesn't require the deprecation of the other, as well as hopefully to stale-proof some of the patch.

This would be a nice enhancement to have in place.

[SergeyBiryukov]

#33879 was marked as a duplicate.

[whyisjake]

Media sideload, return the post ID

[whyisjake]

Any updates on this? Would be handy to return the ID...

[mbcreation]

Hi everyone,

For now, the only optionsseems to query the database (postmeta) searching for the post id matching the url of the upload...

The addition seems pretty easy since we have the $id in the function ?
Or maybe i'm doing what i'm doing the wrong way :)

[swissspidy]

#38938 was marked as a duplicate.

[dotancohen]

Patch to optionally return the ID of a sideloaded media file.

[dotancohen]

I've uploaded a diff which applies cleanly to the current Wordpress version.

Note that when the OP wrote the original patch five years ago, there was no $return parameter. Now that the method already has this parameter my patch is much less invasive and does not change the API or function signature.

[dotancohen]

I just took a look at the patch submitted by @whyisjake. Though similar to my patch, it returns before the is_wp_error( $id ) check, which is a necessary check in my opinion.

[MrGregWaugh]

Patch to return id that includes is_wp_error() check

[danielbachhuber]

sideload_id_19629.diff looks good to me

[kirasong]

Adding @since for feature and applies from src/

[kirasong]

Added @since for the parameter option and patch now applies to src/ in sideload_id_19629.2.diff​.

[kirasong]

In 40597:

Media: Optionally return attachment id from media_sideload_image().

Introduces option to $return parameter to return the attachment id created after a successful image sideload.

Props slbmeh, kawauso, alexkingorg, SergeyBiryukov, georgestephanis, DrewAPicture, nacin, trepmal, mattheu, kraftbj, whyisjake, dotancohen, MrGregWaugh, danielbachhuber.
Fixes #19629.

[kirasong]

Re-opening for tests.

[joemcgill]

Moving this from enhancement to task/blessed to track adding additional tests.

[obenland]

@mikeschroder Could you bring this over the finish line when you get a chance?

[kirasong]

A first pass at tests

[kirasong]

A first pass at tests for media_sideload_image() in 19629.tests.diff​.

A couple notes:

• This feels a bit hacky, so if there's a better way to catch some of the data, anything can be changed here.
• It's possible this should live in tests/phpunit/tests/media.php instead of its own file.

I'm pretty tired at this point, so going to wait for some feedback and do another review on it tomorrow before a commit.

If I'm not around at the proper time before RC, feel free to either commit (whether with changes or not), or punt this ticket if it seems necessary.

[kirasong]

In 40842:

Media: Introduce tests for media_sideload_image().

Adds basic tests for media_sideload_image(), including testing
the return arguments.

Props westonruter, mikeschroder.
Fixes #19629.

[jnylen0]

In 40844:

Revert [40842] due to failing tests.

See #19629.