#43316 closed enhancement (fixed)
REST API: Support autosaves
Reported by: | kraftbj | Owned by: | rmccue |
---|---|---|---|
Milestone: | 5.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | REST API | Keywords: | has-patch has-unit-tests fixed-5.0 |
Focuses: | rest-api | Cc: |
Description
In looking at https://github.com/WordPress/gutenberg/pull/4218 (a PR that, in part, deals with an issue of a _lot_ of autosave revisions out of Gutenberg, e.g. https://github.com/WordPress/gutenberg/issues/3656 ), Gutenburg is looking at an admin-ajax.php approach using core JS APIs since the core REST API does not directly support autosaves.
Marking for WordPress 5.0 so it can be considered early in light of Gutenburg.
Attachments (28)
Change History (164)
#2
@
7 years ago
Why not have the autosave functionality in REST API's update_item()
. Autosaves are regular saves that update the post or create revisions, just follow a bit different logic. The "special" thing about them is that they either update the post without creating a revision, or create a revision without updating the post depending on post_status and current user.
43316.patch attempts to mimic how autosaves currently work through AJAX. It uses a flag is_autosave: true
.
The logic is:
- For drafts and auto-drafts:
- If the draft author is the same as the autosave author, update the draft post.
- If the draft author is different, create an autosave, or if it exists, update it.
- For other (published) posts:
- Create or update autosaves for the autosave author.
TODO: either add another conditional post_type_supports( $existing_post->post_type, 'revisions' )
or consider adding autosave
as another setting for post_type_supports.
#4
@
7 years ago
Nice! I'll give this a test. In addition to testing creating autosaves, I want to verify that it is possible to read an autosave through the revisions or post controller as well, otherwise this limits functionality to one way 'saving' of an autosave. It feels a bit unexpected that saving a post would create an autosave although on the flip side this makes it easier to use vs. a separate autosave controller.
#5
@
7 years ago
autosave-controller.diff for the record, the autosave rest api controller we were working on in gutenberg.
#6
@
7 years ago
@adamsilverstein did you have a chance to test 43316.patch?
Don't think we need separate controller for autosaves. They are standard revisions, the only difference is when saving they are either created or updated.
Planning to add the same flag is_autosave: true
to WP_REST_Revisions_Controller::get_item()
and WP_REST_Revisions_Controller::get_items()
for explicitly requesting autosaves.
Also see https://github.com/WordPress/gutenberg/pull/4218#issuecomment-367643571.
#7
@
7 years ago
@azaozz haven't tested yet, but planning on it soon!
I'm fine not adding another controller for autosaves and added it here only as a point of comparison to show what it would take to implement (I had already created it in a PR for Gutenberg).
#8
@
7 years ago
@azaozz I did some local testing and this seems to work well. I tested in the console after opening a page with wp-api loaded:
var post = new wp.api.models.Post(); post.set( 'title', 'autosave test' ); post.save(); post.set( 'status', 'publish' ); post.save(); post.set( 'title', 'updated autosave test' ); post.save( { is_autosave: true } );
I then logged in as a different user and created a new autosave as that user:
var post = new wp.api.models.Post(); post.set('id',[previous_id]); post.fetch(); post.save( { is_autosave: true } );
At this point, checking the database, the rows look as I expect them to:
Running tests here: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/344856533
#9
@
7 years ago
Replying to adamsilverstein:
Great! Thanks for testing.
In 43316.1.patch: when is_autosave: true
is set in the request WP_REST_Revisions_Controller::get_item()
and WP_REST_Revisions_Controller::get_items()
will return only autosaves (if any). Add couple of tests for that.
#10
@
7 years ago
Still TBD/TODO:
- I agree that passing a flag in the request (when saving or retrieving autosaves) may not be the best. As far as I see the only alternative is to set
post_type: 'autosave'
and then to "convert" it to the properpost_type = 'revision'
as soon as the request is received. As there is no'autosave'
post_type in WordPress, this may be confusing. - Add few more tests (perhaps) for edge cases.
#11
@
7 years ago
alternative is to set post_type: 'autosave' and then to "convert" it to the proper post_type = 'revision' as soon as the request is received
Interesting idea - this at least works within an existing field.
wondering if in the schema somewhere we have or can add a 'supports autosave' flag so clients know a post type supports autosaves (might be there already?).... also thinking we could add a helper to the wp.api client so you could do post.autosave();
and handle the status change internally.
This ticket was mentioned in Slack in #core-restapi by azaozz. View the logs.
7 years ago
#13
@
7 years ago
wondering if in the schema somewhere we have or can add a 'supports autosave' flag so clients know a post type supports autosaves
Was looking at that too. But it would mean we will have to add 'autosave' to post_type_supports()
too, right? There is post_type_supports 'revision' already, the thing is that even if CPTs don't support revisions, we still seem to do autosaves for them. Also autosaves (with or without creating a revision) is somewhat basic functionality, not sure if we need special flag to enable it.
thinking we could add a helper to the wp.api client
Yeah, think that would be great. Both for auto saving and retrieving autosaves.
#14
@
7 years ago
43316.diff adds helpers for the wp-api client wp.api.model.{type}.autosave()
and wp.api.model.{type}.getAutosave()
. I still need to add a getAutosave
for revision collections which I see the backend supports, and maybe some tests.
This ticket was mentioned in Slack in #core-restapi by azaozz. View the logs.
7 years ago
#16
@
7 years ago
After talking through this with @joehoyle, my main question is around how this behaves in situations where you are saving an autosave of a post type which does not support revisions? I suspect he has additional feedback to share soon.
#17
@
7 years ago
From what I gather this means PUTing to /wp/v2/posts
with { is_autosave: true }
will not necessarily update the item but could instead actually create a new autosave revision for the user. I think this is a very bad idea, because a PUT update to an object should update the object. Breaking that fundamental assumption is creating inconsistent design. If doing:
var post = new wp.api.models.Post(); post.set( 'title', 'autosave test' ); post.save(); post.set( 'status', 'publish' ); post.save(); post.set( 'title', 'updated autosave test' ); post.save( { is_autosave: true } );
actually creates a new post in the DB, and that can be fetched via /revisions
then it doesn't make sense (IMO). We should be creating a new revision (with an autosave flag if needed) at /revisions
. The Core behaviour is kinda weird (updates unpublished posts, but creates auto saves for published) which I think would be best left to client implementation to handle, rather than baking this into the endpoint. It breaks assumptions around PUT to object
causes an update, POST to /posts
creates an object, etc.
I think the idea of using autosave
for the post_type might be been discarded, but if not I'd say similar things about that. No need to use an existing field, I think it's likely to just increase complexity rather than a dedicates field on /revisions
for it.
This ticket was mentioned in Slack in #core-restapi by azaozz. View the logs.
7 years ago
#19
follow-up:
↓ 20
@
7 years ago
My suggestion is to use a new route /autosaves
extending the revisions controller.
The concept of an "Autosave" is different to the concept of a "Revision", even though right now, they are technically implemented in a similar manner and stored in the same table. That could potentially change at any moment, though. Routes should be modeled after concepts, not how stuff is stored in the DB.
The /autosaves
endpoint would be used on the client to "trigger" an autosave. The server then decides how (or if) to store that autosave. The server then emits a response to the client, which could contain contextual data that the client needs to provide needed visual feedback. As an example, it could provide the latest valid autosave (=revision) ID. This can be used on the client to adapt its state.
Letting the client decide whether to actually store the autosave or not is not recommend imho, because:
- The client does not have the necessary knowledge to make this decision. It would either require direct access to the DB or multiple network roundtrips to decide.
- The client should be in control of the logic that defines user interactions, and the server should be in control of the logic that defines the business domain. A client is not to be trusted, so giving it too much decisional power is a security liability.
#20
in reply to:
↑ 19
@
7 years ago
Replying to schlessera:
My suggestion is to use a new route
/autosaves
extending the revisions controller.
I agree, this was my original idea in autosave-controller.diff and https://github.com/WordPress/gutenberg/pull/4156. not much has to change since autosaves are so close to revisions.
Letting the client decide whether to actually store the autosave or not is not recommend imho, because:
The client does decide sometimes when to save an autosave verses a revision because these are functionally very different:
- An autosave is created automatically by the client on a timed interval, whenever the post has changed from the last autosave. Autosaves protect against content loss if the browser crashes for example. one autosave is created per user, so each user has a backup of their editing session at all times.
- Revisions are created when a draft post is saved, a published post is updated. This is the result of direct user action - clicking a button, and not automatically.
Also:
- Ideally the server should automatically create a revision if you have made significant changes. this way, if you typed a 10 thousand word document without saving, you would have revisions along the way not just a single autosave.
- Currently published posts only get a revision when they are updated, it would be great if published posts captured revisions, the previous item would solve this.
- Autosaves are used for previews. When previewing, an autosave is created of the current editor state, then that 'post' is previewed on the front end.
#21
@
7 years ago
The /autosaves endpoint would be used on the client to "trigger" an autosave.
Currently autosaves would either update or create a revision, or update the actual post if it is a draft. We need the Autosaves controller to be able to handle updating the post too (without creating another revision).
Ideally the server should automatically create a revision if you have made significant changes. this way, if you typed a 10 thousand word document without saving, you would have revisions along the way not just a single autosave.
Right. This is not a new idea, been around for some years. Not implemented till now because of "revisions use too much resources" concerns. Lately we are using posts (CPTs) for whatnot, lets add it, would be a good user facing enhancement :)
The only thing I'm not sure about is how to detect when significant changes have been made in the incoming autosave. A simple strlen() might work, but... not that good. The other option is to make a diff with the latest revision and count the changes.
Currently published posts only get a revision when they are updated, it would be great if published posts captured revisions, the previous item would solve this.
Yes, it will. Also don't see a problem for the client to save a new revision whenever. There is a revisions controller for that :)
@adamsilverstein could you update your patch as per the above :)
#22
follow-up:
↓ 23
@
7 years ago
@azaozz - Sure I'll work on an update of the patch, can you describe the use case for this? "Autosaves controller to be able to handle updating the post too (without creating another revision)."
#23
in reply to:
↑ 22
@
7 years ago
Replying to adamsilverstein:
can you describe the use case for this? "Autosaves controller to be able to handle updating the post too (without creating another revision)."
That's already in the patch, just better to do define( 'DOING_AUTOSAVE', true );
instead of removing and then re-adding the action remove_action( 'post_updated', 'wp_save_post_revision' );
. Plugins may be "watching for" the constant too.
#24
@
7 years ago
Related: After today's REST API chat, I took a stab at introducing more flexibility regarding query arguments for collection requests in the revisions controller (see #40510, which was a fitting existing ticket).
This doesn't directly benefit the autosave problem, but helps with another use-case of Gutenberg. Regarding the discussion taking place here, it is mainly something to consider regarding the implementation of an /autosaves
controller that would extend the /revisions
controller. Also, reviews/feedback always welcome!
#25
@
7 years ago
- register the autosaves controller
- cleanup after testing
still working on the other points.
Testing:
#26
@
7 years ago
In 43316.3.diff:
- update API fixture for quinit tests
- phpunit tests: adjust expected results from api schema
- only add autosaves route to post types that support revisions
- docblock updates
#27
follow-up:
↓ 29
@
7 years ago
In 43316.4.diff:
- define DOING_AUTOSAVE true instead of un/re hooking wp_save_post_revision
Working on adding some tests, appreciate any feedback on the controller class itself.
#29
in reply to:
↑ 27
@
7 years ago
Replying to adamsilverstein:
43316.4.diff is looking good. Couple of things:
- Perhaps declare the constant at the beginning of
create_item()
to match current behavior. Also an ! if_defined() check is always good :)if ( ! defined( 'DOING_AUTOSAVE' ) ) { define( 'DOING_AUTOSAVE', true ); }
-
// This bit if ( 'draft' === $parent->post_status ) { ... // needs to be if ( $post_author == $parent->post_author && ( 'auto-draft' === $parent->post_status || 'draft' === $parent->post_status ) ) { ...
- The
get_item()
should probably also account forpost_author
. Seeget_autosave( $request )
in 43316.1.patch.
- There may be more than one autosave if several people have edited the post. Do we get all of them in
get_items()
? IMHO logically we should, need to loop through all revisions and pick only the autosaves.
- What are we going to return after updating the draft or saving or deleting an autosave, (when
create_item()
runs)? At the moment it's all over the place: WP_Error, or 0 or the autosave post or the updated draft. We need to pick one :)
In the Slack chat the idea was to only return the ID, but that ID can be to the actual post, to the new or updated autosave revision, or 0 if the autosave revision was deleted so not sure. Or we can return either the post (when a draft was updated) or the autosave revision (when updated or created), or WP_Error (with appropriate error message) when error or the autosave was deleted.
#30
follow-up:
↓ 31
@
7 years ago
@azaozz Thanks for the feedback, I will work on addressing these items soon. Couple of questions...
For #4 currently i return the current users 'autosaves' (there is only ever one). I can see the use case for wanting all autosaves, then you pass author_id to find your own autosave, right?
For #5 it should return the autosave or an error. Thinking about this more, I don't think the autosave controller should update the post, even for drafts. the client can decide whether to issue a post update or create an autosave based on the post status - this seems like a client responsibility.
In 43316.5.diff I added a suite of unit tests matching what we have for revisions. I started working on enabling EDITABLE support (so you could 'update' an autosave), realizing now we don't have that for revisions so going to remove.
#31
in reply to:
↑ 30
@
7 years ago
Replying to adamsilverstein:
For #4 currently i return the current users 'autosaves' (there is only ever one). I can see the use case for wanting all autosaves, then you pass author_id to find your own autosave, right?
Yeah, I'm not so sure about that either. But then what is the difference between get_item()
and get_items()
? There are cases with more than one autosave pet post (created for different users). This is currently exposed on the Post Revisions screen, together with the rest of the revisions. Should we return all of them or should we just remove get_items()
?
There is a need to expose wp_get_post_autosave()
through the API. Need to get an autosave without knowing the autosave_id, just by the post_id. How are we doing that?
For #5 it should return the autosave or an error.
Ok, then we need to remove the 0
.
Thinking about this more, I don't think the autosave controller should update the post, even for drafts. the client can decide whether to issue a post update or create an autosave based on the post status - this seems like a client responsibility.
This was one of the main "problems" discussed in Slack. Think we settled on "The cliend should care about the user UI and how to display the data, the server should care about the business (of saving)".
If we don't update drafts when the client does autosave, how is that going to work? It is not possible to do through the posts controller. Or we have to add a hack (a.k.a a flag) to disable automatic revisions on save.
Thinking we should go with the consensus from the chat in Slack and let the autosaves controller take care of the "business" of saving where appropriate. If the client wants to save a post instead of autosaving it, it can at any time :)
I started working on enabling EDITABLE support (so you could 'update' an autosave), realizing now we don't have that for revisions so going to remove.
Right. Revisions should never be updated. Think of them like user created "backups" for the post. Autosaves should be updated, they are "special" revisions where the special part is being updated or deleted automatically, that's the "auto" part in the name :)
In 43316.5.diff I added a suite of unit tests matching what we have for revisions.
In 43316.1.patch I added few autosave tests but for the patched posts controller. Need to fix them to run for the autosaves controller.
#32
@
7 years ago
As we are on the subject of autosaves, thinking we can (finally) add the enhancement of creating a revision when the autosave data is significantly different. This will cover editing draft posts where the user doesn't do "Save Draft" often enough, and an error or a crash may send an empty autosave. Currently we would wipe the post in these cases without a way to recover it (as autosaves cannot guess user intentions).
This is still going to be an "automatic" decision. We will need a good method of determining when the post data is sufficiently different, strlen()
may work but some basic diff that counts the changes would be better.
Basically we will "redirect" a draft autosave requests to do "full" save (update the post + create a revision) when the content of the autosave is sufficiently different from the last revision.
#33
follow-up:
↓ 35
@
7 years ago
what is the difference between get_item() and get_items()?
get_items only requires that you know the parent post id. to use get_item, you need to know the item id. One other issue with returning all autosaves: you only have permission to read the autosave belonging to your user. I think the
If we don't update drafts when the client does autosave, how is that going to work?
Ah, I see what you are saying - the client wants to update the draft without creating another revision. Perhaps this logic should be in the posts controller? - that is, if you update a post and its in a draft state, set a flag that prevents a revision from being created. so then for autosaves, the client would need to send post updates if the post is draft or autosaves otherwise.
This is still going to be an "automatic" decision. We will need a good method of determining when the post data is sufficiently different, strlen() may work but some basic diff that counts the changes would be better.
performance of the check of how much has changed is important so we should probably keep it simple, as the text grows large and this will fire on every autosave. str_len
may be all we need?
#35
in reply to:
↑ 33
@
7 years ago
Replying to adamsilverstein:
to use get_item, you need to know the item id.
Hm, this is pretty useless for autosaves (that can be deleted on the next request). When would a client know the autosave ID for a post? Would it need to do another request to get it first? Then, why two requests? :)
Generally the client doesn't care about autosaves except when loading a post for editing. Then it should "know" if there is an autosave that is newer than the post.
If get_item()
for autosaves has to be done only by autosave ID, there is no point of it. An autosave can already be requested by ID through the revisions controller.
One other issue with returning all autosaves: you only have permission to read the autosave belonging to your user. I think the
No. Autosaves are revisions and inherit
the permissions of the parent post. Anybody that can edit the post can view the autosaves (just like the rest of the revisions).
Ah, I see what you are saying - the client wants to update the draft without creating another revision. Perhaps this logic should be in the posts controller?
Yeah, that was the gist of the discussion in Slack as far as I understood. We shouldn't use the posts controller (with flags) for autosaving. That's why we went with a separate controller.
performance of the check of how much has changed is important so we should probably keep it simple, as the text grows large and this will fire on every autosave.
str_len
may be all we need?
Yeah, perhaps can start with that. lets say 20% difference in size would trigger a "full save" on autosave requests.
#36
@
7 years ago
@azaozz Thanks for the feedback and clarification.
Hm, this is pretty useless for autosaves
I see what you mean - the singe endpoint is mostly useless, I think the client would hit the collection endpoint, limiting to the a specific user.
When would a client know the autosave ID for a post?
When the client autsaves, the api returns the created autosave ID. Also, on load the id (and revisions ui link) is localized (in my PR) so in theory the client might want to load that autosave data.
Generally the client doesn't care about autosaves except when loading a post for editing. Then it should "know" if there is an autosave that is newer than the post.
I have this part in Gutenberg in https://github.com/WordPress/gutenberg/pull/4218
No. Autosaves are revisions and inherit the permissions of the parent post. Anybody that can edit the post can view the autosaves (just like the rest of the revisions).
Ah, right. So show all autosaves, with the ability to filter by user
We shouldn't use the posts controller (with flags) for autosaving
Right I'm saying more that the posts controller shouldn't create revisions when the post is in the draft state (unless they pass the threshold we decide on).
Similarly, for published posts autosaves don't create a revision unless they meet the threshold. Explicit saves always create a revision.
I'm going to be afk for about a week, then will return and pick this up. Feel free to add the remaining changes.
#37
@
7 years ago
- If the autosave content is significantly different, create a revision. I used a value of 250 (characters) which i think is around a paragraph, i realized a percent might not work, for a very large document for example a revision would only get made aver a huge change. one save per paragraph seems about right, i'd like some feedback though. is this approach multi character compatible I wonder?
- Add a filter
wp_create_revision_for_api_autosave
enabling control of the revision creation behavior - For the collection endpoint, return all the autosaves, not just those belonging to to the current user.
- Regenerate fixtures.
@azaozz what else remains?
#38
@
7 years ago
If the autosave content is significantly different, create a revision. I used a value of 250 (characters)
"Significantly different" is a hard thing to define (and the target of many an algorithm). I could write a haiku, blast it away and start from scratch, and it's surely significantly different, though doesn't qualify by this definition. This is where @azaozz's suggestion of percentage difference may work better (though still not so much for my haiku destruction). As implemented, it's based on the addition of characters from the new content, meaning also that eliminating content doesn't qualify (absolute difference). Finally, if we follow-through with this metric, would we want these thresholds to be filterable?
Related:
- http://www.php.net/manual/en/function.similar-text.php
- http://us1.php.net/manual/en/function.levenshtein.php
(To prior notes of performance, both of these may have poor performance characteristics for comparing long texts)
#39
@
7 years ago
In 43316.9.diff:
- Fixed/tweaked logic in
create_item()
. - Added check if
DOING_AUTOSAVE
is defined. - Added check if the post_author is the current user when updating a draft.
- Added slashes when calling
wp_update_post()
. - Removed
update_item()
. The "business logic" of autosaves is either to create, delete or update the autosave revision. Don't think we need a separateupdate_item()
which may actually need to delete the item :) - Fixed the logic a bit and added slashing to
create_post_autosave()
. - Changed the detection of "significantly different" content. Now uses combination of percentage for shorter content and fixed size for longer content. It's still not ideal, but seems sufficient and very fast.
Still TODO: thinking we need few more tests especially testing content with slashes. That would be good for the posts controller too :)
@adamsilverstein could you have a look if all is good? :)
#40
@
7 years ago
@azaozz
Great, I'll take a look!
I like the idea of using the percentage method for shorter text, which the likely the majority use case. Ideally, when Gutenberg will also add localstorage support for autosaves and we can store those much more frequently.
@aduth
Finally, if we follow-through with this metric, would we want these thresholds to be filterable?
A single filter at the end of the process that passes the difference calculation results (wp_create_revision_for_api_autosave
) is sufficient for plugins to provide custom behavior based on the difference. An extra filter for the threshold by itself seems like overkill.
#41
@
7 years ago
In 43316.10.diff:
- Attempt to fix routes.
- Fix bug trying to create revision of a revision.
- Fix and move the logic for creating a significantly different revision when autosaving.
- Also create that revision when overwriting a draft post (most important).
- Added the tests from 43316.1.patch.
Still TODO/TBD:
- The autosave route can be better? Currently it looks like
/wp/v2/posts/123/autosaves/123
where123
for both/posts/
and/autosaves/
is the actual post_ID and has to be the same. - Moar tests, especially testing the autocreated "significantly different" revision.
#42
@
7 years ago
In 43316.11.diff:
- Make the autosave route
POST
to/wp/v2/posts/123/autosaves
(the default for creating a child item). The permissions check is whether the current user can edit the post. - Tweak the PHP tests accordingly.
This ticket was mentioned in Slack in #core-editor by pento. View the logs.
7 years ago
#44
@
7 years ago
- tiny whitspace/typo fixes
- fix a unit test, switched
$autosave_data['content']
to$autosave_data['post_content']
@ L211
Still want to try to do some more testing manually, also seems fine merging this and then testing it in Gutenberg.
#45
follow-up:
↓ 47
@
7 years ago
Offering some perspective from a fresh pair of eyes. Feel free to discard any feedback you consider unqualified, given I haven't been following the conversation closely.
From the user's perspective, I see two goals:
- I should never lose what I'm currently working on (autosaves).
- I should be able to see, and restore, my history of edits (revisions).
If we agree these are two distinct goals, then it would logically make sense to have two separate REST API endpoints because there are two distinct sets of behaviors:
- The autosaves endpoint is read/write. There is only one 'autosave' resource per user. A client can create an autosave for a user, retrieve the autosave for the user, and retrieve all autosaves for all users.
- The revisions endpoint is read-only. Revisions are created by server when a resource is saved. The client can only retrieve all revisions (I've forgotten how we support "restoring" revisions).
And then we get in to the programmatic implementation of the abstraction:
- It seems magical to create a revision based on some algorithm applied to the saved data. I'd think this logic makes more sense client-side; let the client decide whether to perform a full save or an autosave.
- Similarly, if revisions are only created when a post is fully saved, then I think it makes sense to keep that distinction in the client-side UX (i.e. you hit the "Save" button to create a revision).
- Because autosaves are user-specific, paths and supported operations would be:
GET /wp/v2/posts/<post-id>/autosaves
- List all autosave objects.POST /wp/v2/posts/<post-id>/autosves
- Create a new autosave object.GET /wp/v2/posts/<post-id>/autosaves/<user-id>
- Get the autosave object for a specific user.
- I don't think developers should have to opt-in to autosaves using
post_type_supports()
. Server-side (and client-side) autosave should be a default feature of the WordPress UX.
For a fun thought experiment, or dark rabbit hole, imagine revisions and autosaves for individual blocks :)
Let me know if any of this is off-base, or if there's additional input I can provide.
#46
@
7 years ago
Hey @danielbachhuber thank you for your feedback! Mostly I think you are right on, and I appreciate the fresh perspective. Here are some responses to specific points you raise...
If we agree these are two distinct goals...
Yes, I agree we need two distinct endpoints almost as you describe them, except I think the revisions endpoint can already delete revisions?
It seems magical to create a revision based on some algorithm applied to the saved data. I'd think this logic makes more sense client-side; let the client decide whether to perform a full save or an autosave.
Yes, it is somewhat magical I agree :) In a good way. WordPress decides when you have made enough of a change that more than an autosave is warranted. this would happen if you were updating your post with any client and save you from disaster if you lost your single autosave. Also, this lets us save revisions for published posts without updating them, which currently is not possible.
Similarly, if revisions are only created when a post is fully saved, then I think it makes sense to keep that distinction in the client-side UX (i.e. you hit the "Save" button to create a revision).
Here we are proposing that revisions are created whenever an autosave occurs and the content has changed not insignificantly. This expands the definition of autosaves and lets WordPress automatically track your changes over time versus keeping only a single backup (especially for published posts). This protects users by capturing a history of changes. A filter is provided to disable this behavior.
Perhaps clicking 'update' or 'save' could remove all of the intermediate revisions so we can avoid clogging up the posts table with revisions. Is this part of your concern?
What if you want to save your work in progress when updating an already published post?
GET /wp/v2/posts/<post-id>/autosaves/<user-id>
Shouldn't this endpoint match revisions? GET /wp/v2/posts/<post-id>/autosaves/<autosave-id>
For a fun thought experiment, or dark rabbit hole, imagine revisions and autosaves for individual blocks :)
Would you be surprised if I told you I've already been thinking about this?
#47
in reply to:
↑ 45
@
7 years ago
Replying to danielbachhuber:
From the user's perspective, I see two goals:
- I should never lose what I'm currently working on (autosaves).
- I should be able to see, and restore, my history of edits (revisions).
If we agree these are two distinct goals, then it would logically make sense to have two separate REST API endpoints because there are two distinct sets of behaviors:
Yes, there are two distinct goals and we have two separate REST API endpoints for them. Actually we have two separate REST API controllers for them: WP_REST_Autosaves_Controller
and WP_REST_Revisions_Controller
.
- The autosaves endpoint is read/write. There is only one 'autosave' resource per user. A client can create an autosave for a user, retrieve the autosave for the user, and retrieve all autosaves for all users.
Not quite.
- The autosaves endpoint is a duplicate of the
save
endpoint. The only reason it exists (in a separate controller) is to pass a flag to the server when the client has initiated an automatic save. I'm not sure which makes more sense from pure "RESTfull" point of view: having a flag or having a separate controller that partially duplicates theWP_REST_Posts_Controller
. - "There is only one 'autosave' resource per user." No. There are no separate 'autosave' resources. These are the same 'post' and 'revision' resources.
- "A client can create an autosave for a user". No. A client cannot create an autosave. A client can save the post, and the server decides what to do according to its "business logic".
- "...retrieve the autosave for the user". Yeah, the client should be able to get the latest revision for a post. This has nothing to do with who the user is. Autosave revisions can be retrieved through the revisions controller. Not really sure we need to duplicate this in the autosave controller.
- "...and retrieve all autosaves for all users". This can also be done through the revisions controller. Not sure we need the duplicate here too.
- The revisions endpoint is read-only. Revisions are created by server when a resource is saved. The client can only retrieve all revisions (I've forgotten how we support "restoring" revisions).
Right. Revisions are typically restored in the client, after the user chooses the content. Then the post is saved as usual.
Revisions can also be deleted by the client which generally shouldn't be possible as they are "audit trail". The client is generally "untrusted" and shouldn't "mess with" the server logic. Imagine an author
level user managing all revisions of a post they started, including the revisions that were made by the editor(s). But this is a different discussion/weakness/bug.
And then we get in to the programmatic implementation of the abstraction:
- It seems magical to create a revision based on some algorithm applied to the saved data.
Are you talking about the "disaster recovery when the user was editing for 6 hours and didn't save even once and at the end there was a (network or human) error and they lost all their work" mode? :) There is nothing "magical" about it. It's just a rare edge case we decided to cover in the server logic. Again, this is the "business logic" when saving a post, nothing to do with the client.
- Similarly, if revisions are only created when a post is fully saved, then I think it makes sense to keep that distinction in the client-side UX (i.e. you hit the "Save" button to create a revision).
Yep, agreed. That is up to the client to decide. This is also the way it currently works on the Edit Post screen.
Also, revisions may be created or deleted at any time depending on the server logic. There may be restrictions on how many revisions to keep, revisions may be disabled altogether, plugins may decide when and how to save a revision or to prevent it from saving. None of this logic belongs in the client.
- Because autosaves are user-specific, paths and supported operations would be:
GET /wp/v2/posts/<post-id>/autosaves
- List all autosave objects.POST /wp/v2/posts/<post-id>/autosves
- Create a new autosave object.GET /wp/v2/posts/<post-id>/autosaves/<user-id>
- Get the autosave object for a specific user.
Autosave revisions are not user specific. The user that is also author of the post usually doesn't get an autosave revision while editing. When saving, the logic for creating autosave revisions looks at who is the post author and who is the current user only to prevent content loss/overwriting.
I'm actually thinking we should remove either the "get single autosave" or the "get all autosaves" endpoint. Doesn't make sense to have both on top of all revisions endpoints (that also retrieve autosaves).
#48
@
7 years ago
Thanks @azaozz, @adamsilverstein. I'll reply to the few points I can easily comment on:
Here we are proposing that revisions are created whenever an autosave occurs and the content has changed not insignificantly. This expands the definition of autosaves and lets WordPress automatically track your changes over time versus keeping only a single backup (especially for published posts). This protects users by capturing a history of changes. A filter is provided to disable this behavior.
Has this idea been validated as useful in the real world? Given the tendency of revisions to bloat the database, it seems like it might be worthwhile to verify this is a valuable feature for end users.
Another way of considering: if autosaves create new revisions by some multiple (e.g. 5x), then it will change the UX of working back through the revision history.
Other ideas: only keep the most recent five auto-saves as revisions, only create an auto-save revision every five minutes, etc.
What if you want to save your work in progress when updating an already published post?
We need future revisions! But yeah, interesting question.
I'm not sure which makes more sense from pure "RESTfull" point of view: having a flag or having a separate controller that partially duplicates the
WP_REST_Posts_Controller
.
If the shape of the data is the exact same, then we could co-opt the status
attribute to trigger an auto-save (i.e. status=auto-save
).
If the shape of the data is different, then we need a different REST endpoint.
REST is all about the resources: Posts, Users, Terms, etc. A Post has id
and title
, whereas a user has id
and name
. Because the data is shaped differently, they exist at different endpoints.
Generally, in thinking about this more, I'd be leaning closer to one revisions endpoint that covers past and future revisions. We could handle write operations through /wp/v2/posts
with special status
attribute handling.
Even taking a step back from this, it seems like it might make sense to produce a design document capturing all of the detail on how the endpoint(s) should behave, and write code once that's determined. Right now, it's difficult to understand the decisions and consensus through all of the Trac comments and patches.
#49
@
7 years ago
@danielbachhuber thanks...
If the shape of the data is the exact same, then we could co-opt the status attribute to trigger an auto-save (i.e. status=auto-save).
The shape of the data is identical - its a post thats being saved, both for posts and for autosaves. The problem with status is an autosave can have a published or draft status, we need to store the status. In https://core.trac.wordpress.org/ticket/43316#comment:10 @azaozz proposed using post type as the 'flag' that triggers an autosave. see 43316.1.diff
Generally, in thinking about this more, I'd be leaning closer to one revisions endpoint that covers past and future revisions.
So, add the ability to explicitly create revisions?
Right now, it's difficult to understand the decisions and consensus through all of the Trac comments and patches.
I agree - and there was additional discussion in slack around this ticket as well.
#50
@
7 years ago
@danielbachhuber thanks for the review! :)
Yes, as @adamsilverstein points out above the shape of the data is identical - it is a post that is being saved (hence my idea to use the same controller/endpoint). That includes all of the post "properties", post_type, post_status, post_parent, etc.
If we are using WP_REST_Posts_Controller::update_item()
to do both saves and autosaves, we need some sort of a flag that is different than the post properties, see 43316.1.patch that uses is_autosave
.
Generally, in thinking about this more, I'd be leaning closer to one revisions endpoint that covers past and future revisions. We could handle write operations through /wp/v2/posts with special status attribute handling.
Right, thinking the same.
Even taking a step back from this, it seems like it might make sense to produce a design document capturing all of the detail on how the endpoint(s) should behave, and write code once that's determined.
It's a bit too late for this :) The REST API is actively being used now, and the lack of autosave support means it is creating tens or even hundreds of revisions when it shouldn't. It's not uncommon to see 120-150 revisions for an average length post :(
Generally autosaves are "real" saves but the "business logic" on the server is a bit different. It depends on post_status, post_author vs. current_user, and post_lock (not implemented in the API yet!). It also depends on decisions in some plugins.
Has this idea (auto-creating a revision when an autosave is significantly different) been validated as useful in the real world? Given the tendency of revisions to bloat the database...
Yes, it has. It protects the users in a rare edge case when post_content may "disappear" as a result of a user (or plugin/network?) error. Was planning to add that to the non-API autosave too, have heard of many disastrous cases over the years.
What if you want to save your work in progress when updating an already published post?
We need future revisions! But yeah, interesting question.
Perhaps we can even look at reducing the size diff that will trigger creating a revision when autosaving a published post. But that is not super urgent. We can adjust how this works when we have more "real life" data.
@adamsilverstein @aduth @danielbachhuber @flixos90 @schlessera I'm going to refresh 43316.1.patch over the weekend (yeah, I know it's a holiday, but this cannot wait any longer) and then commit it, if no other objections.
#51
@
7 years ago
One last suggestion :) Would it make sense to change is_autosave
to something like save_type
to leave the door open for other save types in the future?
This ticket was mentioned in Slack in #core-restapi by azaozz. View the logs.
7 years ago
#54
follow-up:
↓ 55
@
7 years ago
If the shape of the data is the exact same, then we could co-opt the status attribute to trigger an auto-save (i.e. status=auto-save).
The shape of the data is identical - its a post thats being saved, both for posts and for autosaves. The problem with status is an autosave can have a published or draft status, we need to store the status. In https://core.trac.wordpress.org/ticket/43316#comment:10 @azaozz proposed using post type as the 'flag' that triggers an autosave. see 43316.1.diff
I still recommend looking at the concept you want to model, not how its data happens to be stored. Putting two models into the same endpoint because their data happens to be saved in the same way is the wrong way of doing DRY. As soon as they need to diverge again (posts needing to do something autosaves shouldn't, or vice-versa) you're facing a big problem again. You could for example imagine autosaves being stored as incremental diffs in a separate table through a later optimization.
That's why I recommend a separate autosaves
endpoint, not some trickery with posts or revisions. Yes, it can just extend one of the existing controllers, but the actual public API should cleanly separate the concepts, not let the current technical implementation bleed through.
#55
in reply to:
↑ 54
@
7 years ago
Replying to schlessera:
I still recommend looking at the concept you want to model, not how its data happens to be stored.
I agree. The concept is that a post is being saved and a revision is being (auto)created. The only difference between a save
and an autosave
is that the latter employs additional server logic when creating the revision.
Putting two models into the same endpoint because their data happens to be saved in the same way...
These are not different models. It is the same model with the same data performing the same action of saving a post to the database.
As soon as they need to diverge again (posts needing to do something autosaves shouldn't, or vice-versa) you're facing a big problem again. You could for example imagine autosaves being stored as incremental diffs in a separate table through a later optimization.
Wouldn't that depend on the server's "business logic"? It depends on it now. We auto-create revisions on each save. If these revisions are to be moved to a different method of saving, should we deprecate the revisions controller and stop auto-creating them? :)
It doesn't seem likely for revisions to be stored as incremental diffs in a different table, unless perhaps we drop MySQL and move to git for storage :) Then we will drop the concept of revisions altogether and won't need to flag the autosave revisions.
That's why I recommend a separate
autosaves
endpoint, not some trickery with posts or revisions.
The problem we seem to be stuck on is that autosaves do not exist outside of posts and revisions. Frankly I don't care much how DRY or non-DRY another controller may be, as long as it is logical, and easy to understand how it works and why.
...but the actual public API should cleanly separate the concepts, not let the current technical implementation bleed through.
That's exactly my point too :) The concept is that a post is being saved and a revision is being auto-created. There is a different server logic when this is done directly by the user or is triggered automatically by the client.
I'm not sure how to stress this more: autosaves are not a different concept of saving. They are identical to "standard" saves. The difference is in the logic that auto-creates the post revisions.
In any case, we have two patches that work well here:
- One adds the specialized revision logic to the existing posts and revisions controllers.
- The other introduces a new controller that partially duplicates the posts and revisions controllers and performs identical actions, but introduces different end points.
This ticket has been "simmering" for over a month now and is urgent. Can we please get on the same boat, pick a patch and commit it already? :)
#56
follow-up:
↓ 57
@
7 years ago
Just want to add the distinction that I wasn't talking about controllers above. It is pretty irrelevant how we split up controllers and where we put them. That's a technical detail in the REST API black box and can be changed at will.
What matters are the endpoints. I think that a POST /posts/
is a different concept than a POST /autosaves/
. Yes, they share the same code and data model, and yes, they can happily use the same controller (for now). The thing is that the action that the client requests is a different one, and should be expressed in that way. All the other discussions are details that the client does not need to know.
When we now split up /posts/
to mean "trigger saving a post" AND "trigger saving an autosave", that is just immediate technical debt and raises the complexity in an unneeded way. And yes, adding a separate endpoint means having a bit more code, but the complexity actually gets lowered, because we have less branching in one individual code path. Most importantly, though, it doesn't need a rewrite of the client in case the handling of "trigger saving a post" and "trigger saving an autosave" should later diverge.
The above being said, I'm not the one in a position to decide here, I just wanted to make sure my concerns are heard. I thought that we had already previously agreed to go with a separate /autosaves/
endpoint (as it seems the more future-safe approach, regardless of any other factors), but I'm happy to help refine whatever approach you finally plan to commit.
#57
in reply to:
↑ 56
@
7 years ago
Replying to schlessera:
What matters are the endpoints. I think that a
POST /posts/
is a different concept than aPOST /autosaves/
.
...
When we now split up/posts/
to mean "trigger saving a post" AND "trigger saving an autosave"...
Sorry I'm going to repeat myself but autosaving is "trigger saving a post". It is the same concept of "saving a post", uses the same data shape, form, and size, and performs the same action as a result of the same intent by the client.
The logical problem in having POST /autosaves/
endpoint is that it is "superficial". Autosaves don't exist outside posts and revisions.
Anyway, think we delved into very "philosophical" discussion here :)
I'm assuming that:
- You are +1 on the patch that introduces an
autosaves
end point. - Daniel is a +1 on the patch that adds autosave support to the
posts
endpoint.
Anybody else?
#58
@
7 years ago
Sorry I'm going to repeat myself but autosaving is "trigger saving a post". It is the same concept of "saving a post", uses the same data shape, form, and size, and performs the same action as a result of the same intent by the client.
Please note that you're talking about integrating a "switch" to distinguish between them so that you can have differing behavior between the two.
But, yes, I agree, the discussion has taken place. I'm indeed +1 on an /autosaves/
endpoint.
#59
follow-ups:
↓ 61
↓ 62
@
7 years ago
I've been watching this ticket for a while and thinking about it; it's a tough problem to crack. Trying to summarise what I think the difficulties and decisions to make are:
I think the key thing we need to get at is defining what an "autosave" is. @azaozz mentions 'autosaving is "trigger saving a post"'; how does autosaving differ from a regular update in that regard? When would I want to differentiate a manual save from an autosave?
To me, the basic logic flow seems something like:
- User starts editing: this creates a sort of "future revision" that they're working on, forked from the original post
- User makes some changes: this updates the "future revision" that they're working on
- User hits save: this "commits" the "future revision", and saves the original post
The difference between 2 and 3 is why we need the distinction between manual saves and autosaves. Only 3 should create a "real revision", whereas 2 is just internal behaviour. Does this seem accurate?
IMO we need to get a concrete mental model of how this works, independent of the actual internal behaviour.
(We never really used it, but the template in the proposals repo would be useful to establish a design doc for this.)
I quite like the concept of treating autosaves as actual future revisions from the client's perspective. Both revisions and autosaves share the same logical underpinnings of being an alternative version of the post with changes, and the post can be updated to use that version's data instead (whether restoring a revision or "committing" an autosave). I'm not certain on the actual implementation of autosaves, but it seems like they use revisions under the hood?
The flow I'm thinking would be something like:
- User starts editing:
POST /wp/v2/posts/{id}/revisions
creates a new "future revision" - User makes some changes:
PUT /wp/v2/posts/{id}/revisions/{rev_id}
updates this "future revision" - User hits save:
POST /wp/v2/posts/{id}/revisions/{rev_id}
commits the "future revision"
The main change here would be this idea of updating a "future revision", which would avoid the massive-amounts-of-revisions problem. It would be entirely up to the client to control the autosave behaviour and when to commit, which is definitely the right choice to make IMO. (This may require changes to the underlying API in WP, as I'm not sure what's supported around this.)
If we don't build it into the revisions endpoint, I'm +1 on a separate autosaves endpoint, which I think makes more sense than having a flag in regular updates. I'm not sure the the current behaviour in 43316.12.diff makes sense though; you POST to /autosaves, which updates the actual post?
Also, I am -1 on this "detect how much has changed" idea; that seems like a pretty fundamental change, and not one that we should make in the context of this issue.
(I've forgotten how we support "restoring" revisions).
We never actually ended up including this ability, since IIRC we thought it may someday interfere with autosaving. :)
I'd propose that we go with the original design, which is POST /wp/v2/posts/{id}/revisions/{rev_id}
restores a revision; this fits with the same action "committing" a "future revision" as well.
#60
@
7 years ago
I'm not certain on the actual implementation of autosaves, but it seems like they use revisions under the hood?
I've been digging into the implementation here; autosaves do indeed create revisions. They are identified by the slug being {id}-autosave
. wp_get_post_autosave()
and wp_create_post_autosave()
appear to be where the one-autosave-per-user concept is handled.
In the REST API, I would map these to a query instead. Something like /revisions?autosave=true&author=1
would get the autosave for the current user if one exists, and you could use the revision's ID to update that autosave.
In one sense, this does expose some of the internals of how autosaves work, but I don't think that's a bad thing. I had no real idea how autosaves worked internally, and my previous thinking still aligned with this; knowing that they really are revisions internally I think solidifies the revision-based design for me.
#61
in reply to:
↑ 59
@
7 years ago
Replying to rmccue:
(We never really used it, but the template in the proposals repo would be useful to establish a design doc for this.)
I wrote up a design doc based on my comment above to lay this out a bit better.
Is there anything fundamentally wrong with this design that I'm missing? I think (hope) that it satisfies the points here.
(I'm happy to also come up with the implementation; I'm trying to get back on top of my open source time.)
#62
in reply to:
↑ 59
;
follow-ups:
↓ 63
↓ 90
@
7 years ago
Replying to rmccue:
I think the key thing we need to get at is defining what an "autosave" is.
To me, the basic logic flow seems something like:
- User starts editing: this creates a sort of "future revision" that they're working on, forked from the original post
- User makes some changes: this updates the "future revision" that they're working on
- User hits save: this "commits" the "future revision", and saves the original post
...
IMO we need to get a concrete mental model of how this works, independent of the actual internal behaviour.
Right. The current (non-API) logic is:
Save:
- User edits the post (a post is always edited as we start with an auto-draft).
- User clicks Save.
- The post is updated in the db.
- At the same time a revision of the post is auto-created with identical content.
When saving, the post is always updated and an identical revision is always created. Note that revisions (including autosave revisions) also provide an "audit trail". That's another reason revisions are not editable directly by the users.
Auto-save:
- User edits the post.
- Periodically the client checks for changes. If there are any, the client saves the post.
- The server has different logic for this save:
- If the post is a draft, it is updated in the db without creating another revision.
- If the post is a draft but the post author is different than the current user, the post is not updated and a revision is created.
- If the post is published a revision is created without updating the post.
- When creating a revision following the above logic, if an autosaved revision already exists, it is updated instead of creating another autosaved revision. Additionally if the post content is identical to the new content that is being auto-saved, the previous autosave revision is deleted to prevent having a redundant revision (i.e. if the update of the previous autosaved revision will make it identical to the current post, there is no need of it).
When auto-saving the post may be updated (most common case), or a revision may be created, updated or deleted.
The flow I'm thinking would be something like:
- User starts editing:
POST /wp/v2/posts/{id}/revisions
creates a new "future revision"- User makes some changes:
PUT /wp/v2/posts/{id}/revisions/{rev_id}
updates this "future revision"- User hits save:
POST /wp/v2/posts/{id}/revisions/{rev_id}
commits the "future revision"
This differs a lot from the current workflow. Generally when auto-saving in any app (not WP), the actual item is being updated. In WP the logic is more complex as we have revisions and states, draft vs. published. If we create and update a revision directly instead of updating the current item, it will be unexpected.
This will generally mean that a post will always have an autosaved revision. That revision will usually be more recent than the post content, and the user will have to manually "restore" that autosave revision before continuing to edit. That's not a good UX :) This can happen now too, but is relatively rare.
I don't mind changing how saving and auto-saving works in WP, but we'll need to come up with a nice workflow. Unsure if now is the best time as this ticket is urgent, but if you think we can manage it in the next 2-3 days, I'm all for trying :)
Also, the part: "User hits save: POST /wp/v2/posts/{id}/revisions/{rev_id}
" would be an actual save, i.e. POST /wp/v2/posts/{id}
.
I'm not sure the current behaviour in 43316.12.diff makes sense though; you POST to /autosaves, which updates the actual post?
Yeah, the auto-saving logic is more complex. It may update a draft post without creating a revision. See the above description.
Also, I am -1 on this "detect how much has changed" idea; that seems like a pretty fundamental change, and not one that we should make in the context of this issue.
This is a "disaster recovery" for a rare edge case that was planned for the current (non-API) autosave too. We can remove it if need be.
#63
in reply to:
↑ 62
;
follow-up:
↓ 65
@
7 years ago
Replying to azaozz:
Right. The current (non-API) logic is:
Save:
- User edits the post (a post is always edited as we start with an auto-draft).
It doesn't seem like this is necessarily true; wp_create_post_autosave shortcircuits if the post is the exact same, so it seems like an autodraft can only be created once there is a change?
When saving, the post is always updated and an identical revision is always created. Note that revisions (including autosave revisions) also provide an "audit trail". That's another reason revisions are not editable directly by the users.
We do already provide the functionality to delete revisions via the REST API, so they're not really usable for an audit trail, but rather as a sort of long-term undo. Their primary use is to see the evolution of a post over time.
Auto-save:
- User edits the post.
- Periodically the client checks for changes. If there are any, the client saves the post.
- The server has different logic for this save:
- If the post is a draft, it is updated in the db without creating another revision.
- If the post is a draft but the post author is different than the current user, the post is not updated and a revision is created.
- If the post is published a revision is created without updating the post.
- When creating a revision following the above logic, if an autosaved revision already exists, it is updated instead of creating another autosaved revision. Additionally if the post content is identical to the new content that is being auto-saved, the previous autosave revision is deleted to prevent having a redundant revision (i.e. if the update of the previous autosaved revision will make it identical to the current post, there is no need of it).
When auto-saving the post may be updated (most common case), or a revision may be created, updated or deleted.
Right now, there's no way to say "update this post, but don't create a revision". The basic thing we want for autosaves is the ability to do this, but providing this functionality so allows any REST API client to do so, which I'm a little uncomfortable about (and which would totally destroy any concept of revisions being usable as an audit trail, since now changes can be made without being recorded anywhere).
The proposed autosave process I'm talking about here would allow this, but in a very controlled way. Each change would still get a revision, but clients would now have the ability to temporarily persist changes while the user is still working on them.
The main thing that seems strange is the special-casing of the post's author. If we can remove that special-casing, I think the rest is reasonably straightforward to model. Is there a reason for this special-casing?
Changing these I think would make the whole process more predictable for clients, and easier to model.
This differs a lot from the current workflow. Generally when auto-saving in any app (not WP), the actual item is being updated. In WP the logic is more complex as we have revisions and states, draft vs. published. If we create and update a revision directly instead of updating the current item, it will be unexpected.
I think in order to preserve the concept of revisions as a useful thing, we need to do this; the alternative is to allow any client to update a post without creating a revision (unless I'm missing something there).
If clients don't want to use the autosave process, then they can keep using the existing process, they'll just potentially end up with lots of revisions. I see this autosave concept as similar to database transactions: you queue up and persist a bunch of changes, then commit them all in one revision at once. This preserves the process of always creating revisions, while reducing the amount of noise.
This will generally mean that a post will always have an autosaved revision. That revision will usually be more recent than the post content, and the user will have to manually "restore" that autosave revision before continuing to edit. That's not a good UX :) This can happen now too, but is relatively rare.
The client can implement the functionality themselves to automatically "restore" the autosave. This is essentially what we're doing already with the special-case for the post author, but editors can do it in a more consistent way.
I don't mind changing how saving and auto-saving works in WP, but we'll need to come up with a nice workflow. Unsure if now is the best time as this ticket is urgent, but if you think we can manage it in the next 2-3 days, I'm all for trying :)
I'm working on the implementation for this at the moment based on the latest, so we should have a working example to study.
Also, the part: "User hits save:
POST /wp/v2/posts/{id}/revisions/{rev_id}
" would be an actual save, i.e.POST /wp/v2/posts/{id}
.
I was thinking this would be a cleaner way of the client actually "committing" the autosaved changes. It avoids the problem where an update to a somewhat-unrelated resource (the post) affects a bunch of others (all the autosaves). This is the current behaviour though: wp_save_post_revision()
is called by wp_insert_post()
, and it wipes out all the autosaves.
(Autosaves are conceptually difficult to model as a resource because of this.)
I'll update the proposal to reflect this.
This is a "disaster recovery" for a rare edge case that was planned for the current (non-API) autosave too. We can remove it if need be.
If it's a global change, that makes sense; I'd rather track and implement it separately I think :)
(Sorry for the long post again; this is a complex topic.)
#64
follow-up:
↓ 66
@
7 years ago
- Owner set to rmccue
- Status changed from new to assigned
(If it's easier to read, there's also a PR for the proposal; I don't want to split the conversation, but might be easier to hash out smaller things there.)
#65
in reply to:
↑ 63
;
follow-ups:
↓ 67
↓ 68
@
7 years ago
Replying to rmccue:
Replying to azaozz:
Right. The current (non-API) logic is:
Save:
- User edits the post (a post is always edited as we start with an auto-draft).
It doesn't seem like this is necessarily true; wp_create_post_autosave shortcircuits if the post is the exact same, so it seems like an autodraft can only be created once there is a change?
No, currently the auto-draft is created right before the user loads the Add New (post) screen. This is done so we have a post_ID for the brand new post and covers cases where the user may upload files or add tags, categories, post meta, etc. before they actually start writing the post (we need a post_ID to "attach" the meta). I'm not sure how that works through the API at the moment, or if it is possible at all. (That has nothing to do with auto-saving as there is nothing in the post to save yet.)
We do already provide the functionality to delete revisions via the REST API, so they're not really usable for an audit trail, but rather as a sort of long-term undo.
Yeah, I'm not sure why this was added to the API. IMHO we shouldn't be removing the functionality of the audit trail. This changes the purpose and functionality of revisions quite a bit...
Their (revisions) primary use is to see the evolution of a post over time.
Perhaps we should look at removing the delete revision endpoint. It makes them "unreliable" and their purpose becomes "to maybe see the evolution of a post over time, or maybe see only some of it, or not see anything".
If I understand the general logic behind a REST API correctly, it should care about the "client" and the "view", not overtake the server "business logic". The client is generally "untrusted" in handling low level logic, that should be handled by the server. There is a good reason why revisions cannot be deleted from the current (non-API) UI :)
I'm aware that revisions can be limited or disabled altogether but that is a "server logic" decision on per site basis. Also note that this doesn't disable autosave revisions.
Right now, there's no way to say "update this post, but don't create a revision". The basic thing we want for autosaves is the ability to do this, but providing this functionality so allows any REST API client to do so, which I'm a little uncomfortable about (and which would totally destroy any concept of revisions being usable as an audit trail, since now changes can be made without being recorded anywhere).
Right. Thinking more about this, it should stay this way (no way to save without a revision). Changing that would make revisions totally untrustworthy.
The proposed autosave process I'm talking about here would allow this, but in a very controlled way. Each change would still get a revision, but clients would now have the ability to temporarily persist changes while the user is still working on them.
So basically you want to move the server auto-saving logic to the client. Not sure this is a good idea. The client can store "persistent backup data" by itself, for example in browser storage. This has nothing to do with server auto-saving. This is how autosaves work at the moment for non-API. There are server autosaves and client (in browser storage) autosaves.
Once the client decides to push a server auto-save, the logic for it should be on the server.
The main thing that seems strange is the special-casing of the post's author. If we can remove that special-casing, I think the rest is reasonably straightforward to model. Is there a reason for this special-casing?
This is mainly needed for post locking (not implemented through the API yet). It prevents data loss when more than one client tries to edit the same post. It also keeps the audit trail more consistent.
If clients don't want to use the autosave process, then they can keep using the existing process, they'll just potentially end up with lots of revisions. I see this autosave concept as similar to database transactions: you queue up and persist a bunch of changes, then commit them all in one revision at once. This preserves the process of always creating revisions, while reducing the amount of noise.
Yes, this is a possible saving workflow. A client can decide to not use the server autosave process and instead store backups locally. Then "once in a while" sync these local backups with the server. Few years ago there was an idea to make the non-API autosave work this way too. It didn't materialize as browser storage was somewhat unreliable/limited back then.
This will generally mean that a post will always have an autosaved revision. That revision will usually be more recent than the post content, and the user will have to manually "restore" that autosave revision before continuing to edit. That's not a good UX :) This can happen now too, but is relatively rare.
The client can implement the functionality themselves to automatically "restore" the autosave. This is essentially what we're doing already with the special-case for the post author, but editors can do it in a more consistent way.
Perhaps but there are some edge cases like when there are several autosaves authored by different users that have edited the post at different times. Then it can get really messy :) Thinking that restoring/overwriting a post from a revision should always be user-controlled.
The current logic is that the post author is "the boss" and other users that edit the post leave audit trail with their changes. This is essential for larger sites that have many authors and many editors.
Also, the part: "User hits save:
POST /wp/v2/posts/{id}/revisions/{rev_id}
" would be an actual save, i.e.POST /wp/v2/posts/{id}
.
I was thinking this would be a cleaner way of the client actually "committing" the autosaved changes. It avoids the problem where an update to a somewhat-unrelated resource (the post) affects a bunch of others (all the autosaves). This is the current behaviour though:
wp_save_post_revision()
is called bywp_insert_post()
, and it wipes out all the autosaves.
Hm, wp_save_post_revision()
doesn't wipe autosave revisions. It has some logic to keep a max number of revisions if they are limited by a plugin. The default is -1
(unlimited). Also note that this logic specifically bypasses (keeps) autosave revisions, even when they are over the max.
Also, the post is not an "unrelated resource" in this case. Generally we can view a WordPress post as consisting of several rows in the db: the actual post row plus all the revisions rows. They are the same "resource" from logical point of view. It doesn't matter how exactly the "post resource" is stored. As @schlessera mentions above, at some point the storage schema may change and perhaps post revisions may be stored as incremental diffs in the actual post row in the db. Well, at last in theory :)
This is a "disaster recovery" for a rare edge case that was planned for the current (non-API) autosave too. We can remove it if need be.
If it's a global change, that makes sense; I'd rather track and implement it separately I think :)
Sure, lets separate that. But maybe first get the server autosaves logic/endpoint in.
(Sorry for the long post again; this is a complex topic.)
Yeah, sorry for the long post too, very complex topic indeed :)
#66
in reply to:
↑ 64
@
7 years ago
Replying to rmccue:
(If it's easier to read, there's also a PR for the proposal; I don't want to split the conversation, but might be easier to hash out smaller things there.)
Yep, agreed. Best is to not fragment the conversation.
#67
in reply to:
↑ 65
;
follow-ups:
↓ 69
↓ 71
@
7 years ago
Replying to azaozz:
No, currently the auto-draft is created right before the user loads the Add New (post) screen. This is done so we have a post_ID for the brand new post and covers cases where the user may upload files or add tags, categories, post meta, etc. before they actually start writing the post (we need a post_ID to "attach" the meta). I'm not sure how that works through the API at the moment, or if it is possible at all. (That has nothing to do with auto-saving as there is nothing in the post to save yet.)
OK, so I guess the first logical step in the workflow is:
- User starts editing post, editor creates an autosave:
POST /wp/v2/posts/{id}/autosaves
Yeah, I'm not sure why this was added to the API. IMHO we shouldn't be removing the functionality of the audit trail. This changes the purpose and functionality of revisions quite a bit...
This was way back in https://github.com/WP-API/WP-API/pull/1110
It does check the delete cap on revisions, so I'd make the argument this should be handled in the capability system instead. This is a conversation for another day though :)
The proposed autosave process I'm talking about here would allow this, but in a very controlled way. Each change would still get a revision, but clients would now have the ability to temporarily persist changes while the user is still working on them.
So basically you want to move the server auto-saving logic to the client. Not sure this is a good idea. The client can store "persistent backup data" by itself, for example in browser storage. This has nothing to do with server auto-saving. This is how autosaves work at the moment for non-API. There are server autosaves and client (in browser storage) autosaves.
Once the client decides to push a server auto-save, the logic for it should be on the server.
The reason we want the server involved is for cross-client purposes though: switching from web to mobile, restoring autosaves by others, etc. The only thing that really requires is storing the content of the autosave on the server.
Now that I'm more familiar with the code, it doesn't seem like there's a huge amount of special casing anyway, so there's not really any complex logic that needs implementing on the client. Basically, just "if this post is a draft and I'm the post author, save directly, otherwise use autosaves".
This is mainly needed for post locking (not implemented through the API yet). It prevents data loss when more than one client tries to edit the same post. It also keeps the audit trail more consistent.
[...]
Perhaps but there are some edge cases like when there are several autosaves authored by different users that have edited the post at different times. Then it can get really messy :) Thinking that restoring/overwriting a post from a revision should always be user-controlled.
The current logic is that the post author is "the boss" and other users that edit the post leave audit trail with their changes. This is essential for larger sites that have many authors and many editors.
Right, my proposal is that we treat the post author the same in this sense, which would improve the audit trail I think. The question I don't have an answer for is whether this makes the UX or data harder to deal with.
Hm,
wp_save_post_revision()
doesn't wipe autosave revisions. It has some logic to keep a max number of revisions if they are limited by a plugin. The default is-1
(unlimited). Also note that this logic specifically bypasses (keeps) autosave revisions, even when they are over the max.
Ah, you're right, I'd misread the code there.
Also, the post is not an "unrelated resource" in this case.
"Unrelated resource" in the sense of REST resources, not in the sense of WordPress resources; that is, one URL affects another.
#68
in reply to:
↑ 65
@
7 years ago
Replying to azaozz:
Also, the part: "User hits save:
POST /wp/v2/posts/{id}/revisions/{rev_id}
" would be an actual save, i.e.POST /wp/v2/posts/{id}
.
I was thinking this would be a cleaner way of the client actually "committing" the autosaved changes. It avoids the problem where an update to a somewhat-unrelated resource (the post) affects a bunch of others (all the autosaves). This is the current behaviour though:
wp_save_post_revision()
is called bywp_insert_post()
, and it wipes out all the autosaves.
Hm,
wp_save_post_revision()
doesn't wipe autosave revisions. It has some logic to keep a max number of revisions if they are limited by a plugin. The default is-1
(unlimited). Also note that this logic specifically bypasses (keeps) autosave revisions, even when they are over the max.
One other question here: how does the autosave actually get applied to the post?
From what I'm understanding here, it seems like it doesn't ever get applied/committed. Rather, the editor just runs a real save with the full content, which doesn't touch the autosave process at all. When the user makes another change after that, the editor then starts autosaving stuff again. Is that accurate?
#69
in reply to:
↑ 67
;
follow-up:
↓ 72
@
7 years ago
Replying to rmccue:
Hm,
wp_save_post_revision()
doesn't wipe autosave revisions. It has some logic to keep a max number of revisions if they are limited by a plugin. The default is-1
(unlimited). Also note that this logic specifically bypasses (keeps) autosave revisions, even when they are over the max.
Ah, you're right, I'd misread the code there.
The code that actually does this is in edit-form-advanced.php
when the editor checks for autosaves. I guess this adds an extra step to the process too:
- User opens the editor
- Editor checks current autosaves:
GET /wp/v2/posts/{id}/revisions?revision_type=autosave
- If any autosaves exist newer than the last save, display restore button
- If any other autosaves exist, delete them
- Editor creates a new autosave:
POST /wp/v2/posts/{id}/revisions?revision_type=autosave
- Editor checks current autosaves:
#70
@
7 years ago
43316.13.diff adapts 43316.12.diff but uses the existing revisions endpoint instead. This implements almost all of the proposal, minus the autosave "commit" process; I'll update once checking per 69. This is a first pass, and probably needs some work still, but does work in local testing.
#71
in reply to:
↑ 67
@
7 years ago
Replying to rmccue:
Yeah, I'm not sure why this was added to the API. IMHO we shouldn't be removing the functionality of the audit trail. This changes the purpose and functionality of revisions quite a bit...
This was way back in https://github.com/WP-API/WP-API/pull/1110
It does check the delete cap on revisions, so I'd make the argument this should be handled in the capability system instead. This is a conversation for another day though :)
The more I think about this, the worse it looks...
Yes, deleting revisions checks the delete_post
cap, however this is still not adequate. Nobody should be able to circumvent the audit trail, not even admins. This is a safety/security feature. I see this as a blocking regression in the API. The only way this should be possible is from a plugin (same as now for non-API).
If you don't want to remove the delete revision endpoint, we probably can require a delete_revisions
capability that will not be mapped to any existing role and will always return false, i.e. a plugin will have to specifically assign that capability to a role.
Created #43709 as a follow-up.
#72
in reply to:
↑ 69
@
7 years ago
Replying to rmccue:
Hm,
wp_save_post_revision()
doesn't wipe autosave revisions. It has some logic to keep a max number of revisions if they are limited by a plugin. The default is-1
(unlimited). Also note that this logic specifically bypasses (keeps) autosave revisions, even when they are over the max.
Ah, you're right, I'd misread the code there.
The code that actually does this is in
edit-form-advanced.php
when the editor checks for autosaves. I guess this adds an extra step to the process too:
- User opens the editor
- Editor checks current autosaves:
GET /wp/v2/posts/{id}/revisions?revision_type=autosave
- If any autosaves exist newer than the last save, display restore button
- If any other autosaves exist, delete them
- Editor creates a new autosave:
POST /wp/v2/posts/{id}/revisions?revision_type=autosave
Hm, no. The code in edit-form-advanced.php
repeats the same logic as in wp_create_post_autosave()
https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-admin/includes/post.php#L1687. It deletes the latest autosave only when the autosaved "fields" match the latest post. It also doesn't look for previous autosave revisions by other authors.
#73
@
7 years ago
Looking at 43316.13.diff, it introduces quite a few inconsistencies. It kind of treats autosaves as different post_type, but still keeps them as revisions. It also moves all auto-saving logic to the client and requires the client to manipulate the data to be able to save. There are also few regressions when comparing with the current logic.
- The client cannot do
autoSavePost( postContent )
the same way it doessavePost( postContent )
. There are quite a few differences in the data. This is a departure from the post saving logic that may be seen as a regression.- It has to set
parent
to be the same as post_ID. That's somewhat weird as an item cannot be a parent of itself :) That's even worse for pages where there may be an actualpost_parent
which will have to be overwritten with the post_ID... - Actually a client cannot do
autoSavePost( postContent )
. It will have to check whether an autosave exists (an extra trip to the server), then manipulate the post data to match the endpoint requirements. It seems the client will have to always do two requests to be able to auto-save.
- It has to set
- Autosaves on draft posts are explicitly forbidden. This is a regression. It means that:
- We will not be able to ever do server-side post locking, as we will loose any data a second client tries to autosave.
- When two clients try to edit the same draft, they will automatically stomp on each other's changes (without knowing) and there will be no server backup. That's not acceptable imho.
There also seem to be few bugs (and typos) in update_item() and update_item_permissions_check() but before looking at them I'd like to look at the proposed functionality from a clien't point of view.
- The difference between saving and auto-saving is quite large.
- Auto-saving seems needlessly complex and that's not warranted.
- The server logic is completely overridden/ignored and the (untrusted) client has access to making some low-level decision which seems to be a regression.
#74
@
7 years ago
One other question here: how does the autosave actually get applied to the post?
When the post edit screen loads, if an autosave is present and belongs to the current user and is newer than and different from the current save, a notice is displayed at the top of the screen informing the user that a newer autosave is available, with a link to "View the autosave".
Clicking the link takes the user into the revisions interface, viewing the autosave. Clicking 'Restore This Autosave' restores this autosave as the post content.
Aside: if the author's autosave is older than the currently saved post when the post edit screen loads, it is deleted.
#75
follow-up:
↓ 76
@
7 years ago
Some good points have been made here and its time to take stock of all the feedback and move forward! The lack of any autosave capability is a blocker for merging Gutenberg because without it autosaves are essentially broken.
My intuition and initial idea was a separate Autosave controller. Still, I liked the idea of adding autosave capability to the revisions controller (as @rmccue suggested in 43316.13.diff) because of their technical similarity, or even adding to the posts controller with a flag as originally proposed in 43316.patch by @azaozz...
However from the client's perspective these actually seem like pretty different concepts. As @schlessera keeps pointing out, its the difference in the concepts of autosave vs. revision vs post that suggests a separate autosave endpoint is appropriate. The fact that technically autosaves are very similar to revisions and even posts is an implementation detail that doesn't concern clients.
To those on this thread advocating alternate approaches, can you agree to moving ahead with a separate Autosaves controller (latest in 43316.12.diff) and if not, what blocking concerns remain?
#76
in reply to:
↑ 75
@
7 years ago
Replying to adamsilverstein:
To those on this thread advocating alternate approaches, can you agree to moving ahead with a separate Autosaves controller (latest in 43316.12.diff) and if not, what blocking concerns remain?
Agreed, lets move on with 12.diff :)
In 43316.14.diff:
#77
@
7 years ago
Sounds good to me, I'm working on a PR leveraging this controller to support autosaves in Gutenberg, based on the previous PR. @danielbachhuber suggested we get this merged there first as a way of testing/proving the approach. I should have something complete in the next few days.
This ticket was mentioned in Slack in #core-editor by adamsilverstein. View the logs.
7 years ago
#79
@
7 years ago
@azaozz I incorporated your latest changes into the autsaves controller in this PR: https://github.com/WordPress/gutenberg/pull/6257
#80
follow-up:
↓ 81
@
7 years ago
I did a read of the GitHub pull request and want to cross-post some nitpicks about 43316.14.diff:
- Why do we need to instantiate a
$this->revision_controller
instance if we're subclassing the revisions controller in the first place? - Re:
create_item_permissions_check()
, I'd be concerned about a point in the future where$this->parent_controller->update_item_permissions_check()
changed and caused an unexpected change in behavior in this controller. If we want to create an explicit internal API for this, we should do so; otherwise, we should copy the contents into this method. - Re:
create_item()
, we should change theid
argument intopost
or something else.id
is always a read-only value generated by the database elsewhere in the REST API.
#81
in reply to:
↑ 80
;
follow-up:
↓ 82
@
7 years ago
Replying to danielbachhuber:
- Re:
create_item_permissions_check()
, I'd be concerned about a point in the future where$this->parent_controller->update_item_permissions_check()
changed and caused an unexpected change in behavior in this controller. If we want to create an explicit internal API for this, we should do so; otherwise, we should copy the contents into this method.
Hmm, possibly but the opposite is also true. An autosave revision is always in a "post update" context and needs the same permissions. If we are to copy/duplicate the functions we most likely will end up having to update them in tandem with the same functions in the posts controller.
Thinking that we can keep using the posts cap check for now. If it needs changes and these changes are not needed for the autosaves, can copy the functions then.
- Re:
create_item()
, we should change theid
argument intopost
or something else.id
is always a read-only value generated by the database elsewhere in the REST API.
Which id
argument do you have in mind? The parent post ID in /wp/v2/posts/{id}/autosaves
? This seems to be the standard way of creating a sub-item in REST. The same ID has to be set in the $request (same as when updating a post). IMHO that makes sense when dealing with a sub-item.
#82
in reply to:
↑ 81
;
follow-up:
↓ 83
@
7 years ago
Replying to azaozz:
Hmm, possibly but the opposite is also true. An autosave revision is always in a "post update" context and needs the same permissions. If we are to copy/duplicate the functions we most likely will end up having to update them in tandem with the same functions in the posts controller.
Thinking that we can keep using the posts cap check for now. If it needs changes and these changes are not needed for the autosaves, can copy the functions then.
The current implementation is poorly coupled and confusing to follow. But, given we can't use PHP traits, I don't have a reasonable alternative to suggest at this point.
On a related note, if WP_REST_Autosaves_Controller
subclasses WP_REST_Revisions_Controller
, is there a reason we're instantiating a new object instead of calling the parent methods directly?
Which
id
argument do you have in mind? The parent post ID in/wp/v2/posts/{id}/autosaves
? This seems to be the standard way of creating a sub-item in REST. The same ID has to be set in the $request (same as when updating a post). IMHO that makes sense when dealing with a sub-item.
In a POST wp/v2/posts/123/autosaves
request, the 123
value should be referenced as parent
, not id
. The route is defined as /wp/v2/posts/{parent}/autosaves
.
Based on the current test_update_item()
, it appears id
is simply a duplication of the parent
value already present in the request. Furthermore, the id
returned is always different than the id
passed. This is incorrect from a technical perspective.
#83
in reply to:
↑ 82
;
follow-up:
↓ 84
@
7 years ago
Replying to danielbachhuber:
On a related note, if
WP_REST_Autosaves_Controller
subclassesWP_REST_Revisions_Controller
, is there a reason we're instantiating a new object instead of calling the parent methods directly?
Think this has been there since the first patch, perhaps @adamsilverstein would know more. I have no preference either way.
In a
POST wp/v2/posts/123/autosaves
request, the123
value should be referenced asparent
, notid
. The route is defined as/wp/v2/posts/{parent}/autosaves
.
Based on the current
test_update_item()
, it appearsid
is simply a duplication of theparent
value already present in the request. Furthermore, theid
returned is always different than theid
passed. This is incorrect from a technical perspective.
Right. The problem here is that (post_)parent
is a property of WP_Post and the API shouldn't be overwriting it, especially for pages
. Taht's a UI setting for them. It seems this is another bug in the API. How would that work when a draft page with a (real) post_parent is autosaved and the actual page row is updated in the db? Would the page's parent be overwritten?
#84
in reply to:
↑ 83
@
7 years ago
Replying to azaozz:
Right. The problem here is that
(post_)parent
is a property of WP_Post and the API shouldn't be overwriting it, especially forpages
. Taht's a UI setting for them. It seems this is another bug in the API. How would that work when a draft page with a (real) post_parent is autosaved and the actual page row is updated in the db? Would the page's parent be overwritten?
Oh, I see what you've run into. In this particular case, we should be more protective about the request data we're passing through. I'd consider what you've described to be a bug.
#85
@
7 years ago
On a related note, if WP_REST_Autosaves_Controller subclasses WP_REST_Revisions_Controller, is there a reason we're instantiating a new object instead of calling the parent methods directly?
I think this is required to get the revisions controller set up properly, although open to removing if thats possible... we leverage the instance for permissions checks to avoid duplicating them. Without instantiating, these checks didn’t work.
#86
@
7 years ago
@danielbachhuber, @adamsilverstein is there anything still stopping this from being committed? Looks like it's working as intended. Shall we add it?
#87
@
7 years ago
@azaozz I'd like to have it land in Gutenberg and soak for a couple releases first.
#88
@
6 years ago
In the latest patch: When creating an autosave, we allow a post with auto-draft
status to be saved in-place. Should it ever be the case that we'd want an auto-draft post to stay as auto-draft after an autosave occurs? Or should we at least change this status to draft
from the server? Otherwise it requires that the developer include status
in the payload to bump the post to draft status. This is more a workflow expectation, and admittedly it seems we should want to limit the amount of "magic" happening on the server, but in this instance we're already varying logic automatically based on the status of the post being saved.
#89
@
6 years ago
Adding to comment:88 , it is not immediately obvious what fields this endpoint is capable of receiving, and in fact it varies by post status / authorship.
For auto-drafts / drafts authored by the current user, the following fields are supported:
- id
- date
- date_gmt
- slug
- status
- password
- title
- content
- author
- excerpt
- comment_status
- ping_status
Notably this excludes: featured_media, format, meta, sticky, template, categories, tags
This was determined in the autosaves controller's use of WP_REST_Posts_Controller::prepare_item_for_database
, which assigns only these fields.
On the other hand, in all other circumstances only the following fields are supported:
- id
- title
- content
- excerpt
This was determined in the use of _wp_post_revision_fields
to intersect fields for _wp_put_post_revision
.
Given then the need to pass status
with the payload as described in comment:88 , it is especially confusing since it is only respected for some, but not all requests (only those which are auto-draft/draft by current user).
#90
in reply to:
↑ 62
;
follow-up:
↓ 91
@
6 years ago
Replying to azaozz:
Auto-save:
[...]
- When creating a revision following the above logic, if an autosaved revision already exists, it is updated instead of creating another autosaved revision. Additionally if the post content is identical to the new content that is being auto-saved, the previous autosave revision is deleted to prevent having a redundant revision (i.e. if the update of the previous autosaved revision will make it identical to the current post, there is no need of it).
With regards to this and relating to the most recent patch version, I'm confused by the proposed flow where an error is returned from the endpoint.
- Do we really need to return an error? Can't we just treat it as though it were successful even if in-fact the server is ensuring there are no redundant revisions? Otherwise this can result in challenges from the client in ensuring to not send fields which are already the saved value for the autosave.
- Because we delete the revision and then return immediately with an error, isn't it true that we're not simply not storing redundant revision, we're in fact destroying a revision without having anything new to serve in its place?
#91
in reply to:
↑ 90
@
6 years ago
Replying to aduth:
...I'm confused by the proposed flow where an error is returned from the endpoint (when deleting identical autosaves).
- Do we really need to return an error? Can't we just treat it as though it were successful even if in-fact the server is ensuring there are no redundant revisions?
Yeah, generally the autosave logic is pretty complex. I'd be happy if we can streamline it a bit (for the API), if possible.
Deleting an autosave revision when the content is identical to the post_content is done for two reasons:
- It ensures there aren't any revisions with no diff/without any changes.
- It reduces the number of revisions.
Don't think the second reason matters much, but the first is confusing when viewing revisions, so this behaviour makes sense.
- Because we delete the revision and then return immediately with an error, isn't it true that we're not simply not storing redundant revision, we're in fact destroying a revision without having anything new to serve in its place?
Yes, technically there is no reason to delete the old autosave revision. Even then, what should be returned for such requests? Seems we have to return the "old" autosave ID? Then the autosave revision content will be different from the submitted content.
Another option is to standardize on always returning the post_ID of the actual post being edited. Currently this is the case for drafts (same user) when the post is updated, but the returned ID is the autosave revision ID when draft for different user or published post. Thinking it will probably be best to always return the "real" post_ID for all autosave requests.
#92
@
6 years ago
As of the most recent patch version, there are errors which are returned from the controller which will issue a 5xx status code, where in fact there are better alternatives available.
- The
rest_autosave_no_changes
error assigns no status code. This has been remedied in Gutenberg's implementation to assign a400
status code. See GB#7347. - When
wp_update_post
returns an error (e.g. "Content, title, and excerpt are empty"), it is returned verbatim. At least in this specific example, a 400 status code would be more appropriate. Gutenberg does not handle this well either as of yet.
This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.
6 years ago
#96
@
6 years ago
@adamsilverstein 43316.15.diff looks good for me, except one nit: I see a couple of error_log()
in the diff.
#97
@
6 years ago
@danielbachhuber thanks, posted that mid testing, apologies, I'll clear those out. Checking with Gutenberg disabled/enabled to make sure the initialization checks are correct.
#98
@
6 years ago
- move the conditional to file load
- only register autosaves against endpoints that support revisions, verified autosaves endpoints still loaded when
WP_POST_REVISIONS
is false. Note that gutenberg currently registers autosaves for all post types - do we want media autosaves for example? See https://github.com/WordPress/gutenberg/blob/master/lib/rest-api.php#L36-L47 - maybe when under core 5 gutenberg can skip adding the routes, as is we are double adding them
#99
@
6 years ago
@adamsilverstein Couple of points:
- The class should always be loaded by
wp-settings.php
, not conditionally. The class isn't meant to be overridden in that manner. - Autosaves are expected to work for a Post Type with Revisions disabled. Good point re: media though; we can skip registering for
'attachment' === $post_type
#100
@
6 years ago
- Ok, I think I was trying to avoid a potential conflict with Gutenberg, I guess that is the plugin's job. Will remove.
- Yea, I can skip that explicitly. I noticed that under Gutenberg blocks have autosaves, is that right?
#102
@
6 years ago
43316.18.diff only register autosaves for post type if 'attachment' !== $post_type->name
#104
@
6 years ago
Hi, I skimmed through the latest patch and noticed the 'gutenberg'
domain for the translated texts, e.g.
__( 'Invalid item ID.', 'gutenberg' )
I'm just curious if that's special for the Gutenberg merge?
ps: it would be nice to have inline docs for the test methods, it's late here so I could only help with that tomorrow if needed.
#105
@
6 years ago
Ah, good catch @birgire I'll clean that up. Also seeing test failures. locally all tests pass if i run the tests in isolation, but revisions tests fail after running the autosave tests - seems like some cleanup/tearDown is not working correctly. One issue was the order the route was added had changed from the tests, 43316.19.diff fixes that other tests are still failing: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/442418410
Appreciate any assistance resolving that!
#106
@
6 years ago
I wonder if it's this part:
if ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) { return; }
in the wp_save_post_revision()
could be the reason for the revisions not being created in WP_Test_REST_Revisions_Controller::setUp()
,
Could it be affected by:
if ( ! defined( 'DOING_AUTOSAVE' ) ) { define( 'DOING_AUTOSAVE', true ); }
in WP_REST_Autosaves_Controller::create_item()
?
Maybe this could explain why WP_Test_REST_Autosaves_Controller
and WP_Test_REST_Revisions_Controller
don't play nice together?
#107
@
6 years ago
The WP_Test_REST_Autosaves_Controller
runs before WP_Test_REST_Revisions_Controller
when the restapi
group is tested.
I did verify that changing the execution order, by changing the filename from rest-autosaves-controller.php
to rest-zautosaves-controller.php
, will run the tests successfully.
We could try to filter DOING_AUTOSAVE
out from the test methods, but that would need some extra changes to core functions.
There is no filterable wp_doing_autosave()
that we could use in wp_save_post_revision()
.
So I searched for any open tickets for this, and found #44786 and #43760.
So if we want to use the filter approach, to run the tests successfully, then this ticket would be blocked by those tickets.
#108
@
6 years ago
The WP_Test_REST_Posts_Controller::test_get_item_links_predecessor()
is also affected by this DOING_AUTOSAVE
issue, since it's also stopping the revisions creation within.
#109
@
6 years ago
@birgire thanks for spending some time digging into this. I also worked on this last night and can confirm everything you said. The constant issue is very interesting and may in fact be related. I'm going to try making sure that is reset between testing runs.
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
6 years ago
#111
@
6 years ago
- Avoid setting the DOING_AUTOSAVE constant, it stays set as the test suite executes and was breaking other tests.
- As far as I can tell, this is not required, feedback?
Tests pass now locally, Travis run in progress: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/442999757
#113
in reply to:
↑ 112
@
6 years ago
Replying to danielbachhuber:
As far as I can tell, this is not required
Don't we need to keep
DOING_AUTOSAVE
?
We do. Or we are back at the beginning with revisions created for each draft save. Apart from that it may bring other regressions in plugins.
This is a problem with testing, not with the actual functionality and should be fixed in the tests :)
A possible (easy) solution would be to run these tests last, after all of the other revisions tests. Another is to just do define( 'DOING_AUTOSAVE', false )
when doing the tests, although that may bring other testing issues down the line and would need proper documentation.
#114
@
6 years ago
Some have set up a <testsuite><file>file1</file><file>file2</file>...</testsuite>
to control the file execution order, See e.g. https://stackoverflow.com/a/45323854/2078474 But then I assume the restapi tests would have to be separated from the default core tests in similar way as the multisite tests that uses it's own tests/phpunit/multisite.xml
config. That config file would then need to be updated for each new restapi class?
Some "is-it-friday-yet" filenames come to mind regarding alphabet filename ordering but I hope we are not going down that path :-)
Then there's the DOING_AUTOSAVE
filter approach.
Maybe other possible approches?
#115
@
6 years ago
@azaozz - Thanks for reviewing, I had a feeling we needed to leave that. I was surprised tests all passed - seems like we should test for the condition that not having that set breaks.
One other idea was to unset the constant at the end of the create_item
method, however we would need to be sure runkit_constant_remove
was available.
Then there's the DOING_AUTOSAVE filter approach.
This would be ideal because it seems wrong that calling create_item
sets a constant and leaves it that way - this assumes it is running in isolation when in fact that may not be the case.
We do. Or we are back at the beginning with revisions created for each draft save.
As far as I can tell that is because of this check: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/revision.php#L111-L114. Wrapping that in a filter would we could add and remove in this callback would be ideal, thoughts?
Another is to just do define( 'DOING_AUTOSAVE', false )
maybe for now we can set it this way for tests, and use runkit_constant_redefine
to set it to true for the autosave tests
#116
@
6 years ago
43316.21.diff uses a filter instead of setting the DOING_AUTOSAVE constant
#117
@
6 years ago
43316.21.diff runs successfully* on my install with the filtering in place.
*For some reason the patch doesn't apply cleanly though on my install, I had to adjust the some files by hand.
I noticed a small typo in $doung_autosave --> $doing_autosave
here:
@param bool $doung_autosave Is method called as part of an autosave?
in wp_save_post_revision()
.
If this approach will be the workaround here, then it seems like a wp_doing_autosave()
function would be useful to filter out other cases of DOING_AUTOSAVE
, for consistency ?
This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.
6 years ago
#119
@
6 years ago
For some reason the patch doesn't apply cleanly though on my install, I had to adjust the some files by hand.
Possibly because I am branching from/diffing against the 5.0 branch, not from master?
would be useful to filter out other cases of DOING_AUTOSAVE, for consistency ?
I think this is actually the only place its used, but i'll double check
#120
@
6 years ago
Looking at 43316.21.diff, adding a filter for DOING_AUTOSAVE
fixes testing but introduces possible regressions in plugins and themes. A quick search on GH shows 350k hits for "DOING_AUTOSAVE" (need to be logged in there), a lot more than what I'd expect. It's also used in TwentySixteen and TwentySeventeen. In that terms I don't think a filter is a good option.
Other possible solution would be to define DOING_AUTOSAVE
conditionally, only when not running PHP tests. We already have WP_RUN_CORE_TESTS
for this (set in phpunit.xml.dist). Or even better: we can have a helper function to determine if tests are running, similarly to is_admin()
, etc.
In any case, we shouldn't add any changes that may introduce regressions, and not defining DOING_AUTOSAVE
when in fact we are doing autosave seems to be one.
#121
@
6 years ago
In 43316.22.diff
, I've made the following changes:
- Removed instances of
'gutenberg'
textdomain. - Used
$this->get_fields_for_response()
to define the fields in response. See #43874. - Assigned extra object fields with
$this->add_additional_fields_to_object()
. - Ported over updated tests from Gutenberg.
- Restored use of
DOING_AUTOSAVE
constant and remove thewp_doing_autosave
filter. The test suite issue is addressed by runningrest-autosaves-controller.php
in a late test suite.
On the last point, we can always improve upon the test suite implementation later. I think keeping DOING_AUTOSAVE
is a priority though.
#125
@
6 years ago
- Keywords fixed-5.0 added; commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#127
@
6 years ago
- Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests fixed-5.0 removed
The autosaves controller currently accepts any fields for autosave updates.
After https://github.com/WordPress/gutenberg/pull/11513 is sorted out, we'll need another patch with unit tests.
#128
@
6 years ago
So in https://github.com/WordPress/gutenberg/issues/10753, it was reported that the post parent was being lost during an autosave. In the PR, https://github.com/WordPress/gutenberg/pull/11513, @aduth solved this by manually unsetting the post_parent
after the posts controller had set it from the request data. This was also accompanied by a change in the JS code to not send the parent
( the ID of the post being autosaved ) to the endpoint as well.
Looking at that change one might think that the accompanying PHP change would be no longer necessary since the parent was no longer being sent with the JS code. That's what Daniel and I thought. And looking at the autosave controller, came to the conclusion that all fields were mistakenly being allowed to be updated and the filtering in _wp_post_revision_fields
should be applied. However, after further investigation, that behavior is intentional and any wp_insert_post
compatible field will be updated when the post is a draft and the current user is the post_author
of that post.
If the parent
property is no longer being sent in the request data, then how is the post parent getting changed? For that we can look at the route definition for the autosaves route.
'/' . $this->parent_base . '/(?P<parent>[\d]+)/' . $this->rest_base
Here we find the source of the mysterious parent field. parent
here is indicating the ID of the post being autosaved. The WP REST Request object allows you to access any parameter in the request, a JSON param, post data, query param and even indeed URL parameters by using WP_REST_Request::get_param()
. So when the request object was sent to the posts controller to format it for the database, it saw that indeed the parent
parameter was in the request, and changed the parent ID to be the post itself! https://github.com/WordPress/wordpress-develop/blob/5.0/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1069
When WordPress inserts a post object, it applies a filter on the parent ID to check that it is valid. The post_parent
being equal to the ID
is not valid, so WordPress changes the post_parent
value to 0
. And that is how the post_parent
ends up lost.
Are we back to unsetting the post_parent
manually then? No, because again, the Classic Editor supports auto-saving the post parent and changing it to a different value. So if we unset the post parent, we wouldn't be able to autosave any changes to it.
TLDR: Instead, I believe we should change the route definition to use id
as the field name in the URL for the collection and creation route.
I believe parent
was chosen to match with WP_REST_Revisions_Controller
. But the only reason the revisions controller hasn't run into this issue is that it has no write endpoints. Only read and delete. The autosaves controller reuses the revisions controller which checks for a parent
request object. I think we should instead have our own version of get_items_permission_check()
. Alternately, we'd need to change the URL parameter in the revisions controller which I don't think is feasible for BC reasons.
Side note. I really think we should stop subclassing the revisions controller and instead subclass the WP_REST_Controller
. We are already instantiating an instance of the revisions controller. The only time I could see the subclass being used was to leverage access to the WP_REST_Revisions_Controller::get_parent()
through the $revisions_controller
property because that method is protected
. We can instead inline that definition or make the method public. Additionally, by subclassing like this we are advertising that this endpoint supports a host of collection parameters that we don't.
#129
@
6 years ago
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
Attached a patch to change the URL parameter to id
from parent
in the get_items/create_item
route. Also changes the collection parameters to indicate that only context is supported and ensures that the schema of the parent controller is used for validation.
#132
@
6 years ago
Thanks for the excellent sleuthing and fixes here @TimothyBlynJacobs!
Side note. I really think we should stop subclassing the revisions controller and instead subclass the WP_REST_Controller.
Good point. I had a feeling that might be better - thanks for explaining some reasons why. I think originally the controller for autosaves was closer to revisions, reusing more of the parent class.
Please note that I explored the alternative of creating an Autosave REST controller (by extending the revisions controller) in https://github.com/WordPress/gutenberg/pull/4156. While it may be worth adding, I'm not sure its the best approach to use in the editor for autosaves.
The current approach uses the
heartbeat api
, which technically uses admin-ajax. Heartbeat is a proven/established API and offers some possible advantages over a REST API endpoint some of which are mentioned here: https://github.com/WordPress/gutenberg/pull/4218#issuecomment-364186696