_wp_post_revision_fields filter is not very useful

[mdawaffe]

The _wp_post_revision_fields filter allows plugins to modify which post fields get revisioned (the default is title, content, excerpt).

It's pretty useless, though, since it's only run once and can't be changed by post_type. A custom post type, for example, might only want to enable revisions on the content.

The _wp_post_revision_fields() is also a variation on the getter/setter pattern (static variable) we use in a few places to avoid globals. It makes the function kind of crazy.

Proposal: Split into two functions

@var int|object|array $post post_id or post object or post array
@return array Fields to revision: array( field => i18n'd label )
function wp_get_post_revision_fields( $post ) {
    // filter with _wp_post_revision_fields: pass post_id, post_type, ...?
    // don't cache results
}

@internal
@var array $past post array
@return array Filled $post array with revision info
function _wp_post_revision_fields( $post ) {
    $revisionable = wp_get_post_revision_fields( $post )
    // ...
}

I'd like to see this in 3.0 if we can, but marking as 3.1 for now.

Adding to the candidate list for review in the 3.6 Revisions Feature work

After chatting as a group we feel this is best addressed at the same time as revisioning postmeta etc.

Related #20564

Replying to mdawaffe:

The _wp_post_revision_fields filter allows plugins to modify which post fields get revisioned (the default is title, content, excerpt).

It's pretty useless, though, since it's only run once and can't be changed by post_type. A custom post type, for example, might only want to enable revisions on the content.

would passing the post object or id to the filter be enough to resolve this? i'm unclear on why 'running only once' is an issue?

The _wp_post_revision_fields() is also a variation on the getter/setter pattern (static variable) we use in a few places to avoid globals. It makes the function kind of crazy.

Not sure what this part means or what you are proposing changes here, please elaborate if possible.

Thanks!

In 13382.diff​:

• Adjust _wp_post_revision_fields() function to allow passing WP_Post object
• Add $post/$revision context to all internal uses of _wp_post_revision_fields()
• Add the $post (array) as an additional parameter to the _wp_post_revision_fields filter, giving you the context you need to determine post_type, or any other post meta information you might need.
• Adjust the doc blocks to reflect the new function and filter parameters.

Appreciate any feedback, I think adding the $post context makes the _wp_post_revision_fields filter more useful.

Looks good to me. Passing the post object provides the context needed so different fields can be returned based on the post.

In 34917:

Pass the $post parameter to the _wp_post_revision_fields filter. This provides more context to the filter, which allows for different fields to be displayed on the revisions screen depending on the post.

The _wp_post_revision_fields() function now also accepts a WP_Post object (in addition to an array of post fields) to facilitate this change.

Fixes #13382
Props adamsilverstein

[34917] causes a unit tests failure:

1) Tests_Post_Revisions::test_revision_dont_save_revision_if_unchanged
Failed asserting that actual size 4 matches expected size 3.

/srv/www/wp-develop/svn/tests/phpunit/tests/post/revisions.php:83

Replying to ocean90:

[34917] causes a unit tests failure:

Good catch. I can dig into this.

In 34925:

Revert [34917] until the broken test is fixed.

See #13382

Before [34917], we only passed the post data to _wp_post_revision_fields() in two places:

• wp_create_post_autosave()​
• _wp_put_post_revision()​

As far as I can see, not passing the post in all other instances was intentional, because the condition in line 64​ is never satisfied now.

13382.2.diff​ just passes $post to the filter as is. The filter would still only run once though, as per the ticket description.

Perhaps a new filter for the return value of the function should be introduced?

13382.2.diff​ looks good to me, I tested and verified the unit tests pass. This is a smaller change and still provides the $post context requested in the ticket.

In 35352:

Docs: Correct description for _wp_post_revision_fields() arguments.

See #13382.

13382.2.diff​ doesn't seem like it would actually be useful, since the post data is only passed to the filter when preparing the revision for saving, but not when retrieving the fields.

From _wp_post_revision_fields() description​:

Does two things. If passed a post *array*, it will return a post array ready to be inserted into the posts table as a post revision. Otherwise, returns an array whose keys are the post fields to be saved for post revisions.

This is extremely confusing. The former should be a separate function.

In 13382.3.diff​:

• Move the array processing to a new function, _wp_post_revision_data().
• Make both functions accept a post array or a WP_Post object.
• Always apply the '_wp_post_revision_fields' filter.
• Always pass the post data to the '_wp_post_revision_fields' filter.
• All unit tests pass.

With a note about deprecating the $autosave parameter

This looks great, thank you @SergeyBiryukov!

I agree, it makes more sense to split up the functions and passing the post in all cases makes the filter actually useful. I verified unit tests pass and revisions and autosaves work as expected.

Question: do we want to call _deprecated_argument for $deprecated, or no because its an internal function?

13382.4.diff​ still applies.

@adamsilverstein What's left here?

Replying to DrewAPicture:

@adamsilverstein What's left here?

@DrewAPicture I think this is ready to go.

In 36659:

Revisions: Clean up _wp_post_revision_fields():

• Move the array processing to a new function, _wp_post_revision_data().
• Make both functions accept a post array or a WP_Post object.
• Always apply the _wp_post_revision_fields filter and pass the post data to it.

Fixes #13382.