WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 days ago

#20564 reopened enhancement

Framework for storing revisions of Post Meta

Reported by: alexkingorg Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4
Component: Revisions Keywords: needs-testing needs-codex dev-feedback
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 (29)

20564.diff (5.8 KB) - added by adamsilverstein 16 months ago.
20564+16215.diff (24.3 KB) - added by adamsilverstein 16 months ago.
for testing
20564.2.diff (5.5 KB) - added by adamsilverstein 16 months ago.
updated against current trunk
20564.3.diff (5.5 KB) - added by adamsilverstein 16 months ago.
update against current
20564.4.patch (3.5 KB) - added by WraithKenny 16 months ago.
20564.5.patch (4.5 KB) - added by WraithKenny 16 months ago.
20564.4.diff (4.2 KB) - added by kovshenin 16 months ago.
20564.5.diff (2.5 KB) - added by kovshenin 16 months ago.
20564-6.patch (8.3 KB) - added by azaozz 16 months ago.
20564.6.diff (3.8 KB) - added by kovshenin 16 months ago.
20564.7.diff (4.0 KB) - added by kovshenin 15 months ago.
20564-8.patch (6.3 KB) - added by azaozz 15 months ago.
20564-9.patch (7.0 KB) - added by adamsilverstein 11 months ago.
reintroduce changes removed in 24397
20564-10.diff (11.7 KB) - added by adamsilverstein 8 months ago.
includes unit test
20564-11.diff (11.0 KB) - added by adamsilverstein 7 months ago.
remove unused post format meta values from default array and tests
20564-12.diff (11.1 KB) - added by adamsilverstein 7 months ago.
slight test cleanup
20564-13.diff (11.3 KB) - added by adamsilverstein 6 months ago.
20564-14.diff (9.1 KB) - added by adamsilverstein 6 months ago.
remove post format revisioning
20564.8.diff (9.9 KB) - added by DrewAPicture 6 months ago.
+ docs fixes
20564.9.diff (10.7 KB) - added by adamsilverstein 5 months ago.
Refresh against trunk. remove redundant restore block; update unit tests
20564.10.diff (11.2 KB) - added by adamsilverstein 5 months ago.
add wp_unslash to $_POST comparison
20564-autosave-data.png (148.4 KB) - added by TV productions 4 months ago.
The data that an autosave sent in wp 3.9-alpha-27445-src
20564.11.diff (11.2 KB) - added by adamsilverstein 4 months ago.
Refresh against trunk
20564.12.diff (9.9 KB) - added by adamsilverstein 2 months ago.
refresh against trunk
20564.13.diff (10.3 KB) - added by adamsilverstein 2 weeks ago.
refresh against trunk
20564.14.diff (12.9 KB) - added by adamsilverstein 12 days ago.
only save changed revisions
20564.15.diff (11.3 KB) - added by adamsilverstein 12 days ago.
don't save blank meta, track revisioned keys
20564.16.diff (12.0 KB) - added by adamsilverstein 12 days ago.
update unit test, restore revision with untracked key
20564.17.diff (12.9 KB) - added by adamsilverstein 9 days ago.
account for the storing of multiple values per key

Download all attachments as: .zip

Change History (138)

comment:1 scribu2 years ago

  • Cc scribu added

comment:2 toscho2 years ago

  • Cc info@… added

comment:3 rspindel2 years ago

  • Cc rspindel added

comment:4 jkudish2 years ago

  • Cc joachim.kudish@… added

comment:5 Ipstenu2 years ago

  • Cc ipstenu@… added

comment:6 kovshenin2 years ago

  • Cc kovshenin@… added

comment:7 hameedullah2 years ago

  • Cc h@… added

comment:8 ryanduff2 years ago

  • Cc ryan@… added

comment:9 maor2 years ago

  • Cc maorhaz@… added

comment:10 travisnorthcutt2 years ago

  • Cc travis@… added

comment:11 sc0ttkclark2 years ago

  • Cc lol@… added

