WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#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)

wp.newPost.patch (21.7 KB) - added by nprasath002 3 years ago.
wp.newPost.2.patch (5.4 KB) - added by markoheijnen 2 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
wp.newPost.3.patch (5.1 KB) - added by markoheijnen 2 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
wp.newPost.4.patch (7.6 KB) - added by markoheijnen 2 years ago.
Updated 4th patch
wp.newPost.5.patch (7.6 KB) - added by markoheijnen 2 years ago.
Inserted the feedback of maxcutler. Removed get_default_post_to_edit
wp.newPost.6.patch (7.9 KB) - added by markoheijnen 2 years ago.
Insert get_default_post_to_edit back again and made wp_insert_post the last step of the method
wp.newPost.7.patch (8.2 KB) - added by markoheijnen 2 years ago.
Fix hierarchical taxonomy issue
wp.newPost.8.patch (10.1 KB) - added by maxcutler 2 years ago.
wp.newPost.9.patch (9.7 KB) - added by maxcutler 2 years ago.
Simplified $post_datatax_input? construction.
wp.newPost.10.patch (10.5 KB) - added by maxcutler 2 years ago.
Added $post_data filter and improved PHPDoc.
new.edit.combined.patch (12.5 KB) - added by maxcutler 2 years ago.
wp.newPost and wp.editPost sharing logic.
new.edit.combined.2.patch (12.5 KB) - added by maxcutler 2 years ago.
Disallowed ID to be passed in wp.newPost.
cycle1.patch (22.0 KB) - added by maxcutler 2 years ago.
Combined patch for #18429, #18430, #18431, #18432, and #18433
wp.newPost.11.patch (5.0 KB) - added by nprasath002 2 years ago.
wp.newPost2.patch (4.9 KB) - added by nprasath002 2 years ago.
wp.newPost3.patch (3.2 KB) - added by nprasath002 2 years ago.
stickpostfix.patch (909 bytes) - added by nprasath002 2 years ago.
missing_dot.patch (796 bytes) - added by kenan3008 2 years ago.
post_unittests.patch (9.4 KB) - added by maxcutler 2 years ago.
Some basic unit tests.
convert_date.patch (2.7 KB) - added by maxcutler 2 years ago.
dates_and_featured_image.patch (4.5 KB) - added by maxcutler 2 years ago.
dates_and_featured_image2.patch (4.6 KB) - added by markoheijnen 2 years ago.
dates_and_featured_image3.patch (18.0 KB) - added by markoheijnen 2 years ago.
dates_and_featured_image4.patch (18.4 KB) - added by markoheijnen 2 years ago.
featured_image.patch (4.0 KB) - added by markoheijnen 2 years ago.
18429.diff (20.5 KB) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (110)

nprasath0023 years ago

comment:1 SergeyBiryukov2 years ago

Closed #16476 as a duplicate.

comment:2 raamdev2 years ago

  • Cc raam@… added

comment:3 westi2 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)

Marking this as a Blessed Task for 3.4 - This is part of the first iteration of XML-RPC new features we will be working on.

TODO: Design API, Implement API, Align with #18432/#18433 for field names etc.

comment:4 follow-up: markoheijnen2 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?

markoheijnen2 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

comment:5 in reply to: ↑ 4 nprasath0022 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

comment:6 nprasath0022 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

comment:7 markoheijnen2 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.

comment:8 markoheijnen2 years ago

Should look at the end of the 2 week cycle to ticket #17109 to maybe add the needed code.

comment:9 westi2 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.

comment:10 markoheijnen2 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.

markoheijnen2 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

comment:11 markoheijnen2 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

comment:12 nprasath0022 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

comment:13 nprasath0022 years ago

Last edited 2 years ago by nprasath002 (previous) (diff)

comment:14 nprasath0022 years ago

Why the option for providing an author_id was removed?? current mw methods provides that.

comment:15 nprasath0022 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

comment:16 nprasath0022 years ago

Why comment, ping statuses are not validated??

comment:17 nprasath0022 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.

comment:18 markoheijnen2 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.

comment:19 markoheijnen2 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

comment:20 nprasath0022 years ago

