Make WordPress Core

Opened 13 years ago

Closed 15 months ago

Last modified 10 months ago

#20564 closed enhancement (fixed)

Framework for storing revisions of Post Meta

Reported by: alexkingorg's profile alexkingorg Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.4 Priority: normal
Severity: normal Version: 3.4
Component: Revisions Keywords: has-patch commit needs-dev-note has-unit-tests
Focuses: Cc:

Description

There are a couple of tickets that would seem to benefit from storing revisions of post meta (#11049, #20299). We had a need for this feature in our Carrington Build and RAMP products and built it a few years back.

It handles the storing of post meta along with revisions, as well as restoring those revisions when someone restores an older version of a post(/page/etc.). There is an API for registering the post meta keys you want to track revisions of, it does not track all post meta keys by default.

There are two versions of the code. The bare-bones revisions framework code is here:

https://github.com/crowdfavorite/wp-revision-manager
https://github.com/crowdfavorite/wp-revision-manager/blob/master/cf-revision-manager.php

while I started adding features to a version that would be more user-friendly plugin, allowing the users to select which post meta keys they want to track revisions for:

http://plugins.svn.wordpress.org/revision-manager/
http://plugins.svn.wordpress.org/revision-manager/trunk/cf-revision-manager.php

If this would be a valuable addition to core, I'd be happy to help polish this up in whatever manner is most helpful. I'd recommend breaking out the code I started on for the admin form and having that be the extent of the "revision manager" plugin - basically it becomes a nicer UI for selecting additional post meta keys to track revisions for.

Attachments (47)

20564.diff (5.8 KB) - added by adamsilverstein 12 years ago.
20564+16215.diff (24.3 KB) - added by adamsilverstein 12 years ago.
for testing
20564.2.diff (5.5 KB) - added by adamsilverstein 12 years ago.
updated against current trunk
20564.3.diff (5.5 KB) - added by adamsilverstein 12 years ago.
update against current
20564.4.patch (3.5 KB) - added by WraithKenny 12 years ago.
20564.5.patch (4.5 KB) - added by WraithKenny 12 years ago.
20564.4.diff (4.2 KB) - added by kovshenin 12 years ago.
20564.5.diff (2.5 KB) - added by kovshenin 12 years ago.
20564-6.patch (8.3 KB) - added by azaozz 12 years ago.
20564.6.diff (3.8 KB) - added by kovshenin 12 years ago.
20564.7.diff (4.0 KB) - added by kovshenin 12 years ago.
20564-8.patch (6.3 KB) - added by azaozz 12 years ago.
20564-9.patch (7.0 KB) - added by adamsilverstein 11 years ago.
reintroduce changes removed in 24397
20564-10.diff (11.7 KB) - added by adamsilverstein 11 years ago.
includes unit test
20564-11.diff (11.0 KB) - added by adamsilverstein 11 years ago.
remove unused post format meta values from default array and tests
20564-12.diff (11.1 KB) - added by adamsilverstein 11 years ago.
slight test cleanup
20564-13.diff (11.3 KB) - added by adamsilverstein 11 years ago.
20564-14.diff (9.1 KB) - added by adamsilverstein 11 years ago.
remove post format revisioning
20564.8.diff (9.9 KB) - added by DrewAPicture 11 years ago.
+ docs fixes
20564.9.diff (10.7 KB) - added by adamsilverstein 11 years ago.
Refresh against trunk. remove redundant restore block; update unit tests
20564.10.diff (11.2 KB) - added by adamsilverstein 11 years ago.
add wp_unslash to $_POST comparison
20564-autosave-data.png (148.4 KB) - added by TV productions 11 years ago.
The data that an autosave sent in wp 3.9-alpha-27445-src
20564.11.diff (11.2 KB) - added by adamsilverstein 11 years ago.
Refresh against trunk
20564.12.diff (9.9 KB) - added by adamsilverstein 11 years ago.
refresh against trunk
20564.13.diff (10.3 KB) - added by adamsilverstein 10 years ago.
refresh against trunk
20564.14.diff (12.9 KB) - added by adamsilverstein 10 years ago.
only save changed revisions
20564.15.diff (11.3 KB) - added by adamsilverstein 10 years ago.
don't save blank meta, track revisioned keys
20564.16.diff (12.0 KB) - added by adamsilverstein 10 years ago.
update unit test, restore revision with untracked key
20564.17.diff (12.9 KB) - added by adamsilverstein 10 years ago.
account for the storing of multiple values per key
20564.18.diff (12.9 KB) - added by adamsilverstein 10 years ago.
refresh against trunk
20564.19.diff (12.9 KB) - added by adamsilverstein 10 years ago.
refresh
20564-new-action.diff (537 bytes) - added by adamsilverstein 10 years ago.
hook for autosave
20564-new-action.2.diff (530 bytes) - added by adamsilverstein 10 years ago.
pass entire $new_autosave array
20564.20.diff (7.8 KB) - added by adamsilverstein 10 years ago.
add one filter, unit tests
20564.21.diff (8.2 KB) - added by adamsilverstein 10 years ago.
Adds hook docs
30033.diff (739 bytes) - added by mattheu 10 years ago.
20564.22.diff (8.7 KB) - added by mattheu 10 years ago.
20564.23.diff (2.3 KB) - added by mattheu 10 years ago.
20564.24.diff (48.7 KB) - added by adamsilverstein 16 months ago.
20564.25.diff (55.0 KB) - added by adamsilverstein 15 months ago.
20564.26.diff (55.3 KB) - added by adamsilverstein 15 months ago.
footnotes-from-gb.diff (6.7 KB) - added by adamsilverstein 15 months ago.
20564.27.diff (58.0 KB) - added by adamsilverstein 15 months ago.
20564.28.diff (62.9 KB) - added by adamsilverstein 15 months ago.
20564.29.diff (62.9 KB) - added by adamsilverstein 15 months ago.
20564.30.diff (62.9 KB) - added by adamsilverstein 15 months ago.
20564c.diff (4.7 KB) - added by adamsilverstein 14 months ago.

Download all attachments as: .zip

Change History (264)

#1 @scribu
13 years ago

  • Cc scribu added

#2 @toscho
13 years ago

  • Cc info@… added

#3 @rspindel
13 years ago

  • Cc rspindel added

#4 @jkudish
13 years ago

  • Cc joachim.kudish@… added

#5 @Ipstenu
13 years ago

  • Cc ipstenu@… added

#6 @kovshenin
13 years ago

  • Cc kovshenin@… added

#7 @hameedullah
13 years ago

  • Cc h@… added

#8 @ryanduff
13 years ago

  • Cc ryan@… added

#9 @maor
13 years ago

  • Cc maorhaz@… added

#10 @travisnorthcutt
13 years ago

  • Cc travis@… added

#11 @sc0ttkclark
13 years ago

  • Cc lol@… added

#12 @xyzzy
13 years ago

  • Cc dennen@… added

#13 @lightningspirit
13 years ago

  • Cc lightningspirit@… added

+1

#14 @swissspidy
12 years ago

  • Cc hello@… added

#15 @sennza
12 years ago

  • Cc bronson@… added

#16 @jaredatch
12 years ago

  • Cc jared@… added

#17 @bainternet
12 years ago

  • Cc admin@… added

#18 @jb510
12 years ago

  • Cc jbrown510@… added

#19 follow-up: @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.6

This and #20299 would block #23314.

#20 @johnbillion
12 years ago

This and #20299 also potentially block the no-JS fallback for #19570, although MarkJaquith mentioned the possibility of not allowing a post to switch formats after the fact if you have no JS.

Last edited 12 years ago by johnbillion (previous) (diff)

#21 in reply to: ↑ 19 ; follow-up: @adamsilverstein
12 years ago

Replying to nacin:

This and #20299 would block #23314.

will bring this back into scope, have you reviewed the code attached? if it looks reasonable i can work to apply.

#22 @adamsilverstein
12 years ago

  • Cc adamsilverstein@… added

#23 @goto10
12 years ago

  • Cc dromsey@… added

#24 in reply to: ↑ 21 @westi
12 years ago

Replying to adamsilverstein:

Replying to nacin:

This and #20299 would block #23314.

will bring this back into scope, have you reviewed the code attached? if it looks reasonable i can work to apply.

It's still outside the scope that the revisions team will primarily focus on.

#25 @kwight
12 years ago

  • Cc kwight@… added

#26 follow-up: @markjaquith
12 years ago

From johnbillion, a very simple take: https://gist.github.com/johnbillion/5225514

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

Replying to markjaquith:

From johnbillion, a very simple take: https://gist.github.com/johnbillion/5225514

i have a patch brewing for this ticket and will post for your review monday.

@adamsilverstein
12 years ago

for testing

#28 @adamsilverstein
12 years ago

20564.diff

  • adds new wp_save_post_revision_meta() - called in _wp_put_post_revision
  • adds wp_restore_post_revision_meta() - added in restore action
  • also stores post format as revision meta (and restores on revision restore), seemed to be best way to store post_format
  • requires http://core.trac.wordpress.org/attachment/ticket/16215/16215.9.diff to work correctly (stores meta on copy of current revision, only present after patch)
  • wp_revisions_keep_meta filter to override storing of meta
  • initial testing verified post format and related meta stored and restored correctly

20564+16215.diff​ combined patch for testing

@adamsilverstein
12 years ago

updated against current trunk

@adamsilverstein
12 years ago

update against current

#29 follow-up: @markjaquith
12 years ago

We need the exact opposite approach of this patch. :-)

