WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 12 months ago

Last modified 12 months ago

#7392 closed defect (bug) (fixed)

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

Reported by: Otto42 Owned by: 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 3 years ago.
7392-2.diff (2.6 KB) - added by adamsilverstein 18 months ago.
7392-3.diff (2.5 KB) - added by adamsilverstein 17 months ago.
removed error_log
revisions.php (1.7 KB) - added by adamsilverstein 17 months ago.
corrected php doc, removed empty setup teardown
dont-save-autosave.2.diff (1.3 KB) - added by adamsilverstein 16 months ago.
adds check in wp_create_post_autosave for duplicate content
dont-save-autosave.diff (1.1 KB) - added by adamsilverstein 16 months ago.
adds check in wp_create_post_autosave for duplicate content
7392-autosave.patch (1.5 KB) - added by azaozz 16 months ago.
7392.2.diff (1.6 KB) - added by adamsilverstein 14 months ago.
check for duplicate data before autosaving in wp_create_post_autosave
7392.4.patch (1.5 KB) - added by ethitter 12 months ago.
7392.3.diff (739 bytes) - added by nacin 12 months ago.
7392.4.diff (1.1 KB) - added by nacin 12 months ago.
7392.5.diff (1.0 KB) - added by azaozz 12 months ago.
7392-6.patch (2.5 KB) - added by azaozz 12 months ago.
7392-7.patch (1.6 KB) - added by azaozz 12 months ago.
7392.6.diff (1.8 KB) - added by markjaquith 12 months ago.
with refresh if the bug is not triggered. Leave it running to test.
7392.7.diff (1.3 KB) - added by markjaquith 12 months ago.
Without the debug cruft.
7392.8.diff (1.1 KB) - added by nacin 12 months ago.

Download all attachments as: .zip

Change History (76)

comment:1 Otto426 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?

comment:2 navjotjsingh6 years ago

  • Milestone changed from 2.9 to 2.8

comment:3 FFEMTcJ5 years ago

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

comment:4 Denis-de-Bernardy5 years ago

  • Component changed from General to Autosave
  • Owner anonymous deleted

comment:5 caesarsgrunt5 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.

comment:7 AmbushCommander4 years ago

  • Cc AmbushCommander added

comment:8 solarissmoke3 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.

solarissmoke3 years ago

comment:9 kurtpayne22 months ago

  • Cc kurtpayne added
  • Keywords 3.6-early added

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

comment:10 westi18 months 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/

comment:11 SergeyBiryukov18 months ago

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

comment:12 follow-up: SergeyBiryukov18 months ago

  • Component changed from Autosave to Revisions

comment:13 in reply to: ↑ 12 westi18 months 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.

comment:14 follow-up: azaozz18 months 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.

comment:15 in reply to: ↑ 14 adamsilverstein18 months 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 18 months ago by adamsilverstein (previous) (diff)

comment:16 adamsilverstein18 months 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.

adamsilverstein18 months ago

comment:17 nacin18 months ago

This is also something we noticed while doing #22687.

comment:18 adamsilverstein18 months ago

  • Cc adamsilverstein@… added

comment:19 dh-shredder18 months ago

  • Cc dh-shredder added

comment:20 markjaquith17 months ago

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

Assigning to westi — can you take another look?

adamsilverstein17 months ago

removed error_log

adamsilverstein17 months ago

corrected php doc, removed empty setup teardown

comment:21 westi17 months 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

comment:22 westi17 months 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.

comment:23 westi17 months 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

comment:24 westi17 months 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.

adamsilverstein16 months ago

adds check in wp_create_post_autosave for duplicate content

adamsilverstein16 months ago

adds check in wp_create_post_autosave for duplicate content

comment:25 azaozz16 months 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.

azaozz16 months ago

comment:26 follow-up: azaozz16 months ago

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

comment:27 in reply to: ↑ 26 adamsilverstein16 months 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.

comment:28 nacin16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening due to remaining patches.

adamsilverstein14 months ago

check for duplicate data before autosaving in wp_create_post_autosave

comment:29 adamsilverstein14 months 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)

ethitter12 months ago

comment:30 ethitter12 months 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.

comment:31 follow-up: nacin12 months ago

Per IRC, it seems this may not be needed?

comment:32 in reply to: ↑ 31 ethitter12 months 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.

comment:33 nacin12 months ago

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

nacin12 months ago

comment:34 nacin12 months 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.

nacin12 months ago

comment:35 follow-up: nacin12 months 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.

comment:36 nacin12 months 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.

azaozz12 months ago

comment:37 in reply to: ↑ 35 azaozz12 months 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 12 months ago by azaozz (previous) (diff)

comment:38 markjaquith12 months ago

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

comment:39 markjaquith12 months ago

  • Keywords commit added; close removed

comment:40 follow-up: nacin12 months 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.

comment:41 AmbushCommander12 months ago

  • Cc AmbushCommander removed

comment:42 in reply to: ↑ 40 nacin12 months 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.)

comment:43 adamsilverstein12 months ago

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

comment:44 adamsilverstein12 months ago

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

comment:45 follow-up: azaozz12 months 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 12 months ago by azaozz (previous) (diff)

comment:46 aaroncampbell12 months ago

  • Cc aaroncampbell added

comment:47 aaroncampbell12 months 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.

comment:48 in reply to: ↑ 45 adamsilverstein12 months 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() {

?

comment:49 follow-up: azaozz12 months 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.

azaozz12 months ago

azaozz12 months ago

comment:50 azaozz12 months 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.

comment:51 in reply to: ↑ 49 ; follow-up: adamsilverstein12 months 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?

comment:52 in reply to: ↑ 51 azaozz12 months 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.

markjaquith12 months ago

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

markjaquith12 months ago

Without the debug cruft.

comment:53 follow-up: markjaquith12 months 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.

comment:54 in reply to: ↑ 53 aaroncampbell12 months 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.

comment:55 nacin12 months ago

In 24849:

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

comment:56 markjaquith12 months ago

In 24852:

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

nacin12 months ago

comment:57 nacin12 months 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.

comment:58 markjaquith12 months 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.

comment:59 markjaquith12 months 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.