To give author id we must validate cap edit_others_posts.

comment/ping status validation has $content_struct instead of $post_data

markoheijnen2 years ago

Updated 4th patch

comment:21 markoheijnen2 years ago

Thanks for the feedback. Update the patch

comment:22 maxcutler2 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 the if ( isset(...)). Because even in the else 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 that if has no else.
  • There's an undefined variable $terms_needed used in the term handling code.
  • Can you separate the complex if statements for comment_status and ping_status? Right now it's hard to read, it'd probably be better with an if ( isset(...)) and a nested if 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 of blog_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.

Last edited 2 years ago by maxcutler (previous) (diff)

comment:23 markoheijnen2 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.

markoheijnen2 years ago

Inserted the feedback of maxcutler. Removed get_default_post_to_edit

comment:24 markoheijnen2 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

comment:25 nacin2 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.

markoheijnen2 years ago

Insert get_default_post_to_edit back again and made wp_insert_post the last step of the method

comment:26 markoheijnen2 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

markoheijnen2 years ago

Fix hierarchical taxonomy issue

maxcutler2 years ago

comment:27 maxcutler2 years ago

Updated the patch with several fixes:

  • Renamed date_created to post_date, date_created_gmt to post_date_gmt.
  • Renamed wp_post_format to post_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:

  1. terms: should contain list of IDs of existing terms.
  2. 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 and post_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?

maxcutler2 years ago

Simplified $post_datatax_input? construction.

comment:28 nprasath0022 years ago

+1 for the new taxonomy handling.

One side note we should include this feature as well #15098 when that patch got committed

comment:29 markoheijnen2 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.

comment:30 maxcutler2 years ago

I'm hesitant to include the #15098 stuff in this ticket for now. There are a number of related tickets, including #13917 and #18683, and someone from core team (westi? josephscott?) needs to make a determination about what the preferred design change should be.

maxcutler2 years ago

Added $post_data filter and improved PHPDoc.

maxcutler2 years ago

wp.newPost and wp.editPost sharing logic.

comment:31 maxcutler2 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.

maxcutler2 years ago

Disallowed ID to be passed in wp.newPost.

comment:32 maxcutler2 years ago

At markoheijnen's suggestion, I added an unset( $content_struct['ID'] ) to wp_newPost.

maxcutler2 years ago

Combined patch for #18429, #18430, #18431, #18432, and #18433

comment:33 westi2 years ago

In [19848]:

XMLRPC: Introduce new create,read,update and delete XMLRPC apis for Posts, Pages and all Custom Post Types.
Introduces: wp.newPost, wp.editPost, wp.deletePost, wp.getPost, and wp.getPosts
See #18429, #18430, #18431, #18432, and #18433 props maxcutler and markoheijnen.

comment:34 westi2 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.

comment:35 koke2 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

comment:36 koke2 years ago

  • Cc jbernal@… added

comment:37 follow-up: markoheijnen2 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.

nprasath0022 years ago

comment:38 follow-up: nprasath0022 years ago

The patch fixes some security flaws.

For new posts

current_user_can( $cap );

For existing posts

current_user_can( $cap, $post_id );

nprasath0022 years ago

comment:39 nprasath0022 years ago

Fixed the patch something went wrong in my IDE :(.
Check the latest patch

comment:40 in reply to: ↑ 38 westi2 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.

comment:41 follow-up: markoheijnen2 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?

Last edited 2 years ago by markoheijnen (previous) (diff)

comment:42 in reply to: ↑ 41 nprasath0022 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

comment:43 in reply to: ↑ 37 koke2 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

nprasath0022 years ago

comment:44 nprasath0022 years ago

Altered _wp_insert_post to fix cap checks

comment:45 follow-up: nacin2 years ago

We should also unset( $content_struct['filter'] ) as well, for security reasons.

nprasath0022 years ago

comment:46 in reply to: ↑ 45 maxcutler2 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?

comment:47 nprasath0022 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.

comment:48 ryan2 years ago

In [19867]:

Simplify cap checking. Props nprasath002. see #18429

comment:49 duck_2 years ago

In [19868]:

Mark wp_editPost() documentation as docblock. See #18429.

comment:50 nprasath0022 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.

Last edited 2 years ago by nprasath002 (previous) (diff)

comment:51 ryan2 years ago

In [19873]:

s/_wp_insertPost/_insert_post/. Helper funcs should follow core rather than xmlrpc style. see #18429

kenan30082 years ago

comment:52 kenan30082 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).