We should have a list of meta keys that we WILL revision, not a list of ones we WON'T. There should be no filter to opt out of revisioning of post format postmeta.

#30 in reply to: ↑ 29 @adamsilverstein
12 years ago

Replying to markjaquith:

We need the exact opposite approach of this patch. :-)

We should have a list of meta keys that we WILL revision, not a list of ones we WON'T. There should be no filter to opt out of revisioning of post format postmeta.

ok, this was the method suggested in #20299 so i went with it. a possible advantage of this method is it will catch new meta options as they are added or used, but i could also see how it could catch fields you didn't need.

i can change the method and include all current used metas, maybe add a filter for plugin authors to register new metas for revisioning?

#31 @WraithKenny
12 years ago

  • Cc Ken@… added

#32 @WraithKenny
12 years ago

I think we just need a few new wrapper functions (like add_versioned_meta(), update_versioned_meta(), and delete_versioned_meta() ) that don't check against wp_is_post_revision(). Using these would "opt in" to metadata on revisions. (of course you could currently just use *_metadata('post'...) functions, no?)

then on 'wp_restore_post_revision' (with 2 args)

$meta = get_post_meta( $revision_id, $key );
if ( empty( $meta ) )
    delete_post_meta( $post_id, $key );
else
    update_post_meta( $post_id, $key, $meta );
// or whatnot.

which is pretty straight forward (and also "opt in").
Optionally, core can hook onto 'wp_restore_post_revision' and loop thru a filterable array of "versioned" meta so that the following could work:

add_filter( 'versioned_meta', 'my_versioned_meta' );
function my_versioned_meta( $meta_keys ) {
    $meta_keys[] = 'my_key';
    return $meta_keys;
}

#33 @WraithKenny
12 years ago

patch introduces add_versioned_meta(), update_versioned_meta(), and delete_versioned_meta() adds the filter 'versioned_meta' and handles restoring versioned meta data in wp_restore_post_revision() Lightly tested.

#34 @WraithKenny
12 years ago

  • Keywords has-patch needs-testing added

#35 follow-up: @markjaquith
12 years ago

I don't think we're ready to bless this for plugin use. As you noted, they can just use the lower-level functions if they know what they're doing. So for this go-around, I'd skip wrapping it up all pretty. Taking a look at your patch now.

#36 @WraithKenny
12 years ago

new patch adds get_versioned_meta_keys

#37 in reply to: ↑ 35 @WraithKenny
12 years ago

Replying to markjaquith:

I don't think we're ready to bless this for plugin use. As you noted, they can just use the lower-level functions if they know what they're doing. So for this go-around, I'd skip wrapping it up all pretty. Taking a look at your patch now.

Yeah, that's cool. I was just working on this because it blocks #20299 which in turn blocks a couple other interesting tickets like #23314 and #23539

Figured if I got a couple working patches up it might move those next round.

@kovshenin
12 years ago

#38 @kovshenin
12 years ago

Taking a stab at this in 20564.4.diff. The patch is quite self-explanatory.

@kovshenin
12 years ago

#39 @kovshenin
12 years ago

20564.5.diff is same as .4, except the *_post_meta functions hi-jacking. Uses the underlying add_metadata where applicable.

#40 @markjaquith
12 years ago

In 23859:

Revision our post format postmeta.

props kovshenin, WraithKenny. see #20564.

#41 @DrewAPicture
12 years ago

  • Keywords needs-codex added

See [23859]

Last edited 12 years ago by DrewAPicture (previous) (diff)

#42 follow-up: @azaozz
12 years ago

Been seeing this lately in the postmeta table, post IDs 9212, 9213 and 9214 are revisions and all meta is empty (no values):

4040 	9214 	_wp_format_video 	
4039 	9214 	_wp_format_audio 	
4038 	9214 	_wp_format_image 	
4037 	9214 	_wp_format_quote_source 	
4036 	9214 	_wp_format_quote 	
4035 	9214 	_wp_format_url 	
4032 	9213 	_wp_format_video 	
4031 	9213 	_wp_format_audio 	
4030 	9213 	_wp_format_image 	
4029 	9213 	_wp_format_quote_source 	
4028 	9213 	_wp_format_quote 	
4027 	9213 	_wp_format_url 	
4022 	9212 	_wp_format_video 	
4021 	9212 	_wp_format_audio 	
4020 	9212 	_wp_format_image 	
4019 	9212 	_wp_format_quote_source 	
4018 	9212 	_wp_format_quote 	
4017 	9212 	_wp_format_url 	

Seems we are doing isset() and should be doing ! empty() somewhere.

#43 @WraithKenny
12 years ago

its the if ( false === $meta_values ) line I think

@azaozz
12 years ago

#44 @azaozz
12 years ago

In 20564-6.patch:

  • When saving revisions (including autosaves): save the current post format as revision meta.
  • When restoring from a revision, restore the format too.
  • When saving revisions: add post format meta to the revision post only for the current post format, skip when empty.
Last edited 12 years ago by azaozz (previous) (diff)

#45 in reply to: ↑ 42 @kovshenin
12 years ago

Replying to azaozz: The original post contains all those meta keys with empty values as well, and the revision post is a complete copy, so I think that's the expected behavior. I also think that there should be a more general approach for all possible meta, rather than post formats only. Thoughts?

#46 @azaozz
12 years ago

The original post contains all those meta keys with empty values as well...

Not sure that's needed either.

...the revision post is a complete copy, so I think that's the expected behavior.

No, not a complete copy. It's a copy of the post data, most fields are not revisioned. By default only title, content and excerpt are. That's why we have _wp_post_revision_fields().

Imagine a small site with 100 posts, each having 10 revisions. That's (1,100 * 7) nearly 8,000 rows in the post meta table just for post formats. And most will be empty. Now imagine a bigger site with 20,000 posts and 50 revisions each (that's not uncommon). Nearly 8,000,000 rows. And now imagine a really big site... :)

I also think that there should be a more general approach for all possible meta, rather than post formats only.