comment:12 xyzzy2 years ago

  • Cc dennen@… added

comment:13 lightningspirit2 years ago

  • Cc lightningspirit@… added

+1

comment:14 swissspidy18 months ago

  • Cc hello@… added

comment:15 sennza18 months ago

  • Cc bronson@… added

comment:16 jaredatch18 months ago

  • Cc jared@… added

comment:17 bainternet18 months ago

  • Cc admin@… added

comment:18 jb51018 months ago

  • Cc jbrown510@… added

comment:19 follow-up: nacin18 months ago

  • Milestone changed from Awaiting Review to 3.6

This and #20299 would block #23314.

comment:20 johnbillion18 months 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 18 months ago by johnbillion (previous) (diff)

comment:21 in reply to: ↑ 19 ; follow-up: adamsilverstein18 months 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.

comment:22 adamsilverstein18 months ago

  • Cc adamsilverstein@… added

comment:23 goto1017 months ago

  • Cc dromsey@… added

comment:24 in reply to: ↑ 21 westi17 months 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.

comment:25 kwight17 months ago

  • Cc kwight@… added

comment:26 follow-up: markjaquith16 months ago

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

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

adamsilverstein16 months ago

adamsilverstein16 months ago

for testing

comment:28 adamsilverstein16 months 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

adamsilverstein16 months ago

updated against current trunk

adamsilverstein16 months ago

update against current

comment:29 follow-up: markjaquith16 months 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.

comment:30 in reply to: ↑ 29 adamsilverstein16 months 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?

comment:31 WraithKenny16 months ago

  • Cc Ken@… added

comment:32 WraithKenny16 months 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;
}

WraithKenny16 months ago

comment:33 WraithKenny16 months 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.

comment:34 WraithKenny16 months ago

  • Keywords has-patch needs-testing added

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

WraithKenny16 months ago

comment:36 WraithKenny16 months ago

new patch adds get_versioned_meta_keys

comment:37 in reply to: ↑ 35 WraithKenny16 months 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.

kovshenin16 months ago

comment:38 kovshenin16 months ago

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

kovshenin16 months ago

comment:39 kovshenin16 months ago

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

comment:40 markjaquith16 months ago

In 23859:

Revision our post format postmeta.

props kovshenin, WraithKenny. see #20564.

comment:41 DrewAPicture16 months ago

  • Keywords needs-codex added

See [23859]

Last edited 16 months ago by DrewAPicture (previous) (diff)

comment:42 follow-up: azaozz16 months 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.

comment:43 WraithKenny16 months ago

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

azaozz16 months ago

comment:44 azaozz16 months 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 16 months ago by azaozz (previous) (diff)

comment:45 in reply to: ↑ 42 kovshenin16 months 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?

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

comment:47 kovshenin16 months 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 :)

comment:48 follow-up: azaozz16 months 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?

kovshenin16 months ago

comment:49 in reply to: ↑ 48 ; follow-up: kovshenin16 months 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 :)

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

comment:51 follow-up: WraithKenny16 months 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?

comment:52 travisnorthcutt16 months ago

  • Cc travis@… removed

comment:53 in reply to: ↑ 49 ; follow-up: azaozz15 months 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 15 months ago by azaozz (previous) (diff)

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

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

kovshenin15 months ago

comment:56 kovshenin15 months 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

comment:57 azaozz15 months 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 15 months ago by azaozz (previous) (diff)

azaozz15 months ago

comment:58 azaozz15 months ago

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

comment:59 kovshenin15 months ago

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

comment:60 azaozz15 months 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.

comment:61 kovshenin15 months 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!

comment:62 ryan15 months ago

[23936] for @since update.

comment:63 adamsilverstein14 months ago

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

comment:64 helen11 months ago

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

comment:65 follow-up: alexkingorg11 months 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?

comment:66 adamsilverstein11 months 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.

Last edited 11 months ago by SergeyBiryukov (previous) (diff)

