WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 17 months ago

#20299 assigned defect (bug)

Preview changes on a published post makes all post meta "live"

Reported by: jakemgold Owned by: adamsilverstein
Milestone: Future Release Priority: normal
Severity: major Version: 3.3.1
Component: Revisions Keywords: has-patch needs-testing dev-feedback 4.6-early
Focuses: administration Cc:

Description

Here's the use case. Client wants to preview an update to a published post (as the Preview Changes button correctly implies they can). This post has some important post meta that impacts that preview.

Here's the problem - because post meta is not saved to a revision (it looks for the "real" post), when the preview button is pressed, save_post runs, and saves the meta data to the real, published post, even though the user only intends to preview the change.

Without realizing it, the user has updated the published version. That can be prevented by not saving post meta to revisions (when using custom save_post hooks), but then there's no non-hacky way to actually preview the full changes.

I believe this bug has been present for a while, we just rarely use the Preview function on published posts, and when we do, probably never tested it with critical post meta.

Attachments (9)

20299.patch (2.0 KB) - added by WraithKenny 5 years ago.
20299-2.patch (1.9 KB) - added by WraithKenny 5 years ago.
20299-test.diff (1.5 KB) - added by adamsilverstein 4 years ago.
test saving post meta to autosave wp_create_post_autosave
20299-3.patch (5.8 KB) - added by adamsilverstein 3 years ago.
20299.4.patch (5.3 KB) - added by adamsilverstein 2 years ago.
20299.2.patch (5.4 KB) - added by adamsilverstein 2 years ago.
20299.5.patch (5.4 KB) - added by adamsilverstein 2 years ago.
20299.diff (5.5 KB) - added by adamsilverstein 2 years ago.
20299.2.diff (4.8 KB) - added by adamsilverstein 2 years ago.

Download all attachments as: .zip

Change History (69)

#1 @johnbillion
6 years ago

Could you show the code you're using to save your post meta?

#2 @alexkingorg
6 years ago

  • Cc alexkingorg@… added

#3 @jkudish
6 years ago

  • Cc joachim.kudish@… added

#4 @ryanduff
6 years ago

  • Cc ryan@… added

#5 @jaredatch
6 years ago

  • Cc jared@… added

#6 @bookwyrm
6 years ago

  • Cc bookwyrm added

This is being done by the update_post_meta function:

    // make sure meta is added to the post, not a revision
    if ( $the_post = wp_is_post_revision($post_id) )
        $post_id = $the_post;

Why wouldn't you want the postmeta data saved to the revision?

#7 @mintindeed
6 years ago

  • Cc gabriel.koen@… added

#9 @coocoomoo
5 years ago

  • Cc coocoomoo added

#10 @ryanduff
5 years ago

So instead of updating the actual post_meta values with each revision, how about it saves an array of the post meta in one row of the post_meta table with the meta key being something like _revision and the id being that of the revision and not of the actual post?

The preview could work with that since you're essentially viewing that revision and we could grab the meta array for that and expand it out. The actual post would continue to use the unmodified entries in the post_meta table. On save the values would finally be pushed to the respective entries in post_meta.

The other option is a full blown post meta revisions system that Alex King proposed over in #20564

Either way, one route or the other needs to be picked before something can be implemented.

#11 @DrewAPicture
5 years ago

  • Cc xoodrew@… added

#12 @jakemgold
5 years ago

  • Version 3.3.1 deleted

I think some branches (pun intended) need to be shaken on this one. I think this is in many ways a serious bug, at least if we want to have a serious Preview feature.

Solution proposed by @ryanduff seems a bit janky to me, and risky (size wise) if a significant amount of post meta is stored.

I think @bookwyrm solution is where my mind is going - it's not immediately clear to me at all why that block of code that prevents post meta from saving to revisions is in there (preventing database clutter... ?).

Like the deeper solution @alexkingorg is proposing in his related ticket, but not sure it even has to be that involved.

#13 @jakemgold
5 years ago

  • Version set to trunk

#14 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 3.3.1

Version number indicates when the bug was initially introduced/reported.

#15 @mintindeed
5 years ago

You'd probably want to exclude some post meta that are specific to the main post, like edit locks. I imagine the patch would look something like this:

$exclude_meta_keys = apply_filters( 'revision_meta_do_not_copy', array(
	'_encloseme',
	'_pingme',
	'_edit_last',
	'_edit_lock'
) );
$current_meta = get_post_custom( $GLOBALS['post'] );
if ( is_array( $current_meta ) ) {
	foreach ( $current_meta as $meta_key => $meta_value ) {
		if ( in_array( $meta_key, $exclude_meta_keys ) ) {
			continue;
		}
		
		update_post_meta( $revision_id, $meta_key, $meta_value );
	}
}

#16 follow-up: @wonderboymusic
5 years ago

You want to ignore everything starting with _
Those are supposed to be "private"

#17 in reply to: ↑ 16 @jakemgold
5 years ago

Replying to wonderboymusic:

You want to ignore everything starting with _
Those are supposed to be "private"

This isn't quite right.

First, it seems to me that some keys (like thumbnail id) should be revision specific (although this might open up another can of worms - I find it disturbing that selecting a new thumbnail immediately updates a published post without clicking "update").

Second, many developers use the prefix to prevent meta data from showing up int he free form "custom fields" meta box.

I think @mintindeed is more on the right track, explicitly ignoring certain keys, in a filterable way.

Are there any other keys we're missing that need to be attached to the "real" post?

#18 @jkrill
5 years ago

  • Cc jkrill added

#19 @aaroncampbell
5 years ago

  • Cc aaroncampbell added

#20 @sennza
5 years ago

  • Cc bronson@… added

#21 @jb510
5 years ago

  • Cc jbrown510@… added

#22 @nacin
5 years ago

  • Component changed from General to Revisions
  • Milestone changed from Awaiting Review to 3.6

This and #20564 would block #23314.

#23 @adamsilverstein
5 years ago

  • Cc adamsilverstein@… added

#24 @danielbachhuber
5 years ago

  • Cc danielbachhuber added
  • Keywords editorial-flow added

#25 @beaulebens
5 years ago

  • Cc beau@… added

#26 @goto10
5 years ago

  • Cc dromsey@… added

#27 @kwight
5 years ago

  • Cc kwight@… added

#28 @WraithKenny
5 years ago

  • Cc Ken@… added

#29 @WraithKenny
5 years ago

#20564 blocks this ticket, but if/when that goes in, the show preview functionality would have to somehow pull the meta from the $preview instead of the $post :

In revision.php -> function _set_preview($post)... maybe set $post->preview_id = $preview->ID (or use a global) then set up a filter on 'get_post_metadata' to pull out the right data.

#30 @WraithKenny
5 years ago

  • Keywords has-patch added

rough pass, not tested. Will test when I get home

#31 @WraithKenny
5 years ago

  • Keywords needs-testing added

@WraithKenny
5 years ago

#32 @WraithKenny
5 years ago

worked with patch from #20564 ('versioned_meta') and the recent 23842.

Last edited 5 years ago by WraithKenny (previous) (diff)

@WraithKenny
5 years ago

#33 @WraithKenny
5 years ago

v2 uses get_versioned_meta_keys from the patch on #20564

#34 follow-up: @WraithKenny
5 years ago

23862 included the gist of my 20299-2.patch (with an improved 'get_post_metadata filter, nice work kovshenin).

I think making _wp_post_revision_meta_keys() filterable, as well as adding some other core meta to the list (like thumbnail maybe) would solve this finally :).

Last edited 5 years ago by WraithKenny (previous) (diff)

#35 in reply to: ↑ 34 @SergeyBiryukov
5 years ago

Replying to WraithKenny:

I think making _wp_post_revision_meta_keys() filterable, as well as adding some other core meta to the list (like thumbnail maybe) would solve this finally :).

Related: #23973

#36 @azaozz
5 years ago

I think making _wp_post_revision_meta_keys() filterable...

_wp_post_revision_meta_keys() is specific for post formats, won't work for arbitrary meta. Even thinking it should be renamed to _post_format_meta_keys().

#37 @ocean90
5 years ago

  • Milestone changed from 3.6 to Future Release

Let's revisit this in 3.7.

#38 @husobj
4 years ago

  • Cc ben@… added

#39 @sanchothefat
4 years ago

I have some code that gets around this problem for previews that doesn't feel too horrendous. It's a problem that's been really bothering some of our clients who want to preview changes to custom post types (eg. events) with lots of metadata.

