WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 16 months ago

#20948 new defect (bug)

Unnecessary post type check in wp_get_attachment_url

Reported by: jfarthing84 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.4
Component: Media Keywords: dev-feedback has-patch
Focuses: Cc:

Description

Not sure why this needs to be done. You wouldn't be calling this function on a post if it wasn't an attachment. If you are, then you must have a good reason to be doing so, like I do.

Attachments (1)

post.php.patch (520 bytes) - added by jfarthing84 22 months ago.
Remove post type check from wp_get_attachment_url.

Download all attachments as: .zip

Change History (7)

jfarthing8422 months ago

Remove post type check from wp_get_attachment_url.

comment:1 ocean9022 months ago

If you are, then you must have a good reason to be doing so, like I do.

And the reason is?

comment:2 follow-up: jfarthing8422 months ago

Well, I have a custom post type for user uploads. This post type should be treated exactly like an attachment. As far as I can tell so far, wp_get_attachment_url is the only "media" function that does this check, and it hinders other functions from working on a post type other than "attachment" (like image_downsize).

Why don't I just use the attachment post type? Well, there's a few reasons for that. The main reason is because you can't assign any post status to an attachment. WordPress forces it to be "inherit" or "private".

Last edited 22 months ago by jfarthing84 (previous) (diff)

comment:3 jfarthing8416 months ago

Any reason we can't get this done?

comment:4 in reply to: ↑ 2 nacin16 months ago

Replying to jfarthing84:

Well, I have a custom post type for user uploads. This post type should be treated exactly like an attachment. As far as I can tell so far, wp_get_attachment_url is the only "media" function that does this check, and it hinders other functions from working on a post type other than "attachment" (like image_downsize).

Attachments are designed to be a special type in WordPress. We do 'attachment' checks frequently (especially on endpoints — look at some recent functions added to ajax-actions.php) to avoid (in part) potential information disclosure based on someone requesting an "attachment ID" of a non-attachment.

Why don't I just use the attachment post type? Well, there's a few reasons for that. The main reason is because you can't assign any post status to an attachment. WordPress forces it to be "inherit" or "private".

Any other reasons? That's not true, it can also be public, and it can be done programmatically otherwise. User uploads should probably go through the attachment post type, or they should be a post type that stores your information and then the attachment post type holds the information for you.

I'm not saying this itself is a bad idea. I don't think it is. But if we do this, we are essentially saying we will always support attachment-specific functions to work on non-attachments, and that is just not something I think we should be guaranteeing. It requires us to support something we don't really care to support. We'll either find a situation where we can't, or we'll break the paradigm accidentally later on. So it's not really fair to either of us.

comment:5 jfarthing8416 months ago

I just don't like the idea of needing two rows in the database per each upload. That's really not needed. Of course, I could also just write my own versions of the WP attachment functions, but that is not really needed also. The image_downsize function provides a handy filter before it runs, allowing plugins to short-circuit it and return their own image. Perhaps something like this can be added there instead?

comment:6 jfarthing8416 months ago

That's not true, it can also be public, and it can be done programmatically otherwise.

So, what about this check in wp_insert_attachment?

if ( ! in_array( $post_status, array( 'inherit', 'private' ) ) )
	$post_status = 'inherit';
Note: See TracTickets for help on using tickets.