WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 16 months ago

#23711 new enhancement

file include wrapper for media_sideload_image

Reported by: norcross Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch dev-feedback 2nd-opinion
Focuses: Cc:

Description

Currently, when using media_sideload_image outside of wp-admin, you are required to load three extra files, as explained at the bottom of this codex page.

This function simply calls those three files, but will allow flexibility in future versions if those file dependencies change.

Attachments (1)

media.php.diff (636 bytes) - added by norcross 16 months ago.
updated to use ABSPATH method

Download all attachments as: .zip

Change History (12)

comment:1 mordauk16 months ago

  • Cc pippin@… added

comment:2 mordauk16 months ago

+1.

While it's obviously not difficult to include the necessary files, the idea of being able to more easily move files / code around without having to worry (as much) about breaking plugins / themes seems like a win win to me.

comment:3 SergeyBiryukov16 months ago

  • Version changed from trunk to 3.5

media.php.diff wouldn't work, you can't require() a file by its URL like that. ABSPATH should be used instead.

If we want to make it easier to use media_sideload_image() on front-end though, I guess it's better to move it to wp-includes, and either make the function itself require the necessary files, or move it along with the functions it depends on.

comment:4 mordauk16 months ago

My vote would be to have the function include the necessary files.

comment:5 markoheijnen16 months ago

A function that includes the necessary files is a hack to me and should not be in core. So I agree with Sergey on this.

comment:6 mordauk16 months ago

I don't really see how that's a hack. It's efficient and makes using media_sideload_image simpler.

comment:7 norcross16 months ago

the problem is that to use media_sideload_image on the front end, these files have to be included. so any theme / plugin that makes use of this is suspect to changes in core. by creating this wrapper (and Sergey is correct with the ABSPATH part, not sure why it worked on my local) it allows core to make changes down the road without breaking existing sites.

also, I don't see it as a hack. if anything, the current method (having to manually call those files) is more of a hack.

norcross16 months ago

updated to use ABSPATH method

comment:8 markoheijnen16 months ago

Moving the functions to wp-includes shouldn't break things. People who are now using it in the front end only would load unnecessary files.

You are right that the current way of doing is a hack. I rather would duplicate the code because when you include wp-admin files in the front end and it breaks then it is you to blame and not WordPress. See when WP_Screen was introduced.

comment:9 follow-up: nacin16 months ago

If you want to use an admin function on the frontend, the only proper way to do that is to include wp-admin/includes/admin.php. Otherwise you risk breakage from wp-admin movement, and I'm not responsible for that.

I think this is a function worthy of wp-includes. But, as it stands now, it is an unwieldy function. I'd like to introduce something a little better if we're going to shift it. There is a ticket from alexkingorg regarding this function.

comment:10 nacin16 months ago

Also, the "proper" way to patch this using the include-only-what-is-needed suggestion would be to place these these require_once calls directly at the top of media_sideload_image(). It'd be weird to do that for only this function, though.

comment:11 in reply to: ↑ 9 SergeyBiryukov16 months ago

Replying to nacin:

There is a ticket from alexkingorg regarding this function.

Related: #19629

Note: See TracTickets for help on using tickets.