comment:67 SergeyBiryukov11 months ago

  • Milestone changed from 3.6 to Future Release

adamsilverstein11 months ago

reintroduce changes removed in 24397

comment:68 adamsilverstein11 months 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 months ago by SergeyBiryukov (previous) (diff)

comment:69 jgwspud11 months ago

  • Cc jgwspud added

comment:70 c3mdigital11 months ago

#23973 was marked as a duplicate.

comment:71 doughamlin11 months ago

  • Cc doughamlin@… added

comment:72 doughamlin11 months ago

  • Cc doughamlin@… removed

comment:73 doughamlin11 months ago

  • Cc doughamlin@… added

comment:74 in reply to: ↑ 65 WraithKenny11 months 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

comment:75 sillybean10 months ago

  • Cc steph@… added

comment:76 donutz10 months ago

  • Cc peterherrel@… added

comment:77 jgwspud10 months ago

  • Cc jgwspud removed

adamsilverstein8 months ago

includes unit test

comment:78 adamsilverstein8 months 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.

adamsilverstein7 months ago

remove unused post format meta values from default array and tests

comment:79 adamsilverstein7 months 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

adamsilverstein7 months ago

slight test cleanup

adamsilverstein6 months ago

comment:80 adamsilverstein6 months 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

adamsilverstein6 months ago

remove post format revisioning

comment:81 adamsilverstein6 months ago

20564-14.diff​:

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

DrewAPicture6 months ago

+ docs fixes

comment:82 DrewAPicture6 months ago

  • Keywords needs-refresh removed
  • Version set to 3.4

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

comment:83 ircbot6 months ago

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

comment:85 ircbot6 months ago

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

comment:86 ircbot5 months ago

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

adamsilverstein5 months ago

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

comment:87 adamsilverstein5 months ago

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

adamsilverstein5 months ago

add wp_unslash to $_POST comparison

comment:88 adamsilverstein5 months 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.

comment:89 ircbot4 months ago

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

comment:90 ircbot4 months ago

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

comment:91 follow-up: TV productions4 months 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).

comment:92 in reply to: ↑ 91 ; follow-up: adamsilverstein4 months 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?

comment:93 ircbot4 months ago

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

TV productions4 months ago

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

comment:94 in reply to: ↑ 92 ; follow-up: TV productions4 months 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?

comment:95 in reply to: ↑ 94 ; follow-up: adamsilverstein4 months 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)

adamsilverstein4 months ago

Refresh against trunk

comment:96 in reply to: ↑ 95 TV productions4 months 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 4 months ago by TV productions (previous) (diff)

adamsilverstein2 months ago

refresh against trunk

comment:97 adamsilverstein2 months ago

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

comment:98 ircbot2 months ago

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

comment:99 helen4 weeks ago

#28553 was marked as a duplicate.

adamsilverstein2 weeks ago

refresh against trunk

comment:100 adamsilverstein2 weeks ago

20564.13.diff - Refresh against trunk, slightly updated

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

adamsilverstein12 days ago

only save changed revisions

comment:101 adamsilverstein12 days 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.

adamsilverstein12 days ago

don't save blank meta, track revisioned keys

comment:102 follow-up: adamsilverstein12 days 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

adamsilverstein12 days ago

update unit test, restore revision with untracked key

adamsilverstein9 days ago

account for the storing of multiple values per key

comment:103 adamsilverstein9 days ago

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

comment:104 ircbot9 days ago

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

comment:105 in reply to: ↑ 102 ; follow-up: p51labs4 days 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 4 days ago by p51labs (previous) (diff)

comment:106 in reply to: ↑ 105 adamsilverstein4 days 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 4 days ago by adamsilverstein (previous) (diff)

comment:107 follow-up: p51labs4 days 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.

comment:108 in reply to: ↑ 107 adamsilverstein3 days 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.

comment:109 ircbot3 days ago

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

Note: See TracTickets for help on using tickets.