Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 4 years ago

#21397 closed task (blessed) (fixed)

Expose post revisions on the XML-RPC Endpoint

Reported by: daniloercoli's profile daniloercoli Owned by: nacin's profile nacin
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)

21397.diff (5.6 KB) - added by brandondove 12 years ago.
patch-core-21397-1.diff (3.3 KB) - added by koke 12 years ago.
patch-core-21397-2.diff (3.8 KB) - added by koke 12 years ago.
patch-core-21397-2-tests.diff (1.3 KB) - added by koke 12 years ago.
patch-core-21397-3.diff (4.2 KB) - added by koke 12 years ago.
patch-core-21397-3-tests.diff (5.0 KB) - added by koke 12 years ago.
patch-core-21397-4-tests.diff (6.5 KB) - added by koke 12 years ago.
patch-core-21397-4.diff (6.5 KB) - added by koke 12 years ago.
21397.5.diff (5.4 KB) - added by JustinSainton 11 years ago.
21397.6.diff (4.7 KB) - added by markoheijnen 11 years ago.
Used feedback of Nacin. Only need to check ->getTimestamp()
21397.7.diff (4.7 KB) - added by markoheijnen 11 years ago.
21397.6.diff was a little bit broken and fixes it
21397.7.unittests.diff (5.0 KB) - added by markoheijnen 11 years ago.

Download all attachments as: .zip

Change History (58)

#1 @markoheijnen
12 years ago

  • Milestone changed from Awaiting Review to 3.5

This would be really awesome to have in 3.5

#2 @markoheijnen
12 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 @maxcutler
12 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 return current_revision and revision_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).

#4 @maxcutler
12 years ago

  • Cc maxcutler added

#5 @markoheijnen
12 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.

#6 @brandondove
12 years ago

  • Cc brandon@… added

#7 @BenjaminRMueller
12 years ago

  • Cc BenjaminRMueller added

#8 @koke
12 years ago

  • Cc jbernal@… added

@brandondove
12 years ago

#9 @brandondove
12 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: @markoheijnen
12 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 @brandondove
12 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: @markoheijnen
12 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 @brandondove
12 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.

#14 @redsweater
12 years ago

  • Cc jalkut@… added

#15 follow-up: @markoheijnen
12 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 @maxcutler
12 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 @markoheijnen
12 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 @koke
12 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

#19 @isaackeyet
12 years ago

  • Cc isaackeyet added

#20 @koke
12 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

#21 @mrroundhill
12 years ago

  • Cc dan@… added

#22 @redsweater
12 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 @koke
12 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 of wp_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: @maxcutler
12 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 existing wp.getPost.
  • Drop wp.newRevision and just use existing wp.newPost or wp.editPost with draft or autosave as post_status.
  • wp.getRevisions should use _prepare_post with default fields of ID, post_date, and post_date_gmt. Method should take a $fields argument so that caller can customize returned fields (to avoid additional calls to wp.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 in wp.getOptions as read-only revisions.
  • Add autosave to allowed list of post statuses in _insert_post (add additional case to the switch ( $post_data['post_status'] ) statement).

#25 @brandondove
12 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: @koke
12 years ago

Replying to maxcutler:

  • Drop wp.newRevision and just use existing wp.newPost or wp.editPost with draft or autosave as post_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 the switch ( $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 @maxcutler
12 years ago

Replying to koke:

draft would just change the status, but I think autosave 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 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

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 @koke
12 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 @koke
12 years ago

What we're doing (from yesterday's IRC chat):

  • wp.getRevisions
  • wp.restoreRevision
  • Add a only_if_no_new_revision option to wp.editPost
  • Add a return_preview_url option to wp.editPost

#30 @koke
12 years ago

Added patch with working wp.getRevisions and wp.restoreRevision. More to come

#31 @koke
12 years ago

Added a second patch including only_if_no_new_revision and unit tests for it

#32 follow-up: @maxcutler
12 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 @koke
12 years ago

Replying to maxcutler:

I believe you can drop $fields from wp.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 @koke
12 years ago

New set of patches drops the $fields argument on wp.restoreRevision, adds cap checks and tests for the new methods

#35 @koke
12 years ago

  • Keywords has-patch added

Final set of patches add return_preview_url option to wp.editPost

#36 @maxcutler
12 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 @nacin
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.

#38 @JustinSainton
11 years ago

  • Keywords needs-refresh removed

#39 @nacin
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.

#40 @nacin
11 years ago

  • Type changed from enhancement to task (blessed)

#41 @markoheijnen
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

@markoheijnen
11 years ago

Used feedback of Nacin. Only need to check ->getTimestamp()

@markoheijnen
11 years ago

21397.6.diff was a little bit broken and fixes it

#42 @markoheijnen
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

#43 @nacin
11 years ago

In [22034]:

XML-RPC: Add an if_not_modified_since argument to wp.editPost.

Accepts a GMT date, which is used to compare to the current post_modified_gmt
value for the post being edited. If the post has since been edited (as in, too
old of a date was passed), the edit is rejected as overwriting a newer version.

It is rejected with a HTTP 409 Conflict status code. (Fancy.)

props koke, markoheinjen.
Tests: [UT1049]

see #21397.

#44 @nacin
11 years ago

[22034], I realized, is missing inline documentation.

#45 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [22037]:

XML-RPC: Introduce wp.getRevisions and wp.restoreRevision.

props brandondove, koke, markoheijnen, JustinSainton, maxcutler.

fixes #21397.

This ticket was mentioned in Slack in #core-editor by talldanwp. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.