Make WordPress Core

Opened 16 years ago

Closed 11 years ago

Last modified 11 years ago

#7392 closed defect (bug) (fixed)

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

Reported by: otto42's profile Otto42 Owned by: westi's profile westi
Milestone: 3.6 Priority: normal
Severity: minor Version: 2.6
Component: Autosave Keywords: has-patch needs-testing autosave-redo commit
Focuses: Cc:

Description

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.

Attachments (17)

7392.diff (2.3 KB) - added by solarissmoke 13 years ago.
7392-2.diff (2.6 KB) - added by adamsilverstein 11 years ago.
7392-3.diff (2.5 KB) - added by adamsilverstein 11 years ago.
removed error_log
revisions.php (1.7 KB) - added by adamsilverstein 11 years ago.
corrected php doc, removed empty setup teardown
dont-save-autosave.2.diff (1.3 KB) - added by adamsilverstein 11 years ago.
adds check in wp_create_post_autosave for duplicate content
dont-save-autosave.diff (1.1 KB) - added by adamsilverstein 11 years ago.
adds check in wp_create_post_autosave for duplicate content
7392-autosave.patch (1.5 KB) - added by azaozz 11 years ago.
7392.2.diff (1.6 KB) - added by adamsilverstein 11 years ago.
check for duplicate data before autosaving in wp_create_post_autosave
7392.4.patch (1.5 KB) - added by ethitter 11 years ago.
7392.3.diff (739 bytes) - added by nacin 11 years ago.
7392.4.diff (1.1 KB) - added by nacin 11 years ago.
7392.5.diff (1.0 KB) - added by azaozz 11 years ago.
7392-6.patch (2.5 KB) - added by azaozz 11 years ago.
7392-7.patch (1.6 KB) - added by azaozz 11 years ago.
7392.6.diff (1.8 KB) - added by markjaquith 11 years ago.
with refresh if the bug is not triggered. Leave it running to test.
7392.7.diff (1.3 KB) - added by markjaquith 11 years ago.
Without the debug cruft.
7392.8.diff (1.1 KB) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (76)

#1 @Otto42
16 years ago

  • Summary changed from Don't create new revision if nothing changed to Don't create new autosave revision if nothing has changed yet

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?

#2 @navjotjsingh
15 years ago

  • Milestone changed from 2.9 to 2.8

#3 @FFEMTcJ
15 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.8 to Future Release

#4 @Denis-de-Bernardy
15 years ago

  • Component changed from General to Autosave
  • Owner anonymous deleted

#5 @caesarsgrunt
15 years ago

  • Severity changed from normal to minor
  • Type changed from enhancement to defect (bug)

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

#7 @AmbushCommander
14 years ago

  • Cc AmbushCommander added

#8 @solarissmoke
13 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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

@solarissmoke
13 years ago

#9 @kurtpayne
11 years ago

  • Cc kurtpayne added
  • Keywords 3.6-early added

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

#10 @westi
11 years ago

  • Keywords revisions-3.6 autosave-redo added

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/

#11 @SergeyBiryukov
11 years ago

  • Keywords revisions-3.6 removed
  • Milestone changed from Future Release to 3.6

#12 follow-up: @SergeyBiryukov
11 years ago

  • Component changed from Autosave to Revisions

#13 in reply to: ↑ 12 @westi
11 years ago

  • Component changed from Revisions to Autosave

Replying to SergeyBiryukov:

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

#14 follow-up: @azaozz
11 years ago

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.

#15 in reply to: ↑ 14 @adamsilverstein
11 years ago

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.

Last edited 11 years ago by adamsilverstein (previous) (diff)

#16 @adamsilverstein
11 years ago

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.

#17 @nacin
11 years ago

This is also something we noticed while doing #22687.

#18 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#19 @dh-shredder
11 years ago

  • Cc dh-shredder added

#20 @markjaquith
11 years ago

  • Owner set to westi
  • Status changed from new to assigned

Assigning to westi — can you take another look?

@adamsilverstein
11 years ago

removed error_log

@adamsilverstein
11 years ago

corrected php doc, removed empty setup teardown

#21 @westi
11 years ago

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

#22 @westi
11 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#23 @westi
11 years ago

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

#24 @westi
11 years ago

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
11 years ago

adds check in wp_create_post_autosave for duplicate content

@adamsilverstein
11 years ago

adds check in wp_create_post_autosave for duplicate content

#25 @azaozz
11 years ago

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.

#26 follow-up: @azaozz
11 years ago

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

#27 in reply to: ↑ 26 @adamsilverstein
11 years ago

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.

#28 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening due to remaining patches.

@adamsilverstein
11 years ago

check for duplicate data before autosaving in wp_create_post_autosave

#29 @adamsilverstein
11 years ago

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
11 years ago

#30 @ethitter
11 years ago

  • Cc erick@… added

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

#31 follow-up: @nacin
11 years ago

Per IRC, it seems this may not be needed?

#32 in reply to: ↑ 31 @ethitter
11 years ago

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.

#33 @nacin
11 years ago

  • Keywords close added; 3.6-early removed
  • Milestone changed from 3.6 to Awaiting Review

@nacin
11 years ago

#34 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.6

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
11 years ago

#35 follow-up: @nacin
11 years ago

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.

#36 @nacin
11 years ago

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
11 years ago

#37 in reply to: ↑ 35 @azaozz
11 years ago

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. <img title="..." src="..."> would become <img src="..." title="...">).

Last edited 11 years ago by azaozz (previous) (diff)

#38 @markjaquith
11 years ago

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

#39 @markjaquith
11 years ago

  • Keywords commit added; close removed

#40 follow-up: @nacin
11 years ago

  • Keywords commit removed

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.

#41 @AmbushCommander
11 years ago

  • Cc AmbushCommander removed

#42 in reply to: ↑ 40 @nacin
11 years ago

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.)

#43 @adamsilverstein
11 years ago

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

#44 @adamsilverstein
11 years ago

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

#45 follow-up: @azaozz
11 years ago

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.

Last edited 11 years ago by azaozz (previous) (diff)

#46 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

#47 @aaroncampbell
11 years ago

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.

#48 in reply to: ↑ 45 @adamsilverstein
11 years ago

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() {

?

#49 follow-up: @azaozz
11 years ago

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
11 years ago

@azaozz
11 years ago

#50 @azaozz
11 years ago

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.

#51 in reply to: ↑ 49 ; follow-up: @adamsilverstein
11 years ago

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?

#52 in reply to: ↑ 51 @azaozz
11 years ago

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
11 years ago

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

@markjaquith
11 years ago

Without the debug cruft.

#53 follow-up: @markjaquith
11 years ago

  • Keywords commit added

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

#54 in reply to: ↑ 53 @aaroncampbell
11 years ago

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.

#55 @nacin
11 years ago

In 24849:

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

#56 @markjaquith
11 years ago

In 24852:

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

@nacin
11 years ago

#57 @nacin
11 years ago

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.

#58 @markjaquith
11 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 24878:

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

Props nacin. Fixes #7392 for trunk.

#59 @markjaquith
11 years ago

In 24879:

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

Props nacin. Fixes #7392 for 3.6.

Note: See TracTickets for help on using tickets.