#18429 closed task (blessed) (fixed)
Create custom post types via XMLRPC
Reported by: | nprasath002 | Owned by: | westi |
---|---|---|---|
Milestone: | 3.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | XML-RPC | Keywords: | has-patch |
Focuses: | Cc: |
Description
Supports any post type
Attachments (26)
Change History (110)
#3
@
13 years ago
- Milestone changed from Awaiting Review to 3.4
- Owner set to westi
- Status changed from new to reviewing
- Type changed from feature request to task (blessed)
#4
follow-up:
↓ 5
@
13 years ago
- Cc marko@… added
- Should we have a default post_status or do we really need to return an error
- Should we generate an error if page templates aren't supported
- Should we generate an error on invalid comment status or ping status
- Not sure about the implementation of post_more. Shouldn't the client handle this instead of the xml-rpc?
- Shouldn't the checks after wp_insert_post not be before it. Sounds like it can give weird situation
- Do we need the "handle enclosures" code in the end of the function. Shouldn't this be a separate function?
@
13 years ago
Redone the first patch to be a wrapper for wp_insert_post. Removed a lot of checks and replaced code by WordPres functionality
#5
in reply to:
↑ 4
@
13 years ago
Replying to markoheijnen:
In XMRPC we are adopting the same behaviour as in the WordPress backend.
- Should we have a default post_status or do we really need to return an error
In the back end they do have a default post status for posts. So yes!!
- Should we generate an error if page templates aren't supported
I think yes. The client must know whether the request is valid
- Should we generate an error on invalid comment status or ping status
Again yes
- Not sure about the implementation of post_more. Shouldn't the client handle this instead of the xml-rpc?
- Shouldn't the checks after wp_insert_post not be before it. Sounds like it can give weird situation
- Do we need the "handle enclosures" code in the end of the function. Shouldn't this be a separate function?
This method is largely influenced by mw_newPost() for consistency and backward capability. So i adopted the same behaviour
#6
@
13 years ago
We need to check all the parameters in and return proper error messages to the client.
For example a post type may/may not support hierarchical post.
We need to check within the function itself.
wp_insert_post does not handle this
#7
@
13 years ago
I know mw_newPost looked a lot at your patch. With the patch I wrote I tried to make the code as simple as I found needed. To have a discussing about what needs to be checked on the client side and what on the XML-RPC side.
The reason I removed checks on page templates and hierarchical post is because I think the client should check for it.
I did discussed with westi about checking of post_parent. Reason that is removed is because post_parent doesn't have to be of the same post_type. Something that happens with media item. From that point I also removed checking if a post is hierarchical.
#8
@
13 years ago
Should look at the end of the 2 week cycle to ticket #17109 to maybe add the needed code.
#9
@
13 years ago
Some feedback on the patch:
- It looks like maybe some post editing code slipped into wp.newPost - I think we should only need to support the supplying of
$post_data['ID']
for wp.editPost. - In the Taxonomy/Terms section of the code it is unclear whether you expect the client to pass in category/tag names or IDs - I think names are preferable and it would be clearer to name things like {{term_id}} as {{term_name}}
- Minor coding style variable naming issue -
$thisEnclosure
Future:
- We should consider if we should auto-create new tags/cats based on what the user has entered in the App UI so as to save on multiple requests - If I want to tag my post with the tag "Awesome" then I don't think we should reject that but instead create the tag as a result of the newPost call.
#10
@
13 years ago
I will look into the taxonomy code today. It's now on id's. I'm not sure if names will work when the taxonomy is hierarchical. Since you can have the same name when it isn't on the same level.
@
13 years ago
- Method now only can be used for new posts - Terms now can be added with id or name. Name only works for non-hierarchical taxonomies to avoid confusion where there may be another child with the same name. - When a term doesn't exists it will get added. - There is now a hard check if the terms array have integers. Not sure why it is needed since xmlrpc already says the value is an integer
#11
@
13 years ago
For readability yet again whole the text
- Method now only can be used for new posts
- Terms now can be added with id or name. Name only works for non-hierarchical taxonomies to avoid confusion where there may be another child with the same name.
- When a term doesn't exists it will get added.
- There is now a hard check if the terms array have integers. Not sure why it is needed since xmlrpc already says the value is an integer
#12
@
13 years ago
We must add a cap check for wp_password to mimic the wp backend behaviour.
Currenty contributors cannot create password protects posts in the backend
#14
@
13 years ago
Why the option for providing an author_id was removed?? current mw methods provides that.
#15
@
13 years ago
Custom post types allows us to specify post type support (title, description). The new method allows us to set these properties even it is not supported in the core. We need to validate this before using wp_insert_post
#17
@
13 years ago
if ($post_data['post_type'] == 'post') { if (!current_user_can($post_type->cap->edit_others_posts)) return new IXR_Error(401, __('Sorry, you are not allowed to stick this post.')); $sticky = $post_data['sticky'] ? true : false; if ($sticky) { if ($post_data['post_status'] != 'publish') return new IXR_Error(401, __('Only published posts can be made sticky.')); stick_post($post_data['ID']); } else { unstick_post($post_data['ID']); } }
Consider this code segment.
The cap check is in the wrong point.
We must validate the cap if the post is set to sticky.
#18
@
13 years ago
The password can be given if you are a contributor. How I looked into it was that the user still can't post it so someone else need to publish it. wp backend behaviour should be mimic indeed. So there should be an cap check over there.
author_id still can be given trough post_author. Not sure if we should mimic mw inputs for that. Don't think so.
My goal for the patch was to create a wrapper around wp_insert_post and that uses post_author as input.
Title and description are allowed to be posted since I think the client should check for it. This is something that should be open for discussion for the next office hour.
The comment and ping status variables probably should be validated. I don't think we should give an error if the status isn't valid but return the default option: default_comment_status / default_ping_status.
About the cap check the validation is on the right spot. Since someone without the cap should also not be have the ability to make a post not sticky. In the current case the else statement will be removed since it is about a new post so it will always be unsticky.
#19
@
13 years ago
Submitted a new patch:
- Added a capability check for password
- Added comment and ping status validation
- Changed sticky code. Fix validation for sticky post
- Date code added
- Redo the whole taxonomy code
- Changes variable name from terms to tax_input. I override the default wp_insert_post implementation with an own because of better capability checking
- Check for the capability to assign or create a term
- Add taxonomy with names when it is hierarchical and there is no collision
Future:
- Maybe the error messages should be changes.
- Possible upload functionality but first need feedback from app developers for how they want it to be. It's probably not handy to have it in one method.
- Maybe a variable to add images to the gallery
#20
@
13 years ago
To give author id we must validate cap edit_others_posts.
comment/ping status validation has $content_struct instead of $post_data
#22
@
13 years ago
- Cc max@… added
I know mw_newPost
uses get_default_post_to_edit
to get an auto-draft to work on before calling wp_insert_post
, but I'm wondering if that's something we really want to copy. Auto-drafts get cleaned out after seven days, but that can leave a significant amount of cruft behind in the meantime if wp_insert_post
fails.
I think I'd prefer if we validated everything before calling wp_insert_post
, and then only performed the saving for sticky, taxonomy terms, post format, custom fields, and enclosures after the call, when the post truly exists.
Other minor things:
- The 'xmlrpc_call'
do_action
should be moved up so that it's immediately after the login check (to be consistent with all of the other methods). - There are duplicate
absint( $post_data['post_author'] )
calls which should be removed in favor of a single instance before theif ( isset(...))
. Because even in theelse
branch, we want that field to be cast to an int. - Should we return an error if
tax_input
is not an array? Right now thatif
has noelse
. - There's an undefined variable
$terms_needed
used in the term handling code. - Can you separate the complex
if
statements forcomment_status
andping_status
? Right now it's hard to read, it'd probably be better with anif ( isset(...))
and a nestedif
testing the value. Speaking of which, should we return an error for invalid statuses, or is silently ignoring it okay? I know mw_newPost ignores it, but that doesn't mean wp_newPost has to. - The existing wp_* methods use
blog_id
instead ofblog_ID
. It doesn't really matter since it's not used, but consistency is a good thing. - I noticed some WP coding style violations, especially with lack of spaces around
!
.
One final thought: it might be nice to have a filter on $post_data
right before the call to wp_insert_post
, with content_struct
as an argument. Might also want to consider something like in #17109.
#23
@
13 years ago
I hope I got time to put your feedback into a new patch before the office hour.
About the use of get_default_post_to_edit. I was in a two fight. Now all the code is with each other.
Not needing it was my plan but then you split things like the taxonomy terms. Something that is also ugly in my point of view.
- post_author check can indeed be better. else branch doesn't need absint because of the user variable.
- I don't know if we should give an error. maybe we do. Curious what westi thinks about that point
- I will look into the undefined $terms_needed term.
- I will seperate the complex if statement. I don't think we should give an error when it is invalid.
- Will change blog_ID to blog_id
- I didn't knew at all that there needs to be a spaces around !. Will do that
The filters sounds as a good idea.
#24
@
13 years ago
Added a new patch. Bigest thing is removing get_default_post_to_edit. So some checks need to go after wp_insert_post.
This isn't a bad thing since I found out that setting the terms for category before wp_insert_post didn't work as aspected. Moving it after wp_insert_post did worked out the issue.
Still need to talk about adding filters
#25
@
13 years ago
I know mw_newPost uses get_default_post_to_edit to get an auto-draft to work on before calling wp_insert_post, but I'm wondering if that's something we really want to copy. Auto-drafts get cleaned out after seven days, but that can leave a significant amount of cruft behind in the meantime if wp_insert_post fails.
We'll probably move that off to cron cleanup at some point (#19663), but either way, don't worry about garbage collection for auto-drafts, as that's a general core quirk, and not something XML-RPC should work around.
get_default_post_to_edit() is necessary in part so you do not publish a post before attaching all of the associated data, in particular taxonomies and post formats.
See #18812 for XML-RPC-specific reasons for why get_default_post_to_edit() is now used here, and see #16192 for how this was a bug in Press This.
@
13 years ago
Insert get_default_post_to_edit back again and made wp_insert_post the last step of the method
#26
@
13 years ago
Also inserted a hackish code for category handling. Need some more testing for other hierarchical taxonomies.
Not sure if it is needed there to do the extra step to make the term names to id's
#27
@
13 years ago
Updated the patch with several fixes:
- Renamed
date_created
topost_date
,date_created_gmt
topost_date_gmt
. - Renamed
wp_post_format
topost_format
and added some error handling. - Totally reworked taxonomy term handling. See notes below.
- Updated PHPDoc as appropriate, though it still needs more work before commit.
Regarding taxonomy terms, I decided to try a different approach than before. It became clear after some IRC discussion that attempting to disambiguate IDs versus names in a single array was a fool's errand, so I changed it to be two separate fields in content_struct
:
terms
: should contain list of IDs of existing terms.terms_names
: should contain list of term names. If existing, we'll look up the ID; if not existing, we'll create a new term for it. If the taxonomy is hierarchical, then it checks whether there are multiple terms with that name, and throws an error in that case (client should use an ID).
Most existing clients should be able to just switch to terms_names
, while new clients should be encouraged to use terms
. But you can use them both together and it'll do the right thing. I've tested this fairly thoroughly and confirmed the described behavior.
Remaining questions:
- Should we silently ignore invalid
comment_status
,ping_status
andpost_status
values? Or should we return errors? - What kind of filters and hooks should we put at the end of the method? Since #17109 is blessed, we should probably use that style?
#28
@
13 years ago
+1 for the new taxonomy handling.
One side note we should include this feature as well #15098 when that patch got committed
#29
@
13 years ago
We should include it now. That patch doesn't involved in the way we do it. We should use set_post_thumbnail( $post, $thumbnail_id ) and delete_post_thumbnail( $post ) for post thumbnail. The patch at #15098 doesn't use that.
I'm not sure if we should do some extra check on custom_fields. To remove the value _thumbnail_id from there. I do think the client is responsible with it.
#31
@
13 years ago
Using wp.newPost.10.patch
as a base, I created a complementary wp_editPost
implementation. Because the two methods share the vast majority of their logic, I factored it out into a _wp_insertPost
method that the other two call.
If this approach is agreeable, then #18430 can be marked as a dupe.
#32
@
13 years ago
At markoheijnen's suggestion, I added an unset( $content_struct['ID'] )
to wp_newPost.
#34
@
13 years ago
I made minor changes to the patch before committing:
- Added some {} bracing to make some single line nested ifs more readable
- Fixed some spelling / wording
- Added the odd extra comment
- Replaced the anonymous function used for array filtering with a real one - they require PHP 5.3 and we support 5.2.4 and newer.
#35
@
13 years ago
Some comments after looking at [19848]:
Why the different error messages for different post_status? If the user can't publish_posts
then the error is "Sorry, you are not allowed to publish posts in this post type".
I think specifying that user is not allowed to do private or password protected posts it's just confusing if there's no specific capability for it.
I don't see the point of this (in trunk/wp-includes/class-wp-xmlrpc-server.php@19848#L645
if ( ! empty( $post_data['post_password'] ) && ! current_user_can( $post_type->cap->publish_posts ) ) return new IXR_Error( 401, __( 'Sorry, you are not allowed to create password protected posts in this post type' ) );
Also, the current implementation looks like it has the same bug as #19733. Being new methods you don't need to keep compatibility on behavior, but mixing mysql2date and IXR_Date on zero dates results on invalid dates and xml parsers going nuts. I believe #16985 applies too
Last, do_action( 'xmlrpc_call', 'wp.getPosts' );
is called twice on wp.getPosts
#37
follow-up:
↓ 43
@
13 years ago
You do have a point with that. Looking at the switch of post_status it even can be simplified little bit more.
The password check is needed to have the same implementation as the backend. There you can only fill in a password when you have the right to publish a post.
#38
follow-up:
↓ 40
@
13 years ago
The patch fixes some security flaws.
For new posts
current_user_can( $cap );
For existing posts
current_user_can( $cap, $post_id );
#40
in reply to:
↑ 38
@
13 years ago
Replying to nprasath002:
The patch fixes some security flaws.
For new posts
current_user_can( $cap );For existing posts
current_user_can( $cap, $post_id );
Good Catch, rather than splitting out the code and duplicating it I think it might be cleaner to alter _wp_insert_post and just have post_id splits around the calls to current_user_can with one call with and one without depending on whether or not it is provided - otherwise we have to make sure to keep two copies of the code in sync in future.
#41
follow-up:
↓ 42
@
13 years ago
Do we have to? if we send a post_id with new post that is has the value: null. Shouldn't that work as well?
#42
in reply to:
↑ 41
@
13 years ago
Replying to markoheijnen:
Do we have to? if we send a post_id with new post that is has the value: null. Shouldn't that work as well?
New posts must check plurals like
current_user_can( 'publish_posts' );
existing posts must check singular
current_user_can( 'publish_post', $post_id );
considering proper error messages like 'you cannot create posts' and 'you cannot edit this post'
i think we must have two seperate code segments.
One think we can do is pass a variable $update to _wp_insert_post
and have everything inside _wp_insert_post.
$update might help in future problems also
#43
in reply to:
↑ 37
@
13 years ago
Replying to markoheijnen:
The password check is needed to have the same implementation as the backend. There you can only fill in a password when you have the right to publish a post.
If I'm not missing anything, given the previous switch, the only thing that check prevents is adding a password to a draft if you can't publish posts. If you go to the backend and save a password protected draft, the password is just removed. If it's published instead of a draft and don't have the proper permissions, the previous switch would take care of that
#45
follow-up:
↓ 46
@
13 years ago
We should also unset( $content_struct['filter'] )
as well, for security reasons.
#46
in reply to:
↑ 45
@
13 years ago
Replying to nacin:
We should also
unset( $content_struct['filter'] )
as well, for security reasons.
Doesn't wp_insert_post
do this within its first few lines?
#47
@
13 years ago
A small fix for sticking posts.
get_post() don't have the field 'sticky'.
If a client sends a request to edit a sticky post which does not have the field 'sticky'
_wp_insert_post will unstick the post.
#50
@
13 years ago
The new fix allows (both mw_editpost and _wp_insert_post) contributors to publish posts.
Also mw_editpost cannot be used for pages in the new fix.
#52
@
13 years ago
Sentence in line 732 is missing a dot at the end. Because of this, translators have two strings to translate instead of one (line 732 and 755).
#54
follow-up:
↓ 57
@
13 years ago
Created some basic unit tests, mostly verifying cap check logic.
This is the first time I've written WordPress unit tests, so before I get too far I wanted to post this and get feedback on style/structure from westi (or other core devs).
#55
follow-up:
↓ 58
@
13 years ago
- Cc screenwavemedia added
Could featured image functionality be added to this? _wp_thumbnail_id is a protected meta field. There's no way the existing metaWeblog.newPost can assign the featured image. See #15098.
#57
in reply to:
↑ 54
@
13 years ago
Replying to maxcutler:
Created some basic unit tests, mostly verifying cap check logic.
This is the first time I've written WordPress unit tests, so before I get too far I wanted to post this and get feedback on style/structure from westi (or other core devs).
#58
in reply to:
↑ 55
@
13 years ago
Replying to screenwavemedia:
Could featured image functionality be added to this? _wp_thumbnail_id is a protected meta field. There's no way the existing metaWeblog.newPost can assign the featured image. See #15098.
Our current plan is to add support for setting Thumbnails yes but it was not in scope for the first version of these functions.
#60
@
13 years ago
#61
follow-up:
↓ 62
@
13 years ago
Changed the code to use set_post_thumbnail() and delete_post_thumbnail().
Couldn't change the get method because of not available when the theme doesn't have post thumbnail support.
Is it possible to move get_post_thumbnail_id() from /wp-includes/post-thumbnail-template.php to /wp-includes/post.php to make this happen?
#62
in reply to:
↑ 61
@
13 years ago
Replying to markoheijnen:
Changed the code to use set_post_thumbnail() and delete_post_thumbnail().
I'm a -0 on this, and had considered it when working on my patch. Using those methods introduces extra work (more get_post calls) that isn't required and introduces a layer of indirection that isn't particularly helpful.
Technically "_thumbnail_id" might be an internal/subject-to-change detail, but it's used in many places in core, and I see no reason why the XML-RPC server implementation can't use it too. If core were to ever change the implementation, they'd have to grep the codebase and they'd find its usage here and change it appropriately.
#63
follow-up:
↓ 64
@
13 years ago
Basically the following code is enough and the more get_post calls isn't there anymore.
if ( isset ( $post_data['featured_image'] ) ) { // empty value deletes, non-empty value adds/updates if ( empty( $post_data['featured_image'] ) ) { delete_post_thumbnail( $post_ID ); } else { if ( set_post_thumbnail( $post_ID, $post_data['featured_image'] ) === false ) return new IXR_Error( 404, __( "Invalid attachment ID." ) ); } unset( $content_struct['featured_image'] ); }
#64
in reply to:
↑ 63
@
13 years ago
Replying to markoheijnen:
Basically the following code is enough and the more get_post calls isn't there anymore.
I meant the work that happens inside delete_post_thumbnail
and set_post_thumbnail
, which each perform a get_post
and some validation that is redundant for our purposes (since we've already done the requisite checks).
I don't see what's gained by calling these wrapper functions instead of calling the post meta functions directly.
#65
@
13 years ago
In my eyes better code. When there are WordPress methods what does the work for you, you should use it.
When removing the checks what I did most of the checks happen in set_post_thumbnail().
The only issue know is that it does a get_post on the post ID. I'm curious what the opinion is from westi or any other core contributor.
#66
follow-up:
↓ 67
@
13 years ago
I don't see what's gained by calling these wrapper functions instead of calling the post meta functions directly.
If there's an API for something, use it, don't reinvent the wheel.
get_post() calls are inexpensive (and they're cached) in the global scheme of things, Validation happens for a reason, and finally, If core changes anything, its best to only do it in one location. If there's anything in core that doesn't use the wrappers, feel free to open a ticket to update it.. In many cases the wrappers simply didn't exist at the time.
#67
in reply to:
↑ 66
@
13 years ago
Replying to dd32:
If there's an API for something, use it, don't reinvent the wheel.
That's fair, though the proposed change doesn't remove all references to _thumbnail_id (see markoheijnen's comment above about get_post_thumbnail_id
). I personally would prefer consistency (all wrappers or all *_post_meta), but I'll defer to the core devs.
#68
@
13 years ago
It's how you look at the problem. You also can say we should check if the theme supports post thumbnail.
And only allowed get/update/remove when the theme does support it.
#69
@
13 years ago
Actually, we should be checking whether the post type supports thumbnails. Technically both post type and theme must support them for them to actually "work", but there's nothing wrong with getting/setting thumbnails even if the current theme doesn't support them (because the site could change to a theme that does at a later point in time).
#70
follow-up:
↓ 72
@
13 years ago
You also can say we should check if the theme supports post thumbnail. And only allowed get/update/remove when the theme does support it.
This. or at least, that's what I expect to be done. If a site doesn't support something, you don't expose that functionality.
There's a reason the Admin UI only exists when the theme/post type supports the functionality..
When the site changes to a theme that supports it, then you can expose it.. allowing something not supported by the theme to be set just seems poor UX to me.
#71
@
13 years ago
Added a full patch for all the methods that should change get effected by the new date en featured image options. Will test it tomorrow.
#72
in reply to:
↑ 70
@
13 years ago
Replying to dd32:
This. or at least, that's what I expect to be done. If a site doesn't support something, you don't expose that functionality.
I agree with the logic here. But in that case, please add some way to query if the functionality is supported so apps can adapt the UI. There's no point on showing a featured image selector in an app if WordPress is going to ignore it
#73
@
13 years ago
Ticket #18436 helps partly with it. You see there all the post type with support.
There should be a value in initialise_blog_option_info() that shows all the post type which has thumbnail support and are supported by the theme. The value can be get by calling: wp_getOptions.
#74
follow-up:
↓ 76
@
13 years ago
It has already been committed, but can I suggest we remove the 'tags' and 'categories' fields from _prepare_post
? We do not accept these fields in wp_newPost
/wp_editPost
, and there's no real reason to include them for back-compat in a brand-new method. If a client wants those old-style values, they can use the old-style methods.
#76
in reply to:
↑ 74
@
13 years ago
Replying to maxcutler:
It has already been committed, but can I suggest we remove the 'tags' and 'categories' fields from
_prepare_post
? We do not accept these fields inwp_newPost
/wp_editPost
, and there's no real reason to include them for back-compat in a brand-new method. If a client wants those old-style values, they can use the old-style methods.
I agree we should remove this before they become baked into the API.
wp.getPost() should return what you would be able to send to wp.editPost() so you can fetch/update/edit easily from a client APP.
I'll remove these.
#78
@
13 years ago
I've looked at: dates_and_featured_image4.patch
And I have the following feedback:
- I don't think the XMLRPC api should change what it returns based on
current_theme_supports( 'post-thumbnails' )
we should return all the data and expose the current_theme_supports info so the App can make a display decision. - We shouldn't block an app from setting a features image just because the current theme won't display it.
Could you update the patch based on these and the fact that the date stuff is now committed?
Closed #16476 as a duplicate.