Yeah, when we open the API to revision post meta (agree with @markjaquith it's out of scope at the moment), thinking it should be opt-in. A lot of the post meta doesn't need to be saved in revisions.

#47 @kovshenin
12 years ago

I'm pretty sure it's been mentioned somewhere before, but we can probably store all the revision meta in a single serialized _revision_meta key. I agree that adding keys to revisioned meta should be opt-in and that it's out of scope for 3.6, I just dislike the post-format-ish approach at _wp_post_revision_meta_keys(), and the $single assumption in get_post_meta :)

#48 follow-up: @azaozz
12 years ago

We could, but having separate meta has some advantages. We could do a query to select posts by these meta fields which might be useful in some cases. Also that makes post formats more "futureproof".

Yes, currently the revisions meta is only for post formats, and the fields can only be "single" there. It's not very hard to turn parts of it into an API, perhaps 3.7?

@kovshenin
12 years ago

#49 in reply to: ↑ 48 ; follow-up: @kovshenin
12 years ago

Replying to azaozz: I think what Aaron meant in his comment is useful for posts only, but not really useful for revisions. I took a stab at the _revision_meta approach in 20564.6.diff which should keep the meta table clean and small for revisions. If this works out we can do the same for revisioned terms (#23893). Not really looking to fix this ticket, but rather lay a good foundation for whatever is next in 3.7 and 3.8.

P.S. Sorry for the .6 vs -6 naming conflict, that's trac's auto-naming kicking in :)

#50 @WraithKenny
12 years ago

The only drawback I can think of for serializing all the meta into one field is size concerns. For formats, it shouldn't be a problem, but if it's opened up later as an API, what if some meta are very large? Potentially, the values would be saved into individual keys, but when combined they maybe too big to store... no? It's an edge case, but who knows.

#51 follow-up: @WraithKenny
12 years ago

For the unique key issue, if the key is actually unique, then get_post_meta( $revision['ID'], $meta_key ) should only have one value in the array making it unneeded to pass a unique flag to add_post_meta (since the loop only goes once). I had a unique param in my patch originally, but after I saw the commit I thought it unnecessary. Is there a different unique assumption that I'm missing?

#52 @travisnorthcutt
12 years ago

  • Cc travis@… removed

#53 in reply to: ↑ 49 ; follow-up: @azaozz
12 years ago

Replying to kovshenin:

I took a stab at the _revision_meta approach in 20564.6.diff which should keep the meta table clean and small for revisions. If this works out we can do the same for revisioned terms (#23893).

Yeah, this works for post formats, not so sure for all meta (only need to add restoring of the post format from 20564-6.patch, perhaps to the same array). Still, having empty keys in the serialized array seems wasteful :)

As long as we know the post supports formats, we can recreate the empty keys programmatically, no need to store them in the DB. Example:

$all_meta = array_merge(
  array(
	'_wp_format_url' => '',
	'_wp_format_quote' => '',
	'_wp_format_quote_source' => '',
	...
  ),
  get_post_meta( $revision->ID, '_revision_meta', true )
);

This brings another interesting question: when restoring from a revision, do we overwrite all data stored in the main post's formats meta? If yes, do empty meta keys from the revision overwrite (i.e. remove) the corresponding values on the main post?

User case:

  • User creates a post, no format set.
  • User saves a draft creating a revision.
  • User edits the post, sets format to quote, enters data in 'url' and 'quote' meta fields.
  • User restores to the first revision. At this point do we delete the data entered in 'url' and 'quote' on the main post or do we keep it? Logically we should delete it, but if we are keeping all post formats data ever entered on the main post, the user may expect to find it if he sets the format to quote again.
Last edited 12 years ago by azaozz (previous) (diff)

#54 in reply to: ↑ 51 @azaozz
12 years ago

Replying to WraithKenny:

...if the key is actually unique, then get_post_meta( $revision['ID'], $meta_key ) should only have one value in the array making it unneeded to pass a unique flag to add_post_meta.

True, but we will need to do $meta[0] everywhere. In the current code post formats meta doesn't support multiple keys, better to use the meta API properly and get/set a single key.

#55 in reply to: ↑ 53 @kovshenin
12 years ago

Replying to azaozz:

Only need to add restoring of the post format from 20564-6.patch, perhaps to the same array.

That's taxonomy and terms, not post meta, hence the similar patch in #23893 for taxonomy. If the the serialized _revision_meta approach works well for meta, we can take a similar approach with terms and not "pollute" the term_relationships table with relations to revisions :)

Still, having empty keys in the serialized array seems wasteful :)

Maybe. We can indeed check the emptiness of meta before storing their revisions to save some space, but I have mixed feelings about it. I don't think it will affect post formats all that much, but I keep thinking that a non-existent meta value is not the same as a stored empty string, especially for multiple values under one key. If there was a $default argument to get_post_meta like there is for get_option, not storing the empty values would be a deal-breaker. So I lean towards doing delete_post_meta for empty strings when saving post format metadata. Just thinking out loud here.

This brings another interesting question: when restoring from a revision, do we overwrite all data stored in the main post's formats meta? If yes, do empty meta keys from the revision overwrite (i.e. remove) the corresponding values on the main post?

I can try and answer this with a similar question :) When restoring from a revision, do empty fields (title or content) overwrite the corresponding values in the main post? Create a post with no title, save a few revisions, give it a title, save and restore a previous revision: do we set the title back to an empty string? The answer is yes, and I think post formats (and revisioned post meta in general) should not be any different.

@kovshenin
12 years ago

#56 @kovshenin
12 years ago

In 20564.7.diff:

  • Merged some of the things from Andrew's patch
  • Switched to single value lookups
  • Save and retrieve post format via a _revision_post_format meta
  • Filters get_the_terms to show the revisioned post format during previews
  • Doesn't store empty meta values for revisions

This also covers #23893

#57 @azaozz
12 years ago

We can indeed check the emptiness of meta before storing their revisions to save some space, but I have mixed feelings about it.

What I mean is at the lowest level: we know the expected keys, so array( 'a' => '', 'b' => '', 'c' => 'something' ); doesn't have to be serialized, saved to the db and unserialized again when getting the data. We can remove the empty keys, save array( 'c' => 'something' ); and then add the empty keys on getting the data. This will work in exactly the same way as saving the original array.

The same is true when getting/setting the post format meta on revision posts. We know there are 7 meta keys, no need to add the empty.

I can try and answer this with a similar question :) When restoring from a revision, do empty fields (title or content) overwrite the corresponding values in the main post?

That's not exactly the same. From the discussion on #19570, we are keeping the unused data for post formats. Basically this is irrelevant post meta unless the user decides to switch back to a previously used post format. The question here is: do we "roll back" that irrelevant data when restoring a revision, and yes, logically we should.

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

@azaozz
12 years ago

#58 @azaozz
12 years ago

20564-8.patch builds on 20564.7.diff​ and adds updating of post format and post format meta for autosaves.

#59 @kovshenin
12 years ago

20564-8.patch looks good and works like a charm!

#60 @azaozz
12 years ago

In 23928:

Revisions:

  • Store the post format as meta on revisions (including autosaves).
  • Add post formats data (post meta) when autosaving.
  • Only add non-empty post formats data to revisions.
  • Correct the post format when previewing a published post.

Props kovshenin, see #19570, see #20564.

#61 @kovshenin
12 years ago

  • Keywords has-patch removed

@azaozz thanks for the commit! One little thing I noticed is the @since version in the docblock of the new _wp_preview_terms_filter function ([23928]). It says 2.6 but should be 3.6. I mistyped that in my patch, sorry!

#62 @ryan
12 years ago

[23936] for @since update.

#63 @adamsilverstein
12 years ago

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

#64 @helen
11 years ago

This is so not fixed. Re-open or make new?

#65 follow-up: @alexkingorg
11 years ago

This ticket seemed to get oddly intertwined with the Post Formats data - perhaps a new ticket would allow it to better exist as a stand-alone framework feature?

#66 @adamsilverstein
11 years ago

  • Keywords needs-refresh added
  • Resolution fixed deleted
  • Status changed from closed to reopened

i think the code from 23928 will work will work as is. re-opening and will refresh the patch against trunk for testing.

Version 0, edited 11 years ago by adamsilverstein (next)

#67 @SergeyBiryukov
11 years ago

  • Milestone changed from 3.6 to Future Release

@adamsilverstein
11 years ago

reintroduce changes removed in 24397

#68 @adamsilverstein
11 years ago

20564-9.patch​ first pass at reintroducing changes removed in [24397].

some small changes:

  • updated selector in autosave.js to current ui selector scheme - seems like a JS hook here would make sense, so plugin developers could use their own UI scheme for post formats
  • includes fix from #24477 (from itthinx)
Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#69 @jgwspud
11 years ago

  • Cc jgwspud added

#70 @c3mdigital
11 years ago

#23973 was marked as a duplicate.

#71 @doughamlin
11 years ago

  • Cc doughamlin@… added

#72 @doughamlin
11 years ago

  • Cc doughamlin@… removed

#73 @doughamlin
11 years ago

  • Cc doughamlin@… added

#74 in reply to: ↑ 65 @WraithKenny
11 years ago

Replying to alexkingorg:

This ticket seemed to get oddly intertwined with the Post Formats data - perhaps a new ticket would allow it to better exist as a stand-alone framework feature?

Indeed, now that post-formats has been tabled, this ticket should revert to it's original intent, a General Framework for saving and restoring metadata from revisions. (Implementation shouldn't focus on post-format data.)

Some notes and considerations collected from various places:

  1. Patches derived from the existing, but removed code, should be abstracted to allow for general use.
  2. Autosaves/Previews needed to be considered: see #20299 #23539
  3. This could enable the feature here: #23314

#75 @sillybean
11 years ago

  • Cc steph@… added

#76 @donutz
11 years ago

  • Cc peterherrel@… added

#77 @jgwspud
11 years ago

  • Cc jgwspud removed

@adamsilverstein
11 years ago

includes unit test

#78 @adamsilverstein
11 years ago

lets revision some post meta!
20564-10.diff​ includes a unit test to verify the meta revisioning and filter functionality as well as improved inline hook docs.

@adamsilverstein
11 years ago

remove unused post format meta values from default array and tests

#79 @adamsilverstein
11 years ago

