WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 15 months ago

Last modified 15 months ago

#20948 closed defect (bug) (wontfix)

Unnecessary post type check in wp_get_attachment_url

Reported by: jfarthing84 Owned by:
Milestone: 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 3 years ago.
Remove post type check from wp_get_attachment_url.

Download all attachments as: .zip

Change History (10)

@jfarthing843 years ago

Remove post type check from wp_get_attachment_url.

comment:1 @ocean903 years 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: @jfarthing843 years 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 3 years ago by jfarthing84 (previous) (diff)

comment:3 @jfarthing843 years ago

Any reason we can't get this done?

comment:4 in reply to: ↑ 2 ; follow-up: @nacin3 years 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 @jfarthing843 years 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 @jfarthing843 years 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';

comment:7 @ericlewis15 months ago

Related: #17255 (Draft status for media files)

comment:8 in reply to: ↑ 4 @ericlewis15 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

Replying to nacin:

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.

This greatly outweighs the counter-argument being made here.

If one wanted to make their own post type with similar functionality to attachments, you can by all means create your own functions that do exactly what you want them to do, copying and editing where the attachments API doesn't fit your model. You're not limited by WordPress, you just won't benefit from WordPress (in this respect) because of your use-case.

Closing out as wontfix.

comment:9 @DrewAPicture15 months ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.