#21397 closed task (blessed) (fixed)
Expose post revisions on the XML-RPC Endpoint
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 3.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | XML-RPC | Keywords: | mobile has-patch |
Focuses: | Cc: |
Description
We'd like to have methods to load/restore/save post revisions via XML-RPC.
- Get the list of revisions for a single post.
- Get a single revision.
- Restore a revision.
- Save a post revision.
Attachments (12)
Change History (58)
#2
@
11 years ago
At this moment most of the stuff can be done with the current methods. We should create new methods that loads the current methods.
We only need to write something for restoring a revision and check if the link is correct so it can be used to review a revision.
#3
@
11 years ago
Does it make more sense to create a bunch of new methods or to add this onto the existing methods? The latter could look like:
wp.getPost
&wp.getPosts
returncurrent_revision
andrevision_count
values which tell you how many revisions are available.wp.getPost
takes a new optional parameter to specify which revision to return (defaults to the latest).- Reverting to an old revision could be done manually or by a new method (e.g.,
wp.revertPostRevision
.
Either way, we should add an option to wp.getOptions
that tells you whether or not revisions are enabled for the blog (WP_POST_REVISIONS
).
#5
@
11 years ago
That was my first though (but in another way) also but after rethinking it and a talk on IRC with daniloercoli I do believe this should have their own methods.
Revisions aren't obvious a post type and their implementation can change and we get methods like:
- wp.getRevision ( blog_id, username, password, revision_id )
- wp.getRevisions ( blog_id, username, password, post_id, filter )
- wp.newRevision ( blog_id, username, password, post_id, constent_struct )
- wp.restoreRevision ( blog_id, username, password, post_id, revision_id )
The addon for wp.getPost & wp.getPosts is helpful but revision_count should be a array with revision ids.
#9
@
11 years ago
Posted first pass at adding the requested methods.
- wp.getRevision returns the content struct of the specified revision
- wp.getRevisions returns an array of content structs for the specified post
- wp.newRevision saves a new revision (via wp_save_post_revision attached to the pre_post_update hook) and updates the post with the specified content struct. This piggybacks off of editPost.
- wp.restoreRevision rolls back a post to a specified revision.
Please confirm that this is the expected behavior.
#10
follow-up:
↓ 11
@
11 years ago
Looked to it and I believe wp.newRevision should use wp.newPost instead of wp.editPost.
Also wp.restoreRevision isn't really working and also the arguments aren't right
#11
in reply to:
↑ 10
@
11 years ago
Replying to markoheijnen:
Looked to it and I believe wp.newRevision should use wp.newPost instead of wp.editPost.
What is the expected behavior of wp.newRevision? Is it to update an existing post or create an entirely new post?
#12
follow-up:
↓ 13
@
11 years ago
You want to create a new post with the post type revision. Looking further to the code wp.newRevision wil not work correctly.
Also I'm wondering if the add_action there is valid since the change is that it is already loaded
#13
in reply to:
↑ 12
@
11 years ago
Replying to markoheijnen:
You want to create a new post with the post type revision. Looking further to the code wp.newRevision wil not work correctly.
What's the point of creating a new revision by itself? Is there functionality in core that supports doing that as a one off action?
Also I'm wondering if the add_action there is valid since the change is that it is already loaded
I'm using the action here because that's how core handles creating the revision when you edit a post in the post editor.
#15
follow-up:
↓ 16
@
11 years ago
About the functionality. We don't want to save a post but we want to create a new revision for a specific post. That can be put live when needed. It's for loading the content into the site without needing it to be saved. A little bit what core does now when you writing a post for a long time and it saves when you are writing.
I will dive into the code later but if I'm correct the hook is already loaded. With your code it's probably getting loaded twice.
#16
in reply to:
↑ 15
@
11 years ago
Replying to markoheijnen:
About the functionality. We don't want to save a post but we want to create a new revision for a specific post. That can be put live when needed. It's for loading the content into the site without needing it to be saved. A little bit what core does now when you writing a post for a long time and it saves when you are writing.
Are you referring to the autosave behavior? I'm not sure why that warrants an entire new method (which must be maintained in perpetuity) instead of just adding a flag to wp.editPost
.
These new methods look awfully similar to the existing post methods, and I don't yet see a clear advantage to increasing the maintenance and documentation burden when the same could be accomplished with simple filters/flags on existing methods.
#17
@
11 years ago
I do mean the autosave behavior. For creating a revision it probably make sense to use wp.editPost for doing that.
For getting revisions I do believe it is better to create new methods for that because when using a API it should be obvious how to use it. It's little bit like the media methods.
#18
@
11 years ago
Some use cases for the revisions API:
- Autosaving posts (as mentioned): user types a long post and the app autosaves every X seconds, or when the user switches to another app
- User is editing an existing post and wants to preview the result, although that's being taken care of in #21398
- User edits post offline in the app, then some other user (or same) changes the post in wp-admin. When the app syncs, there's conflict resolution to be handled
#20
@
11 years ago
What we really need fro every use case I can think of right now:
Case 1: create new post with gallery
Create new post/revision as auto-draft. We need a post ID to attach images to, it can be via wp.newPost accepting 'auto-draft' as status or wp.newRevision
Case 2: Preview of edits to an existing post
Save a new revision without updating the regular post. Both a flag to wp.editPost or a new wp.newRevision would do the job here
Case 3: Preview of new post
Save as draft, then preview. Nothing needed here
Case 4: Conflict management
a) Before save, request a list of revisions, check if there's a new one (not very efficient)
b) Add a flag (or make it default behavior) to wp.editPost to return an error if there's a revision newer than post_modified_gmt
Case 5: Autosave
Save a revision, same as case 2 for existing posts. If post is auto-draft, a regular save would work too
#22
@
11 years ago
for Case 4: Conflict management, it would make a lot of sense to do something akin to ETag support in HTTP (and what AtomPub uses to achieve this same thing). Maybe in WordPress the "revision" is sufficient to achieve this. Anyway, it would be nice if the API support a very low-bandwidth way for the client to ask "is there a newer version available than xxx"?
#23
@
11 years ago
Looking at the patch:
- wp.getRevision: I think wp.getPost would work just the same, but this adds an extra check to make sure we're getting back a revision and not a regular post
- wp.getRevisions: looks good to me
- wp.newRevision: not sure what the goal was here, since editPost will already create a new revision. IMO, the useful thing would be to create an "autosave", just like wp-admin does. This could be wp.newAutosavePost or just some sort of "autosave" flag for wp.editPost
- wp.restoreRevision: like Marko said, it doesn't work. It needs to call
wp_restore_post_revision
instead ofwp_get_post_revision
Also, most of the documentation is wrong in the patch. I can work on it once we're clear on how we want to do newRevision/autosaves
#24
follow-up:
↓ 26
@
11 years ago
- Cc max@… added; maxcutler removed
I agree with most of what koke said. Here's what I would do:
- Drop
wp.getRevision
and just use existingwp.getPost
. - Drop
wp.newRevision
and just use existingwp.newPost
orwp.editPost
withdraft
orautosave
aspost_status
. wp.getRevisions
should use_prepare_post
with default fields ofID
,post_date
, andpost_date_gmt
. Method should take a$fields
argument so that caller can customize returned fields (to avoid additional calls towp.getPost
if the caller knows it needs more than the ID and date).wp.restoreRevision
, fixed as stated by Marko and koke.- Expose
WP_POST_REVISIONS
inwp.getOptions
as read-onlyrevisions
. - Add
autosave
to allowed list of post statuses in_insert_post
(add additional case to theswitch ( $post_data['post_status'] )
statement).
#25
@
11 years ago
I'll get a new patch out today or tomorrow with the new direction. Documentation was cut/paste for the last two methods. Just wanted to get the patch out there for the sake of discussion.
#26
in reply to:
↑ 24
;
follow-up:
↓ 27
@
11 years ago
Replying to maxcutler:
- Drop
wp.newRevision
and just use existingwp.newPost
orwp.editPost
withdraft
orautosave
aspost_status
.
draft
would just change the status, but I think autosave
should work.
- Add
autosave
to allowed list of post statuses in_insert_post
(add additional case to theswitch ( $post_data['post_status'] )
statement).
Also I'd add auto-draft
or make autosave
create a new default post on wp.newPost
. Currently, mw.newPost
accepts auto-draft
as status but I haven't tried wp.newPost
#27
in reply to:
↑ 26
@
11 years ago
Replying to koke:
draft
would just change the status, but I thinkautosave
should work.
Which you should use depends on the current status. If it's already a draft, then you don't need to do anything. If it's been published, then you should use autosave
if you want to do a preview.
Also I'd add
auto-draft
or makeautosave
create a new default post onwp.newPost
. Currently,mw.newPost
acceptsauto-draft
as status but I haven't triedwp.newPost
I'm not sure I see the point of allowing auto-draft. An auto-draft is supposed to just be an empty post, which doesn't seem to apply to XML-RPC scenarios. Unless otherwise explicitly indicated, clients should almost always just let the status be draft
until the user publishes or deletes the post.
#28
@
11 years ago
The point of auto-draft
is to be able to get a post ID, so when you upload media before saving, it gets attached to the right post (so you can do [gallery]
)
#29
@
11 years ago
What we're doing (from yesterday's IRC chat):
wp.getRevisions
wp.restoreRevision
- Add a
only_if_no_new_revision
option towp.editPost
- Add a
return_preview_url
option towp.editPost
#32
follow-up:
↓ 33
@
11 years ago
I believe you can drop $fields
from wp.restoreRevision
. You should also add cap checks against 'edit_post` for both of these methods. Then once we have unit tests for the two new methods we should be ready for trunk.
#33
in reply to:
↑ 32
@
11 years ago
Replying to maxcutler:
I believe you can drop
$fields
fromwp.restoreRevision
wp_restore_post_revision() takes a fields
parameter, so that's why I kept it
You should also add cap checks against 'edit_post` for both of these methods. Then once we have unit tests for the two new methods we should be ready for trunk.
I'll add the checks and keep working on tests today
#34
@
11 years ago
New set of patches drops the $fields
argument on wp.restoreRevision
, adds cap checks and tests for the new methods
#35
@
11 years ago
- Keywords has-patch added
Final set of patches add return_preview_url
option to wp.editPost
#36
@
11 years ago
In wp.restoreRevision
, the cap check need to change from edit_posts
to edit_post, $post->ID
.
I'd also like to see the default xmlrpc_default_revision_fields
value be array( 'post_date', 'post_date_gmt' )
. Only a few fields are actually stored in revisions by default, and plugins/themes have the ability to change those fields. For the use cases discussed, the only fields really necessary are the ID and the date; and if the client needs more fields, then they can use the $fields
parameter to get them.
The patch includes preview-related code, is that intentional? The discussion on #21398 seemed inconclusive, so do we want to put this approach in core for 3.5? If so, need code review from nacin or others on those changes to post.php.
#37
@
11 years ago
- Keywords needs-refresh added
I would like to remove all preview-related code. only_if_no_new_revision is fine.
This patch needs a refresh. And soon, if the goal is to get this into 3.5.
#39
@
11 years ago
21397.5.diff looks like a good base. It does need some changes. Here is a code review.
Stylistically:
- The changes to wp_create_post_autosave() can now be reverted.
- Some indentation looks off, sporadically throughout the diff. Appears to be a tabs versus spaces thing.
Code-wise, for wp.restoreRevision:
- wp.restoreRevision's cap checks need to be refined. It should use wp_get_post_revision() rather than get_post(), and current_user_can() against the revision's parent, not against itself. (Even though map_meta_cap() accounts for this kind of mistake.)
- It should also probably check WP_POST_REVISIONS, post_type_supports($post->post_type, 'revisions'), and wp_is_post_autosave(). Normally, if either of the first two are false, then restoring this revision is only allowed if the revision is an autosave — but in practice thee should be blocked from XML-RPC.
- See the 'restore' branch at the top of wp-admin/revision.php for all of this in action.
For wp.getRevisions:
- wp.getRevisions's cap checks are off. They are based on the 'post' post type. But they should instead be based on the post type of the post ID passed. And it should be an edit_post + ID check rather than a generic edit_posts check.
- As above, WP_POST_REVISIONS, post type support, and autosave should be checked. If wp_is_post_autosave(), then it should be filtered out. (See wp_list_post_revisions() as well as where it is called.)
- Inside the foreach loop, you can just call current_user_can( 'edit_post', $postID? ). map_meta_cap() automatically translates 'edit_post' into the appropriate post type meta cap.
For only_if_no_new_revision:
- I am not sure how this works. It looks like getTimestamp() is called on the incoming value, but I don't see it converted to an IXR_Date anywhere.
- Should we do date-to-date comparisons, or check revisions directly? The date-to-date comparison (using post_modified_gmt) seems like the best option. In that case, it should probably be called only_if_modified_since, or something along those lines.
#41
@
11 years ago
I updated the patch with most of the feedback Nacin said. I need to look into the IXR_Date::getTimestamp() since I'm unsure it works. It also needs more unit tests
#42
@
11 years ago
I checked the code when I was back home and fixed some code issues and updated the unit tests to it. Probably need to add some more.
Weird thing is that $content_structonly_if_modified_since?->getTimestamp() does work. I do however was thinking to use $this->_convert_date() and check inside there is it is a IXR_Date object or not. Any thoughts about that
#45
@
11 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In [22037]:
This would be really awesome to have in 3.5