`wp_insert_post` doesn't save revision for new published post

[JamesDiGioia]

Calling wp_insert_post with post_status of publish or any other status on a new post doesn't save a new revision.

The reason for this is the $update check at the beginning:

https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/post.php#L3085

checks whether or not the args include an ID, indicating that we're updating a post. Further down, that update check is what calls post_updated

https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/post.php#L3473

which wp_save_post_revision is hooked to. Therefore, if we call wp_insert_post in a plugin to insert a post with apost_status of publish, WordPress doesn't save a post revision.

Post revisions are supposed to be up-to-date with the latest post data, but we don't get a revision if we insert a post in this manner. I think the reason we ended up with this is to avoid saving a revision of the auto-draft that is created when opening up post-new.php, but that isn't necessary now, as wp_save_post_revision checks whether the post_status is an auto-draft.

I think the easiest solution is to move the wp_save_post_revision to the save_post hook, but I'm not sure if there are other places in the codebase this would cause issues.

[adamsilverstein]

Hey JamesDiGioia!

Thanks for the bug report. Oh thats a problem, I can see why you aren't getting an initial revision for your posts! We didn't think about posts created this way when creating the original post revision.

I'm not sure moving save_post is good, I think that action fires twice on publish. We could try the change (want to try writing a patch for that?) and run our tests, I'd still worry about unintended consequences. Open to other ideas...

In the mean time, a workaround worth trying would be:

• use wp_insert_post to create an initial post and get the the post ID
• use wp_update_post to insert your post data

[JamesDiGioia]

The only other thing I can think of is to change the $update variable to be true if it either has an ID or the post_status !== auto-draft. Or if it only == 'publish'. Does that make more sense?

I ended up working around it without an issue in my own plugin, but I think it's still a bug.

[georgestephanis]

#39011 was marked as a duplicate.

[georgestephanis]

Unit test attached, demonstrating the issue.

This affects Core as of 4.7-RC1 with Custom CSS losing its first revision.

[adamsilverstein]

Replying to georgestephanis:

Unit test attached, demonstrating the issue.

Thanks for the unit test @georgestephanis, thats a great start towards solving this issue.

[adamsilverstein]

Still digging into this issue, however I posted test_wp_first_revision_is_not_lost.2.diff​ to demonstrate this workaround by calling wp_update_post immediately after creating the post, passing the returned post object, the issue is resolved. I think this actually mirrors what happens when you create a post in the admin.

[adamsilverstein]

In 30854.diff​:

• Hook wp_save_post_revision on to new_to_publish in addition to update_post - triggering a revision save when a new post is created
• Adjust revisions unit test to accommodate change: bump expected revision counts by 1, add first revision to order by comparisons

This needs more testing, especially around creating posts in the admin interface and verifying that we aren't getting duplicate initial revisions.

[westonruter]

In 39477:

Customize: Ensure a custom_css post insertion gets an initial post revision.

Props georgestephanis, westonruter.
See #30854, #38672, #35395.
Fixes #39032.

[hellofromTonya]

Replying to adamsilverstein:

In 30854.diff​:

• Hook wp_save_post_revision on to new_to_publish in addition to update_post - triggering a revision save when a new post is created
• Adjust revisions unit test to accommodate change: bump expected revision counts by 1, add first revision to order by comparisons

This needs more testing, especially around creating posts in the admin interface and verifying that we aren't getting duplicate initial revisions.

Hey @adamsilverstein, Can you provide a testing strategy please to give guidance for manual testers to do the testing and provide needed feedback?

[Mista-Flo]

I have uploaded two patches.

The first one is just a refresh of the last patch.
The second one also remove the special workaround introduced in 39477

[Mista-Flo]

Refresh

[Mista-Flo]

Clean

[adamsilverstein]

Excellent, thank you @Mista-Flo!

@hellofromTonya testing by hand is tricky from the UI because this ticket deals with a programmatic insertion of a post rather than one created in the UI.

One possible test would to try importing some posts using the WordPress importer. After this patch, imported posts should get an initial revision, better matching posts created in the UI (and something we worked around recently in the revision screen). You will have to look in the DB to see the difference though because we "fake" the initial revision on the revisions screen when it is missing.