20564-11.diff​ - removes unused post format meta values - now revisions only 'post_format' itself, plus any meta values added via the wp_post_revision_meta_keys filter

@adamsilverstein
11 years ago

slight test cleanup

#80 @adamsilverstein
11 years ago

20564-13.diff​:

  • refresh against trunk
  • added unit test for revisioning of post_format
  • keeping post_format revisioning since post_format a default post meta field

@adamsilverstein
11 years ago

remove post format revisioning

#81 @adamsilverstein
11 years ago

20564-14.diff​:

  • remove all traces of post formats, no more post format revisioning

@DrewAPicture
11 years ago

+ docs fixes

#82 @DrewAPicture
11 years ago

  • Keywords needs-refresh removed
  • Version set to 3.4

20564.8.diff implements docs fixes for 20564-14.diff as requested.

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


11 years ago

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


11 years ago

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


11 years ago

@adamsilverstein
11 years ago

Refresh against trunk. remove redundant restore block; update unit tests

#87 @adamsilverstein
11 years ago

in 20564.9.diff: refreshed against trunk; removed what appears to be redundant meta restore block; updated unit test.

@adamsilverstein
11 years ago

add wp_unslash to $_POST comparison

#88 @adamsilverstein
11 years ago

  • Keywords dev-feedback added

20564.10.diff corrects issue johnbillion pointed out in IRC - $_POST requires wp_unslash in wp_create_post_autosave or the comparison may fail.

This ticket was mentioned in IRC in #wordpress-dev by adams|mobile. View the logs.


11 years ago

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


11 years ago

#91 follow-up: @TV productions
11 years ago

