Make WordPress Core

Opened 13 years ago

Closed 8 years ago

Last modified 8 years ago

#19629 closed task (blessed) (fixed)

return option for media_sideload_image

Reported by: slbmeh's profile slbmeh Owned by: kirasong's profile kirasong
Milestone: 4.8 Priority: normal
Severity: minor Version: 3.3
Component: Media Keywords: needs-unit-tests has-patch
Focuses: Cc:

Description

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

media_sideload_image.diff (1.1 KB) - added by slbmeh 13 years ago.
19629.diff (1.1 KB) - added by kawauso 13 years ago.
Use $return_html instead
wp-sideload-media-id-ak.diff (2.8 KB) - added by alexkingorg 12 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 12 years ago.
Better abstracted function name
19629.2.diff (2.8 KB) - added by nacin 12 years ago.
19629.3.diff (2.8 KB) - added by trepmal 11 years ago.
19629.4.diff (5.1 KB) - added by mattheu 11 years ago.
19629.5.diff (8.1 KB) - added by mattheu 10 years ago.
Refresh + Unit Tests
19629.6.diff (8.0 KB) - added by mattheu 10 years ago.
19629-enhancement.diff (3.5 KB) - added by kraftbj 9 years ago.
Refresh patch and split enhancement+unit tests from deprecating existing function
19629-deprecated.diff (2.8 KB) - added by kraftbj 9 years ago.
Deprecate media_sideload_image and rewrite 4.2 era Press This
19629-deprecated.2.diff (5.0 KB) - added by kraftbj 9 years ago.
Deprecate media_sideload_image and rewrite 4.2 era Press This (missed picking up deprecated.php in 1st attempt)
media-sideload.diff (1.3 KB) - added by whyisjake 9 years ago.
Media sideload, return the post ID
wordpress_38938.diff (1.5 KB) - added by dotancohen 8 years ago.
Patch to optionally return the ID of a sideloaded media file.
sideload_id_19629.diff (1.2 KB) - added by MrGregWaugh 8 years ago.
Patch to return id that includes is_wp_error() check
sideload_id_19629.2.diff (1.3 KB) - added by kirasong 8 years ago.
Adding @since for feature and applies from src/
19629.tests.diff (3.6 KB) - added by kirasong 8 years ago.
A first pass at tests

Download all attachments as: .zip

Change History (65)

#1 @mfields
13 years ago

  • Cc michael@… added

#2 @SergeyBiryukov
13 years ago

  • Keywords has-patch added

#3 @kawauso
13 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 13 years ago by kawauso (previous) (diff)

@kawauso
13 years ago

Use $return_html instead

#4 @nacin
12 years ago

Related, #15432?

@alexkingorg
12 years ago

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

#5 follow-up: @alexkingorg
12 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

#6 in reply to: ↑ 5 @SergeyBiryukov
12 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 12 years ago by SergeyBiryukov (previous) (diff)

@alexkingorg
12 years ago

Better abstracted function name

#7 @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.5

+1 to wp_sideload_image().

#8 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

#9 @evansolomon
12 years ago

  • Cc evan@… added

#10 @trepmal
12 years ago

  • Cc trepmal@… added

#11 follow-up: @georgestephanis
12 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.

#12 in reply to: ↑ 11 @DrewAPicture
12 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().

#13 @nacin
12 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.

@nacin
12 years ago

#14 @nacin
12 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.

@trepmal
11 years ago

#15 @trepmal
11 years ago

  • Keywords 3.7-early added; 3.6-early removed

Updated nacin's patch to escape question mark in regex

#16 @mattheu
11 years 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?

@mattheu
11 years ago

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


11 years ago

@mattheu
10 years ago

Refresh + Unit Tests

#18 @mattheu
10 years 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

@mattheu
10 years ago

#19 @mattheu
10 years ago

Refreshed patch to merge cleanly

#20 @dd32
9 years ago

#32695 was marked as a duplicate.

@kraftbj
9 years ago

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

@kraftbj
9 years ago

Deprecate media_sideload_image and rewrite 4.2 era Press This

@kraftbj
9 years ago

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

#21 @kraftbj
9 years ago

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.

#22 @SergeyBiryukov
9 years ago

#33879 was marked as a duplicate.

This ticket was mentioned in Slack in #core by travisnorthcutt. View the logs.


9 years ago

#24 @travisnorthcutt
9 years ago

  • Keywords 3.7-early removed

@whyisjake
9 years ago

Media sideload, return the post ID

#25 @whyisjake
9 years ago

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

#26 @mbcreation
8 years ago

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 :)

This ticket was mentioned in Slack in #core-images by kevinwhoffman. View the logs.


8 years ago

#28 @swissspidy
8 years ago

#38938 was marked as a duplicate.

@dotancohen
8 years ago

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

#29 @dotancohen
8 years ago

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.

#30 @dotancohen
8 years ago

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
8 years ago

Patch to return id that includes is_wp_error() check

#31 @danielbachhuber
8 years ago

  • Milestone changed from Future Release to 4.8

sideload_id_19629.diff looks good to me

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

@kirasong
8 years ago

Adding @since for feature and applies from src/

#34 @kirasong
8 years ago

  • Keywords needs-unit-tests added
  • Owner set to mikeschroder
  • Status changed from new to assigned

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

#35 @kirasong
8 years ago

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

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.

#36 @kirasong
8 years ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for tests.

#37 @joemcgill
8 years ago

  • Type changed from enhancement to task (blessed)

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

This ticket was mentioned in Slack in #core by obenland. View the logs.


8 years ago

#39 @obenland
8 years ago

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

This ticket was mentioned in Slack in #core by obenland. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


8 years ago

@kirasong
8 years ago

A first pass at tests

#43 @kirasong
8 years ago

  • Keywords has-patch added

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.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by matias. View the logs.


8 years ago

#47 @kirasong
8 years ago

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

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.

#48 @jnylen0
8 years ago

In 40844:

Revert [40842] due to failing tests.

See #19629.

Note: See TracTickets for help on using tickets.