Make WordPress Core

Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#13917 closed task (blessed) (fixed)

wp.uploadFile cannot be attached to any post

Reported by: ciprian_vb's profile ciprian_vb Owned by: josephscott's profile josephscott
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: XML-RPC Keywords: has-patch commit mobile
Focuses: Cc:

Description

wp.uploadFile from xmlrpc.php saves each upload as an unattached file.

I wanted to attach it to a post once uploaded but a option for this does not exist and the $post_id is given the value 0 before wp_insert_attachment inside the $attachment array.

I am happy to make this enhancement if needed and approved but I'm not sure what the protocol is as this is my first time in doing this.

Thanks,
Ciprian Turcu

Attachments (4)

xmlrpc.diff (362 bytes) - added by djzone 13 years ago.
xmlrpc.2.diff (497 bytes) - added by josephscott 11 years ago.
13917-refresh.diff (503 bytes) - added by ericmann 11 years ago.
Refresh patch to match latest version of trunk.
13917-refresh.2.diff (685 bytes) - added by maxcutler 11 years ago.
Refresh + cap check

Download all attachments as: .zip

Change History (22)

#1 @nacin
13 years ago

  • Component changed from General to XML-RPC
  • Milestone changed from 3.0.1 to Future Release

@djzone
13 years ago

#2 @djzone
13 years ago

  • Keywords has-patch added

#3 @nacin
12 years ago

  • Owner set to josephscott
  • Status changed from new to assigned

#4 @josephscott
11 years ago

  • Keywords commit added

I'm fine with this change. I've included an updated diff.

#5 @ericmann
11 years ago

Updated diff looks good.

#6 @daniloercoli
11 years ago

  • Cc ercoli@… added

#7 @josephscott
11 years ago

  • Type changed from enhancement to task (blessed)

#8 @koke
11 years ago

  • Cc jbernal@… added
  • Keywords mobile added

#9 follow-up: @markoheijnen
11 years ago

  • Cc marko@… added

Patch is fine with me. I only wonder if there should be a check if the post id that is given does exists and it's post type is public (way it works now in the back-end)

@ericmann
11 years ago

Refresh patch to match latest version of trunk.

#10 in reply to: ↑ 9 @ericmann
11 years ago

Replying to markoheijnen:

Patch is fine with me. I only wonder if there should be a check if the post id that is given does exists and it's post type is public (way it works now in the back-end).

Actually, none of the references I can find to wp_insert_attachment(), which is the method we're using to insert the attachment, check this. They blindly pass along whatever post_id was passed in, which is typically $_REQUEST['post_id'] captured from somewhere else.

#11 follow-up: @markoheijnen
11 years ago

How I see it is that wp_insert_attachment() is a low level API so there are always checks needed. Same as for wp_insert_post.
So checking if it is allowed to do should be in the xmlrpc method. No clue how it got handled in the backend. I do now from the user interface you only can attach an image when the post type is a public one.

#12 in reply to: ↑ 11 @ericmann
11 years ago

Replying to markoheijnen:

How I see it is that wp_insert_attachment() is a low level API so there are always checks needed. Same as for wp_insert_post.
So checking if it is allowed to do should be in the xmlrpc method. No clue how it got handled in the backend. I do now from the user interface you only can attach an image when the post type is a public one.

I agree that checks need to be added somewhere but I disagree that this is the right place for it. Take a look at other refs to wp_insert_attachment() for example:

  • handle_upload() omits a post ID
  • create_attachment() takes a post ID of 0
  • wp_update_post() passes whatever ID it received, most often an unsanitized $_REQUEST['post_id']
  • media_handle_upload() handles things the same way as wp_update_post()
  • media_handle_sideload() also handle things the same way as wp_update_post()

So if we really need to be checking that the post exists and is public before attaching an image, that kind of logic should exist in wp_insert_attachment() itself, and should probably be a separate issue.

The front-end checks you're talking about are probably abstracted into the UI ...

#13 @markoheijnen
11 years ago

I disagree with that. low level API's shouldn't have a lot of checks. Same that wp_insert_post doesn't do a lot of capability checking.

I do have to say I'm not sure if checking if a post type is public is a the good way of doing that.
I guess also in terms of permission checking it also is weird handeld.

#14 @maxcutler
11 years ago

  • Cc max@… added

#15 @nacin
11 years ago

Needs an edit_post check to make sure they can upload a file to that post.

@maxcutler
11 years ago

Refresh + cap check

#16 @maxcutler
11 years ago

  • Milestone changed from Future Release to 3.5

Refreshed the patch again and added the edit_post cap check as requested.

#17 @nacin
11 years ago

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

In [21896]:

Allow wp.uploadFile to upload the attachment to a post. props djzone, josephscott, maxcutler. fixes #13917.

#18 @dd32
10 years ago

#25290 was marked as a duplicate.

Note: See TracTickets for help on using tickets.