A second test would be to review the bug fixed in https://core.trac.wordpress.org/ticket/39032. The fix for that is now reverted in the latest patch, because the new fix here corrects the issue.

[Mista-Flo]

Replying to adamsilverstein:

Excellent, thank you @Mista-Flo!

@hellofromTonya testing by hand is tricky from the UI because this ticket deals with a programmatic insertion of a post rather than one created in the UI.

One possible test would to try importing some posts using the WordPress importer. After this patch, imported posts should get an initial revision, better matching posts created in the UI (and something we worked around recently in the revision screen). You will have to look in the DB to see the difference though because we "fake" the initial revision on the revisions screen when it is missing.

A second test would be to review the bug fixed in https://core.trac.wordpress.org/ticket/39032. The fix for that is now reverted in the latest patch, because the new fix here corrects the issue.

btw, for the removal of #39032 I only removed it from the code, but I did not remove the unit test of that ticket, should we do it as well?

[Clorith]

I had a look at 30854.3.diff​, which appears to add a post revision when a post gets the status publish`.

Cross referencing a bbPress ticket here (​https://bbpress.trac.wordpress.org/ticket/3389), which is a big scenario which needs to account for existing content as well.

The current behavior is that the first edit of an entry in a custom post type overwrites the initial post content entirely in the revision history, and then creates an autosave, completely removing any knowledge of the very first post made.

By just adding a filter to when a post gets the publish status, would this not be limiting the fix to new posts, not existing published ones?

[adamsilverstein]

By just adding a filter to when a post gets the publish status, would this not be limiting the fix to new posts, not existing published ones?

Right, this would only solve the case for newly (programmatically) published posts.

Cross referencing a bbPress ticket here (​https://bbpress.trac.wordpress.org/ticket/3389), which is a big scenario which needs to account for existing content as well.

The current behavior is that the first edit of an entry in a custom post type overwrites the initial post content entirely in the revision history, and then creates an autosave, completely removing any knowledge of the very first post made.

Can you test this patch with bbPress to see if it fixes the issue you linked?

Trac ticket: https://core.trac.wordpress.org/ticket/30854

[adamsilverstein]

Hey @Clorith can you please review the latest patch to confirm it fixes the bbPress issue you linked?

Running tests on this PR: ​https://github.com/WordPress/wordpress-develop/pull/1336

[Clorith]

Applied patch from ​PR 1336 and it does create appropriate revisions for _new_ topics that area created, but as indicated in my initial reply, it does not account for _existing_ posts, which is part of the bigger issue here.

Since revisions are enabled per post type with the supports section of register_post_type, I would expect the fix here to work for existing and new content alike when revisions is added as a supported feature of the given post type.

--

For anyone else looking to work on, and test this, the steps to test it (may be of interest to @hellofromTonya and the testing efforts she's heading up):

To start off, you need a test site with the ​bbPress plugin installed, using it for testing, since it has an interface, and is part of the WordPress project.

Some additional information, this bug is only applicable when _not_ using the post editor in the wp-admin back-end, this is why using bbPress is convenient, since it calls the functions from the front-end and as such is able to replicate the bug reliably from a UI point of view as well.

Create a Forum, since we need a place to create the topics.

Throughout testing I will be using primarily two texts:

Original content:

• Title: Test topic #N (increasing the number with each test I make)
• Post content: Post with initial content

Edited content:

• Title: (no change)
• Post content: Post with modified content

Now visit the front-end of your forum, create a new topic with the Original content from above.

After editing, click the Edit link in the forum topic, and replace your content with the Edited content from above.

If you visit the back-end and look in the Topics post type, you should see your new topic, but if you open it for editing in the back-end, no revisions section will show up, as there are no revisions yet.

In the front-end, edit the post a third time, this time enter any text you like, at this point it does not matter what is used, as we have our two baseline texts above.

Visit the back-end again and look at your topic, this time you will see revisions show up, but if you click in to look at them, you will see that there is one revision less than the total amount of edits and the original creation combined, and if you browse the revisions, you will not find the Post with initial content string, as the entry with Post with modified content is considered the first post.

After the patch is applied, if you complete the same steps above, you should see the correct amount of revisions (the total amount of edits you made plus one for the initial topic), and you should also see that the very first revision is the Post with initial content text.

[Clorith]

I should mention that for testing it for existing content, I created a new topic in the front-end first, then I applied the patch and performed an edit.

The expected scenario is that my content from the Original content should then show up in the revisions after the patch is applied, even if my topic was created without the patch.

The actual result at this time is the same as if the patch had not been applied, and the Original content is lost upon editing.

[hellofromTonya]

Thanks for pinging me @Clorith. Adding this one to the testing queue. Marking it as needs-testing. Will also run through the unit tests.

Sorry this didn't get done before today as today. As today 5.8 Beta 1 day and there's testing to is 5.8 Beta 1. Ran out of time for it to land in 5.8. Punting to 5.9 (instead of Future Release) as it's very close to landing.

[adamsilverstein]

Applied patch from PR 1336 and it does create appropriate revisions for _new_ topics that area created, but as indicated in my initial reply, it does not account for _existing_ posts, which is part of the bigger issue here.

@Clorith - Ah, I think I see what you are saying, although it does feel like a bit of an edge case.

You might be able to fix this in bbPress with some existing hooks, eg copy the content to a revision on that first update if you know it is missing because a new topic is used?

The current patch still addresses the bug as originally reported by @JamesDiGioia:

wp_insert_post doesn't save revision for new published post

So let's get this fix in as is, it solves a long standing issue with this "original revision" missing.

[adamsilverstein]

30854.4.diff​ is a refresh against trunk.

​https://github.com/WordPress/wordpress-develop/pull/1336 is also updated (and tests running).

[adamsilverstein]

Marking this for commit; also, marking as needs-dev-note. We should inform developers about this change, and we can also provide a way they can opt out if they need to for some reason. namely:
remove_action( 'new_to_publish', 'wp_save_post_revision', 10, 1 );

[adamsilverstein]

30854.6.diff​

• Address some linting errors.
• Add a doc block explaining the source of the new_to_publish action.

[adamsilverstein]

30854.9.diff​ fixes some failing tests after this change.

[peterwilsoncc]

At the moment, revisions can be disable with remove_action( 'post_updated', 'wp_save_post_revision', 10, 1 );, with this change this will no longer be effective.

The BC change might be fine but I am not keen to make the decision under the pressure of feature freeze in an hour or two.

I've moved this to the next release (actually future release because the 6.0 milestone doesn't exit) but let's keep the discussion going to enable a considered, less pressured decision.

[adamsilverstein]

@peterwilsoncc this change is mainly to fix a bug for posts created programmatically, for example with a CLI script.

This is a bug fix, not a feature so... we can continue to consider it for 5.9, right?

At the moment, revisions can be disable with remove_action( 'post_updated', 'wp_save_post_revision', 10, 1 );, with this change this will no longer be effective.

Do we recommend that approach in our handbook, or that common practice in the wild?

To disable revisions entirely, I would recommend:

add_filter( 'wp_revisions_to_keep', '__return_zero' );

Or set the WP_POST_REVISIONS constant to 0.

This approach will continue to work after this change, we can document that in the dev note.

[peterwilsoncc]

Replying to adamsilverstein:

This is a bug fix, not a feature so... we can continue to consider it for 5.9, right?

Yes, sorry, I have been looking at enhancements all morning.

[adamsilverstein]

@peterwilsoncc do you have additional questions about this change? Do you have the time to give it a test? You shouldn't see any change in the current behavior for revisions when using the WordPress UI. Eg, there shouldn't be any new/additional crated revisions vs. before the patch.

The main thing this would effect is plugins that create new content, for example a post duplication plugin, or a distribution plugin, or restoring content from an XML export.

In these cases (unless the plugin manually created a revision), users would not have to save their content twice before they could access the revision screen, and there would be no revisions copy of the original content state. After this patch, these posts will all have a revisions versions that matches the original content, and you will see the revisions screen link after a single update.

I don't think we ever fixed this for imports, so one way to test this (cc: @hellofromTonya ) would be to use the export feature to export some posts from a site, then import them into a site and edit a published imported post. Try removing some content and updating. You will find that you can't use revisions to go back to the original state before you removed the content. if you make additional changes and save again, revisions will appear, but the original state will be missing entirely.

This seems like a worthwhile fix for a longstanding issue that continues to effect users. I'd still like to get it in for 5.9.

[peterwilsoncc]

@adamsilverstein Yeah, I think it's fine. I'll action the commit in the next few hours.

I saw some examples of the remove_action on support sites but couldn't find anything in the plugin repo so it should be fine.

In the dev note, let's provide a specific code example for sites that have moved revision saving to wp_after_insert_post so they can include meta data in revisions and avoid off-by-one errors in the rest api.

@adamsilverstein I'll push a few things to your branch for a conflict resolution and some minor nitpick fixes.

[peterwilsoncc]

Since the last patch, there were a few more tests to update following the introduction of wp_get_post_revisions_url() in [52095]. I've pushed some fixes to @adamsilverstein's existing branch.

Something that occurred to me while doing so is that the existing patch only accounts for the post status publish, whereas there are other post statuses that are in a similar state that could require a revision:

• private
• future
• pending and family
• custom post statuses

I'm worried that if the new_to_publish hook is used now, in the near future Core will need to switch to another hook/add additional new_to_* default actions.

To account for the custom post statuses, whether a revision is saved upon a transition from new might best be included as property within register_post_status(). (Props @TimothyBlynJacobs for this suggestion.)

For now, I'm removing the certainty of the commit keyword so some further consideration can be given to the ticket.

[TimothyBlynJacobs]

Perhaps something like create_revision_for_new_posts or create_initial_revision?

It also looks like there are a couple of REST API tests that will need to be updated. The only one that might be a functionality change we are concerned about is WP_Test_REST_Revisions_Controller::test_get_item. It seems like the revision no longer has author information?

[adamsilverstein]

It also looks like there are a couple of REST API tests that will need to be updated. The only one that might be a functionality change we are concerned about is WP_Test_REST_Revisions_Controller::test_get_item.

Do you see these tests failing?

It seems like the revision no longer has author information?

Interesting, I would expect that to come from the saved post, but perhaps it comes from current_user which might not be set for something like an import. This might be ok in that case. How does it show in the UI? Any issues when restoring the revision (I wouldn't think so because the author would become the current user)?

To account for the custom post statuses, whether a revision is saved upon a transition from new might best be included as property within register_post_status()

To be clear, this only effects posts created programmatically (eg an xml import). when posts are created in the editor UI, an initial revision is already being created. I'm not sure post types need to opt into this behavior, this is how revisions should work when have them enabled.

For custom post statuses programmatically created posts, I like the idea of adding something to register_post_status, as long as the name includes "programmatic" or "wp_create_post" to make it clear it only effects that.

I agree we should consider new_to_ private, future and pending statuses, these would also be missing an initial revision when importing for example, vs if you created them in the admin UI. Might apply to every status, I'll have to check what triggers for a normal draft.

I will do some follow up testing with various post statuses and report back here .

[TimothyBlynJacobs]

Replying to adamsilverstein:

It also looks like there are a couple of REST API tests that will need to be updated. The only one that might be a functionality change we are concerned about is WP_Test_REST_Revisions_Controller::test_get_item.

Do you see these tests failing?

Yes, they're failing on the PR.

It seems like the revision no longer has author information?

Interesting, I would expect that to come from the saved post, but perhaps it comes from current_user which might not be set for something like an import. This might be ok in that case. How does it show in the UI? Any issues when restoring the revision (I wouldn't think so because the author would become the current user)?

The REST tests create the post in the setup where IIRC there isn't a current user set. I'm not sure if it is causing any breakage in the revisions screen, but does seem incorrect to me.

[audrasjb]

As today is WP 5.9 beta 1, let's move this ticket to the next milestone.

[kirasong]

This ticket was checked into ​during a 6.0 bug scrub today.

Because it hasn't had recent movement, consensus was to move it out of the milestone. If it's ready before RC, please feel free to move it back!

I've also swapped has-patch for needs-patch because the last reports were that there are failing tests that require changes.

[SirLouen]

Replying to kirasong:

I've also swapped has-patch for needs-patch because the last reports were that there are failing tests that require changes.

Forgot to also remove needs-testing given that there is no new patch now to test.