Don't create new autosave revision if nothing has changed yet

[Otto42]

I noticed that simply loading and saving a post creates a new revision. This seems rather dumb to me. Could we not load the post up, compare it with the revision, and not create a whole new revision if nothing changed (ignoring post save time, of course)?

Discuss.

[Otto42]

Additional: I worded that badly.

What I mean is that when I load a post up and wait for the autosave, I get the autosave revision there, and future loads of that post tell me that there's a newer autosave, even though nothing has changed with the post.

Can we disable autosave until something actually changes in the post?

[caesarsgrunt]

+1 to this. I'm guessing it would be fairly easy to fix, though I haven't investigated.

[Denis-de-Bernardy]

see also #9843

[solarissmoke]

This patch prevents revisions being created if the content hasn't changed. I think it should resolve #9843 as well.

[kurtpayne]

Confirmed original issue and tested 7392.diff​. Looks good.

[westi]

Maybe a candidate for investigation and resolution during the autosave/revision work in 3.6.

​http://make.wordpress.org/core/2013/01/19/autosave-and-post-locking/

[westi]

Replying to SergeyBiryukov:

I left this in Autosave as it is an Autosave issue not a revisions issue.

[azaozz]

This happens only when the Visual editor is default (is shown first). Caused by running the post_content through wpautop before outputting it in the textarea (TinyMCE needs the <p>), so the first autosaveLast (used to determine if something has changed) gets post_content with <p> and is later compared to post_content without <p>.

There are better ways to do that, should be fixed with the autosave updates in 3.6.

[adamsilverstein]

Replying to azaozz:

This happens only when the Visual editor is default (is shown first). Caused by running the post_content through wpautop before outputting it in the textarea (TinyMCE needs the <p>), so the first autosaveLast (used to determine if something has changed) gets post_content with <p> and is later compared to post_content without <p>.

There are better ways to do that, should be fixed with the autosave updates in 3.6.

i've noticed this doesn't get fired on new posts if i create a post and publish it with just a title without entering any content.

in addition to the extra autosave, this winds up creating an extra revision when creating a new post, so creating a post with title and content starts out with two revisions, the 1st one is title only, the 2nd has title and content. thats confusing to users - see #9843. the current patch seems to do too much by checking for duplication, can we just check for the initial state you mention and skip the first and only the first autosave?

going to test the current patch anyway.

[adamsilverstein]

the current patch works as advertised, the post is not updated if the content is unchanged.

posting corrected diff so patch works agains current for testing.