comment:53 nacin2 years ago

In [19879]:

Combine two strings. props kenan3008, see #18429.

comment:54 follow-up: maxcutler2 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).

maxcutler2 years ago

Some basic unit tests.

comment:55 follow-up: screenwavemedia2 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 via the existing metaWeblog.newPost can assign the featured image. See #15098.

Version 0, edited 2 years ago by screenwavemedia (next)

comment:56 ryan2 years ago

In [19914]:

Disallow changing the post type in mw_editPost(). see #18429

comment:57 in reply to: ↑ 54 westi2 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).

These look great - Committed in [UT527] and [UT528]

comment:58 in reply to: ↑ 55 westi2 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.

comment:59 maxcutler2 years ago

While writing tests ([UT554]) for wp.getPost, I encountered the date issues from #19733. I created a new helper method to fix this for the new wp.* post methods.

This helper method could be used by the legacy methods without side effects, but I left that out of the patch for now pending feedback.

maxcutler2 years ago

comment:60 maxcutler2 years ago

Added support for a featured_image field which sets the _thumbnail_id meta field in a safe way. See tests in [UT555].

It differs from the patch in #15098 in that it only returns the attachment ID. If a client needs more information (such as URL or caption), it should use wp.getMediaItem.

Last edited 2 years ago by maxcutler (previous) (diff)

comment:61 follow-up: markoheijnen2 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?

comment:62 in reply to: ↑ 61 maxcutler2 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.

comment:63 follow-up: markoheijnen2 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'] );
		}

comment:64 in reply to: ↑ 63 maxcutler2 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.

comment:65 markoheijnen2 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.

comment:66 follow-up: dd322 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.

comment:67 in reply to: ↑ 66 maxcutler2 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.

comment:68 markoheijnen2 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.

comment:69 maxcutler2 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).

comment:70 follow-up: dd322 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.

comment:71 markoheijnen2 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 further tomorrow.

Last edited 2 years ago by markoheijnen (previous) (diff)

comment:72 in reply to: ↑ 70 koke2 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

comment:73 markoheijnen2 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.

comment:74 follow-up: maxcutler2 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.

comment:75 westi2 years ago

In [20153]:

XMLRPC: Intoduce a date generation helper method to improve the dates returned over XMLRPC when we have a 0 date stored for drafts.

This improves the ability of clients to work with the new wp Post APIs. See #18429 and #19733 props maxcutler.

comment:76 in reply to: ↑ 74 westi2 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 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.

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.

comment:77 westi2 years ago

In [20154]:

XMLRPC: Remove the "Backward Compatibility" code from the new api as we don't need to support this older format and it keeps the new api cleaner.

The new Posts API is designed so that you can fetch a post with get, update the data structure returned and submit that with edit and so we need to ensure to only include in get data in a single form - tags and categories are returned as terms.
See #18429

comment:78 westi2 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?

comment:79 markoheijnen2 years ago

Attachment added for featured images.
Patch for dates is added at ticket #19733

comment:80 nprasath0022 years ago

Related #20212. Could be a useful feature

comment:81 westi2 years ago

In [20270]:

XMLRPC: Add support for Feature Images to the new wp.xxxPost apis. See #18429 props maxcutler and markoheijen.

comment:82 maxcutler2 years ago

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

New bugs or enhancements on new tickets.

comment:83 westi2 years ago

In [20321]:

XMLRPC: Fix the featured image support in mw_newPost to use the correct variable names. See #18429 and [UT592].

comment:84 westi2 years ago

In [20322]:

XMLRPC: Fix bugs in mw_editPost hilighted by tests in [UT593]

  • Feature image code did not actually set the feature image - See #18429
  • This api call could trash post data - Fixes #20321.

nacin2 years ago

Note: See TracTickets for help on using tickets.