Here's the gist: https://gist.github.com/sanchothefat/498ed66a48a4a56d349d

You don't get post meta for each revision rather the preview meta data is stored against the original post with a prefixed meta_key as others have suggested. It gets overridden on each update/preview and you won't end up with orphaned post meta. Works fairly well so far but those sound like famous last words...

#40 in reply to: ↑ description @yrosen
4 years ago

  • Cc yrosen added

#41 @doughamlin
4 years ago

  • Cc doughamlin@… added

#42 @westonruter
4 years ago

  • Cc weston@… added

@adamsilverstein
4 years ago

test saving post meta to autosave wp_create_post_autosave

#43 @adamsilverstein
4 years ago

As far as I can tell - in trunk - the post meta is not saved to published posts when clicking preview. Testing using the 'Custom Fields' section of the edit post page, the values are not saved until I click publish (when a post is in the draft state, the meta is saved when I click the preview button).

I believe the ticket description is (no longer) accurate when it states "when the preview button is pressed, save_post runs, and saves the meta data to the real, published post".

The bigger, remaining, issue is: "there's no non-hacky way to actually preview the full changes". This is because the wp_create_post_autosave function does not save the post meta - when the preview is loaded from the revision, post meta is pulled from the parent post.

(The function post_preview calls wp_create_post_autosave for published posts, creating an 'autosave' revision, and returns a url for previewing this autosave. )

The current patch for #20564 (Framework for storing revisions of Post Meta) includes _wp_preview_meta_filter to display revisioned post_meta fields with the preview. Landing #20564 will go a long way towards fixing this and #23314.

#44 @azaozz
4 years ago

The "gist" of the problem is that clicking "Preview" submits the whole form, i.e. quite similar to clicking either Save or Update or even Publish, without changing the post_status.

For drafts this works well. It is equal to clicking Save Draft + View Post in a new tab. All data from all form fields (including those added by plugins) is saved.

However for published posts this is not ideal. The filters that run on updating and inserting posts can trigger all sorts of related tasks and many plugins are not aware that the post is not being updated, only previewed.

This needs standardizing. Thinking that best would be to limit what is being submitted on clicking "Preview" and "Preview Changes" only to post_title, post_content and excerpt. Then add a filter for additional form fields that will be saved and included in the preview. This could work by filtering $_POST. Even maybe set DOING_AUTOSAVE or perhaps DOING_PREVIEW.

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

This ticket was mentioned in IRC in #wordpress-dev by danielbachhuber. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

#47 @adamsilverstein
3 years ago

  • Keywords dev-feedback added

Notes on 20299-3.patch:

  • First take, feedback welcome!
  • Requires the wp-post-meta-revisioning plugin
  • Changes wp_updating_autosave hook to wp_creating_autosave when creating autosave
  • Adds wp_updating_autosave hook when actually updating autosave
  • Sets DOING_PREVIEW so plugins know not to save meta on previews
  • Includes revisioned meta field values as part of autosaves (expects key name to match element ID)
  • Localises the list of revisioned meta keys so autosave.js knows which fields to add to the autosave
  • When doing preview, uses the autosave revisioned post meta
  • Includes a unit test verifying the fix, uses an include to pull in the wp-post-meta-revisions plugin


Testing:

  • You can test the current patch using this simple plugin; set the theme to twenty-fifteen and choose a background color. Changing and previewing you will nee the new color, but the original post will remain unaffected. Notes: checks DOING_PREVIEW to avoid saving meta during preview; uses the wp_post_revision_meta_keys filter to specify meta for revisioning
Last edited 3 years ago by adamsilverstein (previous) (diff)

#48 follow-up: @adamsilverstein
2 years ago

[attachmen:20299.4.patch]:

  • Update against trunk
  • Cleanup
  • Update for namespaced plugin

Note: Requires the latest version of the wp-post-meta-revisioning plugin

#49 @adamsilverstein
2 years ago

  • Focuses administration added
  • Keywords editorial-flow removed
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#50 in reply to: ↑ 48 ; follow-up: @azaozz
2 years ago

Replying to adamsilverstein:

Looking at 20299.4.patch, there are some inconsistencies:

  • Autosave has nothing to do with previews, maybe better to patch it in a separate ticket. Also, the patch just "pokes a hole" in it letting plugins pass through arbitrary data. Maybe that needs some structuring.
  • $( document.getElementById( meta ) ).val() should probably be $( '#' + meta ).val(), or document.getElementById( meta ).value if you don't want jQuery.
  • Namespaces are not supported in PHP 5.2, fatal error :)