investigating double initial revisions (related - #9843, #16215 ) next.

[nacin]

This is also something we noticed while doing #22687.

[markjaquith]

Assigning to westi — can you take another look?

[adamsilverstein]

removed error_log

[adamsilverstein]

corrected php doc, removed empty setup teardown

[westi]

In 1211/tests:

Revisions: When we are trying to save a new revision we shouldn't save one if the previous version was the same.

See #7392 and #9843 props adamsilverstein

[westi]

In 23414:

Revisions: Before saving a new post revision make sure that something has changed in the fields that we are revisioning.

Fixes: #7392 and #9843 props adamsilverstein.

[westi]

In 1214/tests:

Revisions: Add a new test for a force save filter in wp_save_post_revision

Also Update the tests to have correct ordering, be less complex and use things like assertCount.
See #7392 and #9843

[westi]

In 23415:

Revisions: Allow a plugin to force us to skip the don't save this revision because it hasn't changed code if it knows better.

See #7392 and #9843. Also cleans up the whitespace.

[adamsilverstein]

adds check in wp_create_post_autosave for duplicate content

[adamsilverstein]

adds check in wp_create_post_autosave for duplicate content

[azaozz]

dont-save-autosave.diff won't work as expected (and has a copy/paste typo, $new_data is not defined in wp_create_post_autosave()).

It only checks whether the autosave is the same as the current post when a previous autosave exists. To catch the first autosave, perhaps we can move the test for changed fields from wp_save_post_revision() to _wp_put_post_revision(). We will still need the same test in wp_create_post_autosave() for the cases when an autosave is updated.

[azaozz]

Alternatively something like 7392-autosave.patch​ should work.

[adamsilverstein]

Replying to azaozz:

Alternatively something like 7392-autosave.patch​ should work.

thanks, that looks good. do we still need the check in _wp_put_post_revision as well? will try testing this patch.

[nacin]

Re-opening due to remaining patches.

[adamsilverstein]

check for duplicate data before autosaving in wp_create_post_autosave

[adamsilverstein]

in 7392.2.diff​​:

• add duplicate content check in wp_create_post_autosave, refreshed from 7392-autosave.patch​
• note that if "revisioned fields" (by default: title, content, excerpt) are unchanged, autosaves will not fire, even if you change revisioned post_meta such as post format. (also true of regular update revisions)

[ethitter]

Refreshed adamsilverstein's patch in ​7392.4.patch and confirmed that, when applied, we no longer create new autosaves if nothing's changed.

[nacin]

​Per IRC, it seems this may not be needed?

[ethitter]

Replying to nacin:

​Per IRC, it seems this may not be needed?

I agree. The changes to autosave in 3.6 pretty thoroughly solve this problem on the client side. I don't think it's worth worrying about what people may be doing in third party apps that could result in autosaves whose content mirrors that of an existing revision or the post itself.

[nacin]

MarkJaquith and I ran into this again while working on #24804. (This is a bit more noticable in the new revisions UI.) I came to the same conclusion azaozz happened to have reached months ago in comment 14:

This happens only when the Visual editor is default (is shown first). Caused by running the post_content through wpautop before outputting it in the textarea (TinyMCE needs the <p>), so the first autosaveLast (used to determine if something has changed) gets post_content with <p> and is later compared to post_content without <p>.

There are better ways to do that, should be fixed with the autosave updates in 3.6.

It's not yet fixed, but 7392.3.diff​ does it.

I agree with fixing this in JS still — so let's do it.

[nacin]

7392.4.diff​ does two things:

• Prevents an initial autosave when the TinyMCE editor is active on load.
• Deletes useless autosaves when edit-form-advanced.php confirms it is useless.

This, however, does not solve this issue:

• Add a sentence.
• Let autosave occur.
• Remove the sentence.
• Let autosave occur.

At this point, the autosave is identical to the active/current revision, and should actually be *deleted*. This sounds like 7392.4.patch​ but not quite — that would prevent the autosave from being updated with the removed sentence, likely leaving the autosave with the added sentence. In reality the user's autosave should be deleted.

7392.4.diff​ is clearly a simple 3.6 improvement, though. Needs azaozz.

[nacin]

In 24787:

Autosave: Don't save an autosave unnecessarily when the post editor loads and TinyMCE is the default editor.

Delete such useless autosaves when we detect them in edit-form-advanced.php.

see #7392.

[azaozz]

In .4.diff there is no need to run the content through switchEditors.pre_wpautop() again. We wait for TinyMCE to load, then we call editor.save() which serializes the iframe html, runs it through all filters including pre_wpautop() and places it in the textarea. At this point the content of the textarea is exactly the same as when autosave retrieves it (if the user didn't type anything).

We need to load the content into TinyMCE and then save it back to the textarea as when serializing the iframe html the order of tag attributes may change and trigger a false positive (i.e. <a title="..." src="..."> would become <a src="..." title="...">).

[markjaquith]

7392.5.diff​ seems to work. Doesn't regress the fix in [24787].

[nacin]

7392.5.diff​ regresses [24787]. Also confirmed by aaroncampbell.

For whatever reason, on initialization, autosaveLast is getting assigned paragraph tags. I don't know why, but it does. And [24787] fixes it. I don't know why, but it does. Once autosave fires, autosaveLast is re-populated and it is no longer assigned paragraph tags, so subsequent autosave checks bail for lack of content changes.

[nacin]

Replying to nacin:

7392.5.diff​ regresses [24787]. Also confirmed by aaroncampbell.

For whatever reason, on initialization, autosaveLast is getting assigned paragraph tags. I don't know why, but it does. And [24787] fixes it. I don't know why, but it does. Once autosave fires, autosaveLast is re-populated and it is no longer assigned paragraph tags, so subsequent autosave checks bail for lack of content changes.

So... Upon further review, this isn't entirely right. aaroncampbell, markjaquith, and I are sometimes getting autosaveLast with paragraph tags, and sometimes without. We're all reproducing it at vastly different frequencies. (Meanwhile, duck_ still can't reproduce it.) All on Chrome (same version, some Linux, some Mac), also reproduced in Safari to some extent. (Not reproduced in Firefox at all.)

[adamsilverstein]

interesting problem! this sounds like a timing issue between when the first autosave fires and TinyMCE initialization.

[adamsilverstein]

any tricks to reproducing, I've tried safari and chrome and have yet to reproduce.

[azaozz]

Was able to reproduce in Chrome (after lots of attempts). The problem is with script timing. This should run after TinyMCE is loaded but before tinymce.init(). One way to do this is by firing a JS event. Currently we init TinyMCE as soon as it loads as we want it to be ready for use as soon as possible.

Another option is to move that code from autosave.js to the 'wordpress' TinyMCE plugin. As autosaveLast is a JS global we can set it initially to the as-loaded value and then set it to as-saved-from-tinymce when the Visual editor loads. Testing a new patch.

[aaroncampbell]

Just to add to the madness that Nacin posted, here's the results I got when I tested:

• I can reproduce on nearly every page load on Chromium (28.0.1500.71) and Chrome (28.0.1500.71) on Linux. Duck_ has a similar setup with the exact same version of Chromium and he can't reproduce.
• I tested for hours (auto-refresh on several tabs, so that's a LOT of loads) on Firefox and Opera and could not reproduce at all.

[adamsilverstein]

Replying to azaozz:

Was able to reproduce in Chrome (after lots of attempts). The problem is with script timing. This should run after TinyMCE is loaded but before tinymce.init(). One way to do this is by firing a JS event. Currently we init TinyMCE as soon as it loads as we want it to be ready for use as soon as possible.

Another option is to move that code from autosave.js to the 'wordpress' TinyMCE plugin. As autosaveLast is a JS global we can set it initially to the as-loaded value and then set it to as-saved-from-tinymce when the Visual editor loads. Testing a new patch.

i'm confused about this line:

editor.onLoad.add( function() { (​http://core.trac.wordpress.org/browser/trunk/wp-includes/js/autosave.js#L8)

shouldn't that be

editor.onLoadEditor.add( function() {

?

[azaozz]

i'm confused about this line...

No, onLoad is a custom event in TinyMCE 3.x ('load' in 4.x) fired when the iframe body is loaded (it is literally fired by body.onload). However there are some browser inconsistencies with firing this event when the iframe html is set from JS. It seems it sometimes fires too soon in WebKit and IE10.

[azaozz]

7392-6.patch adds a custom jQuery event when the TinyMCE iframe body is loaded, then saves the content to the textarea and sets autosaveLast after 1 sec. delay.

7392-7.patch removes the TinyMCE parts from [24787] as they don't work consistently.

Both patches have temporary debug code still in for easier testing.

[adamsilverstein]

Replying to azaozz:

i'm confused about this line...

No, onLoad is a custom event in TinyMCE 3.x ('load' in 4.x) fired when the iframe body is loaded (it is literally fired by body.onload). However there are some browser inconsistencies with firing this event when the iframe html is set from JS. It seems it sometimes fires too soon in WebKit and IE10.

ah, thanks for clarifying. found the call in the source code, but curiously not in the API documentation.

hopefully, your latest patches solve the problem! I'm trying to understand whats happening here more fully - you said "This should run after TinyMCE is loaded but before tinymce.init()" - can you tell me why you aren't using tinymce.Editor.onPreInit?

[azaozz]

Replying to adamsilverstein:

I'm trying to understand whats happening here more fully - you said "This should run after TinyMCE is loaded but before tinymce.init()" - can you tell me why you aren't using tinymce.Editor.onPreInit?

Sry, that wasn't clear. Meant the .onAddEditor bit in autosave.js should be run after tiny_mce.js is loaded but before the editor instance is created with tinymce.init(). That's the best time to use tinymce.onAddEditor to be able to get the new instance as soon as it's ready from outside TinyMCE. To use .onPreInit you need access to that instance. Moving that bit to the 'wordpress' TinyMCE plugin and firing a custom jQuery event in 7392-6.patch solves this.

The problem is with doing .save() as soon as the editor is ready. The last event fired is editor.onLoad however this seems to still be too soon in some cases in WebKit and IE10. Both "loading" content in the iframe and "saving" it back to the textarea run a lot of internal filters, cleanup, browser quirks fixes, etc.

[markjaquith]

with refresh if the bug is not triggered. Leave it running to test.

[markjaquith]

Without the debug cruft.

[markjaquith]

Ran 7392.6.diff​ for a good long while in 5 browser tabs and couldn't get the "bad" situation to happen.

[aaroncampbell]

Replying to markjaquith:

Ran 7392.6.diff​ for a good long while in 5 browser tabs and couldn't get the "bad" situation to happen.

I just did the same thing in several browsers. Didn't get the issue to happen at all.

[nacin]

In 24849:

Avoid racing TinyMCE, which avoids the creation of unnecessary autosaves. props azaozz. see #7392.

[markjaquith]

In 24852:

Avoid racing TinyMCE, which avoids the creation of unnecessary autosaves. props azaozz. see #7392, for 3.6.

[nacin]

7392.8.diff​ fixes this situation:

• Add a sentence.
• Let autosave occur.
• Remove the sentence.
• Let autosave occur.
• At this point, the autosave should be deleted.

It only deletes the autosave when there is an existing autosave, because JS handles the situation when there is no existing autosave (where an autosave is identical to the current post) and because it's actually more difficult to handle that case.

[markjaquith]

In 24878:

Delete old autosave if new autosave has same content as the post.

Props nacin. Fixes #7392 for trunk.

[markjaquith]

In 24879:

Delete old autosave if new autosave has same content as the post.

Props nacin. Fixes #7392 for 3.6.