WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 3 years ago

#13382 closed enhancement (fixed)

_wp_post_revision_fields filter is not very useful

Reported by: mdawaffe Owned by: adamsilverstein
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.0
Component: Revisions Keywords: has-patch commit 4.5-early
Focuses: Cc:

Description

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.

Attachments (4)

13382.diff (4.5 KB) - added by adamsilverstein 4 years ago.
13382.2.diff (1.2 KB) - added by SergeyBiryukov 4 years ago.
13382.3.diff (10.3 KB) - added by SergeyBiryukov 4 years ago.
13382.4.diff (10.4 KB) - added by SergeyBiryukov 4 years ago.
With a note about deprecating the $autosave parameter

Download all attachments as: .zip

Change History (29)

#1 @nacin
9 years ago

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

#2 @westi
7 years ago

  • Keywords revisions-3.6 added

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

#3 @westi
7 years ago

  • Keywords revisions-3.6 removed

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

Related #20564

#4 in reply to: ↑ description @adamsilverstein
5 years ago

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!

#5 @adamsilverstein
4 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed
  • Owner set to adamsilverstein
  • Status changed from new to assigned

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.

#6 @johnbillion
4 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.4

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

#7 @wonderboymusic
4 years ago

  • Owner changed from adamsilverstein to johnbillion

#8 @johnbillion
4 years ago

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

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

#9 follow-up: @ocean90
4 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[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

#10 in reply to: ↑ 9 @adamsilverstein
4 years ago

Replying to ocean90:

[34917] causes a unit tests failure:

Good catch. I can dig into this.

#11 @johnbillion
4 years ago

  • Owner changed from johnbillion to adamsilverstein
  • Status changed from reopened to assigned

#12 @johnbillion
4 years ago

In 34925:

Revert [34917] until the broken test is fixed.

See #13382

#13 @SergeyBiryukov
4 years ago

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

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?

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

#14 @wonderboymusic
4 years ago

  • Milestone changed from 4.4 to Future Release

#15 @adamsilverstein
4 years ago

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.

#16 @adamsilverstein
4 years ago

  • Keywords has-patch commit added; needs-patch removed

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


4 years ago

#18 @SergeyBiryukov
4 years ago

In 35352:

Docs: Correct description for _wp_post_revision_fields() arguments.

See #13382.

#19 @SergeyBiryukov
4 years ago

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.

#20 @SergeyBiryukov
4 years ago

  • Keywords 4.5-early added

@SergeyBiryukov
4 years ago

With a note about deprecating the $autosave parameter

#21 @adamsilverstein
4 years ago

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?

#22 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 4.5

#23 follow-up: @DrewAPicture
3 years ago

13382.4.diff still applies.

@adamsilverstein What's left here?

#24 in reply to: ↑ 23 @adamsilverstein
3 years ago

Replying to DrewAPicture:

@adamsilverstein What's left here?

@DrewAPicture I think this is ready to go.

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

#25 @SergeyBiryukov
3 years ago

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

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.

Note: See TracTickets for help on using tickets.