Having a patch that makes core dependent on a plugin feels off. I understand it is just for testing, but thinking we need a better way.

#51 in reply to: ↑ 50 @adamsilverstein
2 years ago

Thanks for the feedback, much appreciated!

A few comments below:

Looking at 20299.4.patch, there are some inconsistencies:

  • Autosave has nothing to do with previews, maybe better to patch it in a separate ticket. Also, the patch just "pokes a hole" in it letting plugins pass through arbitrary data. Maybe that needs some structuring.

Yes, this takes advantage of autosave to add the meta for the preview, that seemed like a natural progression from the current code. I will try adding structure. Not exactly sure how :)

  • $( document.getElementById( meta ) ).val() should probably be $( '#' + meta ).val(), or document.getElementById( meta ).value if you don't want jQuery.

good suggestion, thanks. jQuery we have, I'm using document.getElementById out of habit, because it's faster.

  • Namespaces are not supported in PHP 5.2, fatal error :)

Right. I switched the plugin to namespaced to make an eventual core patch a little cleaner, but really could switch back to a Class for 5.2 compat at any time if that is preferable.

Having a patch that makes core dependent on a plugin feels off. I understand it is just for testing, but thinking we need a better way.

The better way would be to merge the plugin into core :)

That let us solve this and a handful of other related tickets.

I have some time to put into this now. Should I suggest the plugin as a feature plugin? I made it a plugin because the code had languished for too long on #20564 and also to open it up to more users. I have heard from some people using the plugin successfully.

#52 @adamsilverstein
2 years ago

20299.5.patch

  • Switch back to class vs namespacing for PHP 5.2 compatibility

Still working on the other suggestions.

#54 @adamsilverstein
2 years ago

20299.diff :

  • Refresh against trunk
  • Use factory for post creation in unit test

#55 @adamsilverstein
2 years ago

20299.2.diff

  • Clean up docs
  • Removed unused 'DOING_PREVIEW' constant
  • This patch is pretty simple, the bulk of the functionality is in the wp-post-meta-revisions plugin

Note, requires the the wp-post-meta-revisions plugin.

#56 @johnbillion
21 months ago

  • Keywords 4.6-early added

#57 follow-ups: @westonruter
21 months ago

@adamsilverstein in regards to #11049 and not being able to preview a change to a page template, will the scope here also include addressing the issue where changing the featured image actually makes it go live immediately (the opposite problem from page template)? I couldn't find a trac ticket about this, but as a user I would expect to be able to set a featured image and then click Preview, and then actually _preview_ that change without it going live for all. The solution seems as simple as changing the featured image behavior to populate a hidden _thumbnail_id input so that it gets sent along with all of the other postmeta upon hitting Save or Preview. (In other words, the wp_ajax_set_post_thumbnail() should be eliminated.)

#58 in reply to: ↑ 57 @adamsilverstein
21 months ago

Replying to westonruter:

@adamsilverstein in regards to #11049 and not being able to preview a change to a page template, will the scope here also include addressing the issue where changing the featured image actually makes it go live immediately (the opposite problem from page template)?

Yes, this plugin plus some whitelisting would address this issue.

Previews currently rely on an autosave revision to load their data, and since post meta like featured image is not revisioned it gets saved, then the stored value 'previewed'.

All that should be required to fix this (after installing the plugin) is whitelisting _thumbnail_id with the wp_post_revision_meta_keys filter. The plugin already includes code to load the revisioned meta for previews, see https://github.com/adamsilverstein/wp-post-meta-revisions/blob/master/wp-post-meta-revisions.php#L38

If you have a chance, please give it a try: I will do more testing and refreshing of patches on my end soon.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


19 months ago

#60 in reply to: ↑ 57 @ocean90
17 months ago

Replying to westonruter:

.... will the scope here also include addressing the issue where changing the featured image actually makes it go live immediately (the opposite problem from page template)? I couldn't find a trac ticket about this, but as a user I would expect to be able to set a featured image and then click Preview, and then actually _preview_ that change without it going live for all.

See #12922.

Note: See TracTickets for help on using tickets.