For the ability for plugin devs to add custom meta to custom post type revisions (#13382), you do also need a kind of javascript "filter" for the data that is send with the autosave. Something like this http://jsfiddle.net/jTjzK/3/ (just a concept to extend the data var).

#92 in reply to: ↑ 91 ; follow-up: @adamsilverstein
11 years ago

Replying to TV productions:

For the ability for plugin devs to add custom meta to custom post type revisions (#13382), you do also need a kind of javascript "filter" for the data that is send with the autosave. Something like this http://jsfiddle.net/jTjzK/3/ (just a concept to extend the data var).

I'm not sure this is required. All fields should be 'sent' with the auto update post, and the attached patch filters these fields and revisions ones that are specified. All fields are sent, and the patch allows authors to specify which fields to revision, Does that make sense? If not can you please explain a bit more what you would want to revision that the current patch would not work for?

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


11 years ago

@TV productions
11 years ago

The data that an autosave sent in wp 3.9-alpha-27445-src

#94 in reply to: ↑ 92 ; follow-up: @TV productions
11 years ago

Replying to adamsilverstein:

Replying to TV productions:

For the ability for plugin devs to add custom meta to custom post type revisions (#13382), you do also need a kind of javascript "filter" for the data that is send with the autosave. Something like this http://jsfiddle.net/jTjzK/3/ (just a concept to extend the data var).

I'm not sure this is required. All fields should be 'sent' with the auto update post, and the attached patch filters these fields and revisions ones that are specified. All fields are sent, and the patch allows authors to specify which fields to revision, Does that make sense? If not can you please explain a bit more what you would want to revision that the current patch would not work for?

Just to check, I've pulled the latest code form the repo and applied the latest patch (20564.10.diff).
As you can see in the attached image The data that an autosave sent in wp 3.9-alpha-27445-src the default meta fields are saved.

But what I would like to see is a way to "prepaire" the data that will be saved into the meta field. The post type in the image is a photo album, and instead of "normal" content it saves some image data and the server generates the post_content from this meta field. So I don't have one html input field that matches an meta field, but a lot. There are different solutions to this: (1) create a field that does matches the meta field and that is updated by javascript on autosave (so you need a hook for that) or (2) a "jsfilter" to generate the data and pass it trough.

As I write this, I note that it is kinda focussed to my example, but what I actually would say is: could we make an autosave API where you can add data on client side and extract (and save) it on the server side?

#95 in reply to: ↑ 94 ; follow-up: @adamsilverstein
11 years ago

Replying to TV productions:

Replying to adamsilverstein:

Replying to TV productions:

For the ability for plugin devs to add custom meta to custom post type revisions (#13382), you do also need a kind of javascript "filter" for the data that is send with the autosave. Something like this http://jsfiddle.net/jTjzK/3/ (just a concept to extend the data var).

I'm not sure this is required. All fields should be 'sent' with the auto update post, and the attached patch filters these fields and revisions ones that are specified. All fields are sent, and the patch allows authors to specify which fields to revision, Does that make sense? If not can you please explain a bit more what you would want to revision that the current patch would not work for?

Just to check, I've pulled the latest code form the repo and applied the latest patch (20564.10.diff).
As you can see in the attached image The data that an autosave sent in wp 3.9-alpha-27445-src the default meta fields are saved.

But what I would like to see is a way to "prepaire" the data that will be saved into the meta field. The post type in the image is a photo album, and instead of "normal" content it saves some image data and the server generates the post_content from this meta field. So I don't have one html input field that matches an meta field, but a lot. There are different solutions to this: (1) create a field that does matches the meta field and that is updated by javascript on autosave (so you need a hook for that) or (2) a "jsfilter" to generate the data and pass it trough.

As I write this, I note that it is kinda focussed to my example, but what I actually would say is: could we make an autosave API where you can add data on client side and extract (and save) it on the server side?

Thank you for the more detailed description and the screenshot.

I think what you are looking for is a way to hook in right before the autosave, or a filter applied to the transmitted autosave data. This type of JavaScript hook or action is currently not standardized in WordPress - see ticket #21170 for more details and a proposed solution.

In the mean time you may be able to hook in using this:

$( document ).on( 'before-autosave', function() { /* Your Function */ } );

(based on this code)

@adamsilverstein
11 years ago

Refresh against trunk

#96 in reply to: ↑ 95 @TV productions
11 years ago

Replying to adamsilverstein:

Replying to TV productions:

Replying to adamsilverstein:

Replying to TV productions:

[...]

[...]

[...]

Thank you for the more detailed description and the screenshot.

I think what you are looking for is a way to hook in right before the autosave, or a filter applied to the transmitted autosave data. This type of JavaScript hook or action is currently not standardized in WordPress - see ticket #21170 for more details and a proposed solution.

In the mean time you may be able to hook in using this:

$( document ).on( 'before-autosave', function() { /* Your Function */ } );

(based on this code)

Okay, thanks for pointing me in the right direction and for the jQuery hook. I am going to dig into #21170.

Last edited 11 years ago by TV productions (previous) (diff)

@adamsilverstein
11 years ago

refresh against trunk

#97 @adamsilverstein
11 years ago

20564.12.diff refresh against trunk, verified unit tests still pass.

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


11 years ago

#99 @helen
10 years ago

#28553 was marked as a duplicate.

@adamsilverstein
10 years ago

refresh against trunk

#100 @adamsilverstein
10 years ago

20564.13.diff - Refresh against trunk, slightly updated

  • Working on a version that only saves changed revision meta values

@adamsilverstein
10 years ago

only save changed revisions

#101 @adamsilverstein
10 years ago

20564.14.diff

  • Only save metas that have changed
  • Keep a serialized array of the 'whitelisted' (revisioned) meta keys
  • When restoring a revision, traverse back thru past revisions to find the last time the meta was revisioned

While this patch works and the unit tests pass, it seems a little too complicated. I'm going to work on this a bit more - keeping the serialized array of the 'whitelisted' (revisioned) meta keys, but switching to only saving non-blank meta values. This will eliminate the complicated traversal back to find the previously stored meta. We will still be creating rows for each (non-blank) meta which I think is ok as long as we make that fact clear to developers.

@adamsilverstein
10 years ago

don't save blank meta, track revisioned keys

#102 follow-up: @adamsilverstein
10 years ago

In 20564.15.diff:

  • Don't store blank meta values
  • Track meta keys stored for each revision
  • Updated unit test to test restoring blank meta

@adamsilverstein
10 years ago

update unit test, restore revision with untracked key

@adamsilverstein
10 years ago

account for the storing of multiple values per key

#103 @adamsilverstein
10 years ago

in 20564.17.diff - account for multiple meta values per key, added test for this, deserves more testing

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


10 years ago

#105 in reply to: ↑ 102 ; follow-up: @p51labs
10 years ago

Replying to adamsilverstein:

In 20564.15.diff:

  • Don't store blank meta values
  • Track meta keys stored for each revision
  • Updated unit test to test restoring blank meta

I think that blank meta should be stored if exists on the current post. Sometimes a blank value is the intended value. A case an point would be a field with checkboxes, by setting a blank value I know they purposely didn't select anything from the list.

Thoughts?

Last edited 10 years ago by p51labs (previous) (diff)

#106 in reply to: ↑ 105 @adamsilverstein
10 years ago

Thanks for the feedback!

The issue is that when retrieving meta, WordPress returns blank if the meta is not set, OR if the meta is blank - so not storing the meta is the same as storing blank meta :) (unless I'm missing something)

the trick here is that because I am tracking which metas are 'revisioned' for each stored revision, i know that a meta that is revisioned and for which no value was stored is in fact blank; therefore, when restoring a revision with a meta that is missing (but which was tracked), that meta is considered blank, and the restored value is set as blank (guess I could just delete it).

Does that make sense? in short, I am tracking blank metas, just not bothering to store them.

Replying to p51labs:

Replying to adamsilverstein:

In 20564.15.diff:

  • Don't store blank meta values
  • Track meta keys stored for each revision
  • Updated unit test to test restoring blank meta

I think that blank meta should be stored if exists on the current post. Sometimes a blank value is the intended value. A case an point would be a field with checkboxes, by setting a blank value I know they purposely didn't select anything from the list.

Thoughts?

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

#107 follow-up: @p51labs
10 years ago

Let me expand a little on my previous example. Let's say I have 3 checkboxes, all for one meta_key and the values are 1,2,3. Now by default in my meta box I set 1,3 as checked. Now a user comes in and creates a post and decides none of them should be checked. Now not storing the data is accurate however it doesn't tell me that the user unchecked anything so when I render the field again do I assume I need to load the default values or now leave them all unchecked? I suppose a I could check whether or not the post is new or being edited but is that the best approach?

This solution requires checking for the meta and if the meta key exists.

Just throwing in some thoughts, thanks for taking the time to consider them.

#108 in reply to: ↑ 107 @adamsilverstein
10 years ago

I think you can solve your problem using metadata_exists to check to the presence of the meta data. I don't think this issue affects the revisioning of post meta, deserves some testing!

Replying to p51labs:

Let me expand a little on my previous example. Let's say I have 3 checkboxes, all for one meta_key and the values are 1,2,3. Now by default in my meta box I set 1,3 as checked. Now a user comes in and creates a post and decides none of them should be checked. Now not storing the data is accurate however it doesn't tell me that the user unchecked anything so when I render the field again do I assume I need to load the default values or now leave them all unchecked? I suppose a I could check whether or not the post is new or being edited but is that the best approach?

This solution requires checking for the meta and if the meta key exists.

Just throwing in some thoughts, thanks for taking the time to consider them.

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


10 years ago

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


10 years ago

@adamsilverstein
10 years ago

refresh against trunk

@adamsilverstein
10 years ago

refresh

#111 follow-up: @adamsilverstein
10 years ago

20564.19.diff

  • refresh against trunk
  • verify unit tests pass

#112 in reply to: ↑ 111 @Andrew Moawad
10 years ago

Last edited 10 years ago by Andrew Moawad (previous) (diff)

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


10 years ago

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


10 years ago

@adamsilverstein
10 years ago

hook for autosave

#115 @adamsilverstein
10 years ago

I have refactored the latest code on this ticket as a plugin - https://github.com/adamsilverstein/wp-20564

Only one additional hook is required to complete functionality (and only for the or the autosaving of post meta), I added that in 20564-new-action.diff

I haven't tested the plugin carefully yet, my next step is getting the existing unit tests to pass.

@adamsilverstein
10 years ago

pass entire $new_autosave array

@adamsilverstein
10 years ago

add one filter, unit tests

#116 @adamsilverstein
10 years ago

One more filter was required: on my list to document these new hooks/filters, the check for change wasn't quite right.

With 20564.20.diff and the plugin ( https://github.com/adamsilverstein/wp-20564 ) metas are revisioned.

I wasn't certain how to run the unit tests when a plugin is required, I found that dropping a copy of wp-20564.php in trunk/tests/phpunit/tests/post/ did the trick for me, with that in place and the patch, all tests pass.

@adamsilverstein
10 years ago

Adds hook docs

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


10 years ago

#119 @mattheu
10 years ago

I have tested this plugin & related patch and its working well.

I think it is highly desirable for the revisioned meta fields to be included in the revision diff UI.

On this related ticket: #29920 I added a filter that allows revisioned meta fields to be added here. I am updating the patch on this ticket to include this.

I have also made a new version of the plugin to handle this. I will open a PR on github with these changes.

UPDATE - here is the pull request on github: https://github.com/adamsilverstein/wp-20564/pull/2

Last edited 10 years ago by johnbillion (previous) (diff)

@mattheu
10 years ago

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


10 years ago

@mattheu
10 years ago

#121 @mattheu
10 years ago

Added the correct patch!

Also thought about the idea of using the current register_meta function - which supports an $args param to register meta keys for revisions.

#122 follow-up: @johnbillion
10 years ago

  • Milestone changed from Future Release to 4.1

After some discussion at WCSF, moving to 4.1 for the temporary hooks suggested by adamsilverstein and matt_eu above.

#123 in reply to: ↑ 122 @mattheu
10 years ago

Replying to johnbillion:

After some discussion at WCSF, moving to 4.1 for the temporary hooks suggested by adamsilverstein and matt_eu above.

johnbillion asked me (https://wordpress.slack.com/archives/core/p1414551155002002) to split up the unit tests - however these tests are actually related to the plugin that uses the new hooks and should probably not belong in core. I've updated the patch to remove them.

I will open a PR on github to add the tests to the plugin.

Also added some more documentation.

@mattheu
10 years ago

#124 @johnbillion
10 years ago

In 30091:

Introduce some actions and filters which aid plugins in revisioning post meta.

  • wp_save_post_revision_post_has_changed filter which can be used to determine if a post has been changed, and therefore if a revision should be created for a post.
  • wp_get_revision_ui_diff filter which can be used to filter the fields displayed in the post revision diff UI.
  • wp_creating_autosave action which is fired just before an autosave is created.

See #20564.
Props mattheu, adamsilverstein.

#125 @johnbillion
10 years ago

  • Keywords needs-testing needs-codex dev-feedback removed
  • Milestone changed from 4.1 to Future Release

#126 @adamsilverstein
10 years ago

I have bundled up the code for this feature and released to the .org repository, requires trunk or 4.1: https://wordpress.org/plugins/wp-post-meta-revisions/

Please test! Issues and pull requests welcome on the GitHub repo.

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


10 years ago

#128 @adamsilverstein
9 years ago

  • Keywords dev-feedback added
  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

This ticket was mentioned in Slack in #core-fields by khromov. View the logs.


9 years ago

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


9 years ago

This ticket was mentioned in Slack in #forums by netweb. View the logs.


7 years ago

#132 @brothman01
6 years ago

Obviously this is an old ticket but I agree that this would be a good feature too

#133 @adamsilverstein
6 years ago

@brothman01 thanks! it is still a valid ticket. please test the plugin if you need this feature.

#134 @chrissiq
6 years ago

@adamsilverstein, there are a lot of bug fixes available on the plugin on github as PRs, but none of them are getting resolved and merged into master. I had to create my own fork of the repo and merge in a lot of PRs, fix bugs, etc, in order to get the plugin to work. This isn't something the community can do, the maintainer (you?) needs to review, or set reviewers, and approve the PRs I guess? Is the github repo at parity with the plugin download on the wordpress site?

#135 @adamsilverstein
6 years ago

@chrissiq

Apologies for my lack of activity on maintaining the plugin, I hope to loop back to it soon to review and merge any PRs that are good. I welcome any help in form of reviewing PRs, writing tests, etc.

The github repo is at parity with the plugin in .org, when I get to merging the outstanding PRs I will update it as well.

#136 @chrissiq
6 years ago

Thanks @adamsilverstein !

#137 @brothman01
6 years ago

I am using WordPress 5.1 and CMB2 to add the fields.

I am on WordPress 5.1 and installed the plugin from the repo, added the code snippet to my functions and changed the key to the real key. Should I do anything else to set this up or should it now be work but it's not?

#138 @adamsilverstein
6 years ago

@brothman01 that is how it should work. are you sure it isn't working? the meta will not show up in the revisions UI, but will be restored when you restore the revision it was attached to.

#139 @kirasong
17 months ago

A quick note that working around this for the Editor Footnotes Block is being discussed in this issue:
https://github.com/WordPress/gutenberg/issues/52541

This ticket was mentioned in PR #4859 on WordPress/wordpress-develop by @adamsilverstein.


17 months ago
#140

  • Keywords has-patch added

#142 @adamsilverstein
17 months ago

  • Milestone changed from Future Release to 6.4

@mikeschroder thanks for the ping...

Now that we are using meta directly in core for storing footnotes, we can re-consider landing support for revisioning of meta directly in core.

I took the existing code from the plugin and attempted to merge that back into core at the appropriate places in https://github.com/WordPress/wordpress-develop/pull/4859

This is a first pass but should be close to ready to test with footnotes.

To test:

  • Create a post with footnotes then change several times changing footnote each time... check
  • saving and revisioning of footnotes, restored when restoring post
  • saving of footnotes into autosaves (pause until autosave, close window ignoring window, re-open and restore autosave)
  • footnote updates showing correctly in previews (without publishing first)

What is missing?

@chrissiq it would be great if you could review/test this as well since you are familiar with fixes on the repo.

@Mamaduka commented on PR #4859:


17 months ago
#143

@adamsilverstein, should we try using register_meta for enabling post meta revisions instead of wp_post_revision_meta_keys filter?

@adamsilverstein commented on PR #4859:


17 months ago
#144

@adamsilverstein, should we try using register_meta for enabling post meta revisions instead of wp_post_revision_meta_keys filter?

Sure, good idea - I'll give it a try

@adamsilverstein commented on PR #4859:


16 months ago
#145

@adamsilverstein, should we try using register_meta for enabling post meta revisions instead of wp_post_revision_meta_keys filter?

@Mamaduka have it working way in the latest version (also filterable)

#146 @adamsilverstein
16 months ago

20564.24.diff includes the latest changes from the PR:

  • Revisoining can be enabled in register_post_meta or using a filter
  • Revisioned meta are handled in autosaves (needs more tests)
  • Revisioned meta are restored properly
  • The footnote revisions are shown clearly in the revisions screen (code belongs in Gutenberg and is included here for testing only)
  • Other revisioned meta is also shown on the revisions screen (could use additional testing)
  • REST API posts endpoint unhooks meta save until meta has been set
  • REST API revisions endpoint includes (read only) meta data
  • Extensive tests

This could use testing for:

  • Custom post type meta revisioning, setting and retrieving in the REST API
  • Storing and restoring multiple meta values on a single key
  • Autosaves and previews work correctly for meta data, including updated footnotes showing properly in previews
  • Also: now that this is in place, we could consider revisioning the post_thumbnail id so we could stop using the current fragile query parameter approach currently used in previews.

#147 @oglekler
16 months ago

  • Keywords needs-testing added

@ellatrix commented on PR #4859:


16 months ago
#148

Do we still have GB e2e tests in core? If they pass with this code, it's an instant approval for me.

@adamsilverstein commented on PR #4859:


15 months ago
#149

Do we still have GB e2e tests in core? If they pass with this code, it's an instant approval for me.

I'm not sure and will investigate. I should also be able to test locally by running the GB tests against core built with this patch.

@adamsilverstein commented on PR #4859:


15 months ago
#150

I added additional tests for pages and a custom post type, final steps would be manual testing as well as getting Gutenberg E2E testing run to test this out.

@Mamaduka commented on PR #4859:


15 months ago
#151

I manually ran the Footnote block e2e test using the patch from https://github.com/WordPress/gutenberg/pull/52988 and this branch. Two tests related to revisions are failing.

## Screenshot
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/240569/3bab7fea-2d7e-4010-9680-23ed267a94a7

@ellatrix commented on PR #4859:


15 months ago
#152

Thanks for running the tests!

@adamsilverstein commented on PR #4859:


15 months ago
#153

I manually ran the Footnote block e2e test using the patch from WordPress/gutenberg#52988 and this branch. Two tests related to revisions are failing.

Excellent, thank you!

I will try to reproduce the test steps manually to see what is happening. i'll also manually test all the features.

can you provide or point to instructions for how you ran the tests this way? i can build core and GB, but i'm not sure how to change the container GB uses for test runs?

@Mamaduka commented on PR #4859:


15 months ago
#154

can you provide or point to instructions for how you ran the tests this way

  1. Clone the Gutenberg as a plugin in WP trunk installation.
  2. Switch to the related branch in Gutenberg - https://github.com/WordPress/gutenberg/pull/52988 and build.
  3. Switch to the related branch in the trunk and build.
  4. Make sure that the WP trunk installation has a sample admin user (admin/password).
  5. Run the following command in the Gutenberg plugin directory - WP_BASE_URL=http://trunk.test/ npm run test:e2e:playwright -- test/e2e/specs/editor/various/footnotes.spec.js
  6. The http://trunk.test/ should point to your local install. It can be localhost as well.

@adamsilverstein commented on PR #4859:


15 months ago
#155

Thanks! I'll get these running and fixed.

@ellatrix commented on PR #4859:


15 months ago
#156

Worth nothing Beta 1 is on 26 September, and the change in GB need to be done in the release before that. I'm just hoping this makes it in 6.4! :)

@adamsilverstein commented on PR #4859:


15 months ago
#157

Worth nothing Beta 1 is on 26 September, and the change in GB need to be done in the release before that. I'm just hoping this makes it in 6.4! :)

Working on this again now!

@Mamaduka I'll work on testing manually; when I try running the tests in GB I'm stuck with an error:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/2676022/59528acb-8c76-4932-8d4b-b2a75c9d705e

Any tips to get past that?

@Mamaduka commented on PR #4859:


15 months ago
#158

@adamsilverstein, you'll need to copy `disable-animations.php` plugin.

@adamsilverstein commented on PR #4859:


15 months ago
#159

@Mamaduka thanks that fixed it for me and I was able to get the tests running. In headed mode I observed that the preview menu wasn't closing properly causing the next click to fail. I added a context click back to the first paragraph in WordPress/gutenberg@`59bc54f` (#52988) and the tests passed for me after that.

Can you double check to verify tests all pass on your end?

I'm going to do some manual "smoke testing" to validate revisioning works correctly, then I think this is ready for commit!

#160 @adamsilverstein
15 months ago

20564.25.diff is a refresh against trunk and includes the latest code from the PR.

#161 @adamsilverstein
15 months ago

  • Keywords commit needs-dev-note added; dev-feedback needs-testing removed

This is in a good place so I am marking for commit.

@adamsilverstein commented on PR #4859:


15 months ago
#162

I did some manual testing as well, similar to the e2e test that was failing:

https://github.com/WordPress/wordpress-develop/assets/2676022/24134ae9-c245-43d7-b99c-83b07092125f

and a more extended session where I also restored a previous revision

https://github.com/WordPress/wordpress-develop/assets/2676022/e78112e1-be29-4571-a7ab-618d5832c801

everything worked as expected.

@adamsilverstein commented on PR #4859:


15 months ago
#163

I did some manual testing as well, similar to the e2e test that was failing. These screencasts capture the tests:

Note: tests run with this patch active as well as https://github.com/WordPress/gutenberg/pull/52988

https://github.com/WordPress/wordpress-develop/assets/2676022/24134ae9-c245-43d7-b99c-83b07092125f

and a more extended session where I also restored a previous revision

https://github.com/WordPress/wordpress-develop/assets/2676022/e78112e1-be29-4571-a7ab-618d5832c801

everything worked as expected.

@Mamaduka commented on PR #4859:


15 months ago
#164

Thank you, @adamsilverstein!

The can be previewed when published test is still failing for me, and I can reproduce the issue when testing manually.

I am unsure why this is happening, but the first autosave (generated when clicking the preview button) doesn't save changes to the footnotes meta. After I click preview a second time, all consecutive changes are correctly saved.

@adamsilverstein commented on PR #4859:


15 months ago
#165

Hmm. Was passing for me although I did make one additional change. I will give that test another try; thanks for testing

@adamsilverstein commented on PR #4859:


15 months ago
#166

Hmm, all tests pass for me - maybe something different in our environments? I'll try running all the tests.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/2676022/d8ffd098-3868-4804-bc95-1069bcc14e90

The can be previewed when published test is still failing for me, and I can reproduce the issue when testing manually.

I am unsure why this is happening, but the first autosave (generated when clicking the preview button) doesn't save changes to the footnotes meta. After I click preview a second time, all consecutive changes are correctly saved.

I will try manually running through the steps to see if I can reproduce. Are you looking in the database or at the initial revision on the revisions screen - could it be an autosave?

@Mamaduka commented on PR #4859:


15 months ago
#167

Odd. It could be something on my local installation.

Can you double-check by following the steps from the test manually?

@adamsilverstein commented on PR #4859:


15 months ago
#168

Can you double-check by following the steps from the test manually?

I did and it seemed to work fine.

Along the way, however, I discovered a couple of issues:

  • The footnotes file here had been updated with some additional hooks since my last updated; I have a new commit to wrap those in conditionals as they are no longer needed
  • I noticed a bug that is described in https://core.trac.wordpress.org/ticket/20299 - namely that post meta values on the live post are changed when previewing meta changes! I'm going to check to see if this is also true in WP 6.3, I suspect it may be.

@adamsilverstein commented on PR #4859:


15 months ago
#169

I verified the published post previewing meta bug already exists - still it would be good to fix this!

Testing on a vanilla 6.3.1 site with the latest GB plugin installed, I verified that previewing changes to post meta also writes that meta to the published post.

Here is a screencast, note that I publish the post, change the meta, then click preview - I don't ever update the post; nonetheless, the live post meta (footnotes) are updated:

https://github.com/WordPress/wordpress-develop/assets/2676022/ce605e17-597f-4b8d-8538-704c0e062fc8

@adamsilverstein commented on PR #4859:


15 months ago
#170

Testing in core with the footnote.php code updated with the latest from https://github.com/WordPress/gutenberg/pull/52988 resolves the overwriting of the live post meta, so I think this is a separate but that is introduced by one of the filters there.

@adamsilverstein commented on PR #4859:


15 months ago
#171

Odd. It could be something on my local installation.

when manually testing, you need to copy the changes over from Gutenberg footnote block to core footnote.php - activating the plugin isn't enough to remove the current GB behavior.

@Mamaduka commented on PR #4859:


15 months ago
#172

I re-run the tests using the following method:

  1. I deactivated Gutenberg on my trunk installation.
  2. Copied PHP changes from https://github.com/WordPress/gutenberg/pull/52988 into src/wp-includes/blocks/footnotes.php.
  3. Run unit tests using - WP_BASE_URL=http://trunk.test/ npm run test:e2e:playwright -- test/e2e/specs/editor/various/footnotes.spec.js

## Result

All tests pass except the last assertion in can be previewed when published:

// Note: quote will get curled by wptexturize.
await expect(
        previewPage2.locator( 'ol.wp-block-footnotes li' )
).toHaveText( '123″  ↩︎' );

Here's the trace archvie from my local run; it can be viewed via https://trace.playwright.dev/.

activating the plugin isn't enough to remove the current GB behavior.

We'll need to temporarily remove old callbacks in the plugin before changes from https://github.com/WordPress/gutenberg/pull/52988 are synced with the core.

This ticket was mentioned in PR #5206 on WordPress/wordpress-develop by @adamsilverstein.


15 months ago
#173

  • Keywords has-unit-tests added

Trac ticket:

@adamsilverstein commented on PR #4859:


15 months ago
#174

I re-run the tests using the following method:


Thanks for testing.

Odd that test fails for you, it works for me!

I wasn't able to open the trace. If you try the steps manually, what do you see on the preview page, is the meta not updated?

Copied PHP changes from Footnotes: use core’s meta revisioning if available gutenberg#52988 into src/wp-includes/blocks/footnotes.php.

I assume you did an grunt build after this or are running from src?

I will plan to double check functionality manually, then commit this before beta so it can get wider testing. Once it is in core, we can make sure all the GB tests pass.

I tried working on adding similar tests in core directly, but was frustrated by core's use of Puppeteer.

@Mamaduka commented on PR #4859:


15 months ago
#175

I'm still having a problem successfully running the can be previewed when published e2e test locally. However, everything is working correctly when testing manually, so it's probably an issue with my local setup.

Sorry for the false alarm, @adamsilverstein!

I will plan to double check functionality manually, then commit this before beta so it can get wider testing. Once it is in core, we can make sure all the GB tests pass.

Sounds good to me ✅

I tried working on adding similar tests in core directly, but was frustrated by core's use of Puppeteer.

We need to switch core to use Playwright as well; then we can just copy-paste tests as needed.

@adamsilverstein commented on PR #4859:


15 months ago
#176

I'm still having a problem successfully running the can be previewed when published e2e test locally. However, everything is working correctly when testing manually, so it's probably an issue with my local setup.


Excellent, good to hear! Thanks for the manual testing.

I tried working on adding similar tests in core directly, but was frustrated by core's use of Puppeteer.

We need to switch core to use Playwright as well; then we can just copy-paste tests as needed.

That would be fantastic, especially for cases like this where a core change needs to sync with a Gutenberg change!

#177 @adamsilverstein
15 months ago

For testers: footnotes-from-gb.diff are the changes upstream from GB - apply these to test the latest core patch (they aren't included in the core patch because this file is imported directly from the Gutenberg project so it can't be modified here).

#178 @adamsilverstein
15 months ago

I am prepping this for commit, pending any final reviews on the PR so we can land this before Beta 1.

This change will require a detailed dev note (I can start on) to explain how meta revisions work, how to opt in and also the change to where post revision handling is hooked.

In addition, this PR for Gutenberg is required to adjust the behavior there to use the core support instead of the current hooks approach.

@Mamaduka commented on PR #4859:


15 months ago
#179

@adamsilverstein, are you planning to commit this before beta 1 in this release cycle?

@adamsilverstein commented on PR #4859:


15 months ago
#180

This needs better error handling. There are places where functions can return WP_error that are not handled.

Good points and thanks for the review! will address before commit.

@adamsilverstein commented on PR #4859:


15 months ago
#181

@adamsilverstein, are you planning to commit this before beta 1 in this release cycle?

Yes

#182 @adamsilverstein
15 months ago

Thanks for the additional reviews @spacedmonkey and @hellofromTonya - I'm working to address all the points; since everything is minor I'm still planning to commit this before the beta release tomorrow.

#183 @adamsilverstein
15 months ago

I have addressed all of the feedback and added some additional unit tests.

@spacedmonkey commented on PR #4859:


15 months ago
#184

Pinging @swissspidy as this change may effect the web stories plugin that has a custom revision / autosave endpoint.

@spacedmonkey commented on PR #4859:


15 months ago
#185

Pinging @swissspidy as this change may effect the web stories plugin that has a custom revision / autosave endpoint.

@spacedmonkey commented on PR #4859:


15 months ago
#186

Do we need to update wp_create_initial_post_meta?

@adamsilverstein commented on PR #4859:


15 months ago
#187

Do we need to update wp_create_initial_post_meta?

Not sure, I'm going to get this committed then we can revisit during beta.

#188 follow-up: @adamsilverstein
15 months ago

20564.29.diff contains all the latest improvements from the PS. Once tests pass in CI I'll commit this (passing locally).

#189 in reply to: ↑ 188 @sc0ttkclark
15 months ago

Replying to adamsilverstein:

20564.29.diff contains all the latest improvements from the PS. Once tests pass in CI I'll commit this (passing locally).

That diff looks great to me and covers the needs of my projects for integration.

Thanks so much for getting us here! Very exciting to see this hit core finally!

#190 @sc0ttkclark
15 months ago

That line tweak in 20564.30.diff looks good too.

Last edited 15 months ago by sc0ttkclark (previous) (diff)

#191 @adamsilverstein
15 months ago

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

In 56714:

Revisions: framework for storing post meta revisions.

Enable the storing of post meta in revisions including autosaves and previews:

Add a new argument revisions_enabled to the register_meta function which enables storing meta in revisions.

Add a new wp_post_revision_meta_keys filter which developers can use to control which meta is revisioned - it passes an array of the meta keys with revisions enabled as well as the post type.

Meta keys with revisions enabled are also stored for autosaves, and are restored when a revision or autosave is restored. In addition, meta values are now stored with the autosave revision used for previews. Changes to meta can now be previewed correctly without overwriting the published meta (see #20299) or passing data as a query variable, as the editor currently does to preview changes to the featured image.

Changes to meta with revisions enabled are considered when determining if a new revision should be created. A new revision is created if the meta value has changed since the last revision.

Revisions are now saved on the wp_after_insert_post hook instead of post_updated. The wp_after_insert_post action is fired after post meta has been saved by the REST API which enables attaching meta to the revision. To ensure backwards compatibility with existing action uses, wp_save_post_revision_on_insert function exits early if plugins have removed the previous do_action( 'post_updated', 'wp_save_post_revision' ) call.

Props: alexkingorg, johnbillion, markjaquith, WraithKenny, kovshenin, azaozz, tv-productions, p51labs, mattheu, mikeschroder, Mamaduka, ellatrix, timothyblynjacobs, jakemgold, bookwyrm, ryanduff, mintindeed, wonderboymusic, sanchothefat, westonruter, spacedmonkey, hellofromTonya, drewapicture, adamsilverstein, swisspiddy.
Fixes #20564, #20299.

#193 @adamsilverstein
15 months ago

In 56715:

Revisions: avoid double call to wp_restore_post_revision_meta when restoring post.

Remove an extra call to wp_restore_post_revision_meta - the meta restore action is already hooked to wp_restore_post_revision.

Follow up to [56714].

Props: spacedmonkey.
Fixes #20564.

#194 @mukesh27
15 months ago

@adamsilverstein Just to flag. The unit tests test_revisioned_single_post_meta_with_posts_endpoint_page_and_cpt added in [56714] did not perform any assertions.

Here is CI message:

There was 1 risky test:

1) WP_Test_REST_Post_Meta_Fields::test_revisioned_single_post_meta_with_posts_endpoint_page_and_cpt_data_provider
This test did not perform any assertions

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:52
/var/www/vendor/bin/phpunit:118

This ticket was mentioned in PR #5344 on WordPress/wordpress-develop by @adamsilverstein.


14 months ago
#195

Correct an issue where meta values with values like a quote could not be previewed. update_metadata expects data to be slashed.

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

See also: https://github.com/WordPress/gutenberg/pull/52988

#196 @adamsilverstein
14 months ago

@mukesh27 thanks for flagging, I will take a look and get that fixed.

E2E testing in GB on https://github.com/WordPress/gutenberg/pull/52988 also revealed a bug where published post meta with quotes (") cannot be previewed. The reason is the meta values need to be slashed before being inserted. This doesn't affect other code paths because in general we copy the already saved raw meta values over to the revision, so slashing has already been added.

I have a fix for this issue in https://github.com/WordPress/wordpress-develop/pull/5344

To test this bug:

  1. publish a post with a footnote
  2. add a quote to the end of the footnote
  3. preview the post

expected: footnote shows quote
actual: footnote is missing or missing quote

Last edited 14 months ago by adamsilverstein (previous) (diff)

#197 @adamsilverstein
14 months ago

@adamsilverstein Just to flag. The unit tests test_revisioned_single_post_meta_with_posts_endpoint_page_and_cpt added in [56714] did not perform any assertions.

Turns out this was just poor naming, the function in question is a data provider and should have a data_ prefixed name.

I fixed this, and added tests for slashed data in my PR

@mukesh27 commented on PR #5344:


14 months ago
#198

@adamsilverstein, If we remove the wp_slash, the unit tests pass successfully ✅. Do we still need to implement safeguard measures?

@adamsilverstein commented on PR #5344:


14 months ago
#199

@adamsilverstein, If we remove the wp_slash, the unit tests pass successfully ✅. Do we still need to implement safeguard measures?

I added a test that failed before the patch, but removed it to get tests passing in https://github.com/WordPress/wordpress-develop/pull/5344/commits/6927209e425dac834183c5c45c0a62ac174d8906 because it was causing other tests to fail (I suspect because it wasn't cleaning up completely). I'm going to try to reintroduce it in another spot.

@adamsilverstein commented on PR #5344:


14 months ago
#200

@mukeshpanchal27 - I reintroduced the test by putting it in the metaRevisions test file and cleaning it up a bit. This test fails before the PR and essentially reproduces the issue surfaced by the GB E2E test.

#201 @adamsilverstein
14 months ago

20564c.diff includes the changes from the PR: https://github.com/WordPress/wordpress-develop/pull/5344

  • Add slashing to REST API autosaves, fixing an issue with previewing footnotes containing quotes
  • Add a test that validates the slashing and reproduces the json decoding error surfaced by the Gutenberg E2E tests
  • Fix the naming of a test data provider

@adamsilverstein commented on PR #5344:


14 months ago
#202

Trying again in `413ac9f` (#5344); I moved the tests to tests/phpunit/tests/rest-api/rest-autosaves-controller.php and simplified a bit.

#203 @adamsilverstein
14 months ago

In 56745:

Revisions: slash meta values for autosave (preview) revisions.

Correct an issue where meta values containing characters like quote could not be previewed on published posts. The function update_metadata expects data to be slashed.

Also, add a test to confirm that storing JSON data which requires slashing in autosave meta works as expected, and improve naming for a data provider added in [56714].

Follow up to [56714].

Props mukesh27, spacedmonkey.
Fixes #20564.

This ticket was mentioned in PR #5364 on WordPress/wordpress-develop by @adamsilverstein.


14 months ago
#206

Follow up to - follow up https://core.trac.wordpress.org/changeset/56745, this PR now includes the logic for displaying the revisioned meta on the revisions screen.

This core code would replace the filtered in display added by Gutenberg - https://github.com/WordPress/gutenberg/blob/3e260aa41d3c50684efac9728bdbba918658d131/packages/block-library/src/footnotes/index.php#L92-L120. Plugins can still overwrite the default display using the _wp_post_revision_field_{$meta_key} filter.

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

#207 @adamsilverstein
14 months ago

@spacedmonkey or @Mamaduka - I have one more follow up PR @ https://github.com/WordPress/wordpress-develop/pull/5364 - this one adds the display of meta fields to the revisions screen, letting us remove some custom code from the GB plugin.

My thinking on this PR that could use review/feedback:

  • All revisioned meta fields should show on the revisions screen.
  • Use a new filter (wp_revision_ui_fields) instead of the existing _wp_post_revision_fields filter which is meant more for post fields than meta and is called in many places.
  • The new filter allows the field display to be easily unhooked
  • Developers can replace the default display by adding their own callback for the _wp_post_revision_field_{$meta_key} filter.

@adamsilverstein commented on PR #5364:


14 months ago
#208

This looks like a separate enhancement from the original ticket. While I think it's a nice addition to the post-meta revisions, it might be too late to ship in betas.

Yea, I wasn't sure about that since it builds on a new feature we sometimes allow follow up improvements. If not, we can merge in early 6.4. This change is more "compatible" with the existing GB filtering because they overload the same key, there is no harm in running both code paths.

@adamsilverstein commented on PR #5364:


14 months ago
#209

This looks like a separate enhancement from the original ticket. While I think it's a nice addition to the post-meta revisions, it might be too late to ship in betas.

I'll plan to get this in the next release.

@Mamaduka commented on PR #5364:


14 months ago
#210

Thanks, @adamsilverstein!

I don’t want to block the patch. Feel free to check with release leads about this.

#211 @webcommsat
14 months ago

Hello @adamsilverstein is there a link to a dev note draft on this ticket please and is this for the miscellaneous post? The documentation tracking [issue]https://github.com/WordPress/Documentation-Issue-Tracker/issues/1190[] for info. Thanks.

@mikeschroder commented on PR #5364:


14 months ago
#212

Hi @adamsilverstein -- thank you so much!

My apologies for not seeing the ping initially.
I think your plan to land this in 6.5 at this point makes sense, and that it'd be a great addition to the feature.

If you haven't already created a trac ticket for this improvement/enchancement specifically (I didn't find one on a quick search), I think it might make it easier to follow that way.

What do you think?

@adamsilverstein commented on PR #5364:


14 months ago
#213

If you haven't already created a trac ticket for this improvement/enchancement specifically (I didn't find one on a quick search), I think it might make it easier to follow that way.

Yes good idea, I will go ahead and open up a follow up ticket to add this in 6.5

#214 @adamsilverstein
14 months ago

Hello @adamsilverstein is there a link to a dev note draft on this ticket please and is this for the miscellaneous post? The documentation tracking [issue]https://github.com/WordPress/Documentation-Issue-Tracker/issues/1190[] for info. Thanks.

Hi @webcommsat - I will post a draft on the issue you linked later today.

#215 @adamsilverstein
14 months ago

Here is a draft of the dev note: https://docs.google.com/document/d/1ctRkft6qFCHAS3HmJGsBvsWosE7LrYV5mLho0breB28/edit?usp=sharing - Ideally I would like to get some feedback from other contributors before posting to make sure I got it right, made things clear and didn't miss anything.

@adamsilverstein commented on PR #5364:


13 months ago
#216

If you haven't already created a trac ticket for this improvement/enchancement specifically (I didn't find one on a quick search), I think it might make it easier to follow that way.

Yes good idea, I will go ahead and open up a follow up ticket to add this in 6.5

Here is the follow up ticket - https://core.trac.wordpress.org/ticket/59636

@adamsilverstein commented on PR #5364:


10 months ago
#217

@Mamaduka rethinking if we should add fields automatically, see my comment on the trac ticket. let me know what you think.

@Mamaduka commented on PR #5364:


10 months ago
#218

Thank you, @adamsilverstein! Your reasoning makes sense to me.

Note: See TracTickets for help on using tickets.