Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22289 closed enhancement (fixed)

Filter to override WP_POST_REVISIONS (or define it later)

Reported by: batmoo's profile batmoo Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.4.2
Component: Revisions Keywords: westi-likes has-patch
Focuses: Cc:

Description

We currently have the WP_POST_REVISIONS constant which allows you to disable or set the number of revisions allowed. This is great but doesn't easily allow a per-blog setting on a multisite install.

Two possible options:

  • A new filter to override the value defined in the constant.
  • Define the constant later so that plugins/themes can override it based on the current blog context.

Attachments (5)

22289.patch (6.1 KB) - added by ethitter 11 years ago.
22289.2.patch (5.8 KB) - added by ethitter 11 years ago.
22289.3.patch (5.2 KB) - added by ethitter 11 years ago.
22289.4.patch (5.2 KB) - added by ethitter 11 years ago.
22289.5.patch (6.0 KB) - added by SergeyBiryukov 11 years ago.
Minor phpdoc and formatting fixes

Download all attachments as: .zip

Change History (21)

#1 @westi
11 years ago

  • Keywords needs-patch westi-likes revisions-3.6 added

In general I think it would be good to move away from using a CONSTANT here and instead have two new functions called something like wp_revisions_enabled and wp_number_of_revisions_to_keep.

We can then have a filter in both of these functions and use the CONSTANT if defined or fall back to the default behaviour.

Using a CONSTANT throughout the code for this was a mistake.

Patches entertained with interest.

#2 @westi
11 years ago

  • Keywords revisions-3.6 removed
  • Milestone changed from Awaiting Review to 3.6

#3 @jb510
11 years ago

http://wordpress.org/extend/plugins/revision-control/ works well for this. While it'd be "nice" and "cleaner" to have a filter for this in core it seems low priority with a good working plugin addressing it.

#4 @dd32
11 years ago

While it'd be "nice" and "cleaner" to have a filter for this in core it seems low priority with a good working plugin addressing it.

The above plugin is mine, and a filter is dearly needed for this.. While I managed to get it working reasonably well, it wasn't exactly the nicest thing to do.
Remember that with constants, only one place can define them (a single plugin, or the wp-config file, etc) leading to many compatibility problems.

A number of plugins over time have also been known to define the number of revisions to store which caused havok for some users, bad plugins, which were fixed, but still shows problems exist.

#5 @jb510
11 years ago

Thanks Dion. I agree it shouldn't be a constant won't argue over it's priority any longer.

I'm less worried about a setting per site though and think about it needing to be set per post_type, perhaps in register_post_type and post_type_supports. That would allow later modifcation with add/remove_post_type_support. Is that a reasonable starting point for this, or am I totally off base?

#6 @ethitter
11 years ago

  • Cc erick@… added

#7 follow-up: @nacin
11 years ago

I've thought about this for a long time now. I think this ought to be post type specific. You may care about lots of revisions for a fast-edited post, but you may care less about pages. Though, I'm thinking more about individual post types wanting control over this.

The API will probably look very similar to another thing I'd like to see — post type specific trash durations.

@ethitter
11 years ago

#8 in reply to: ↑ 7 @ethitter
11 years ago

Replying to nacin:

I've thought about this for a long time now. I think this ought to be post type specific.

I've been working on this for a bit now, and post-type-specific revision values will be possible, but I haven't made any changes that explicitly do so (yet).

There are a few uses of WP_POST_REVISIONS, such as those in wp-includes/post.php, where only a post ID is available. I don't think it's a great idea to move the get_post() calls in those cases, since as is we bail beforehand if someone sets the constant's value to false.

#9 follow-up: @ethitter
11 years ago

  • Keywords has-patch added; needs-patch removed

As 22289.patch demonstrates, there are a variety of places where WP_POST_REVISIONS is used, and the data that is available and useful in each situation varies. As such, I opted to use compact() to ensure that a single filter is available, context about the filter calls is provided, and whatever data is available in each context is passed as well.

From call to call, the wp_post_revisions filter will always provide a context array key in the additional data it passes, and a full post object is generally available. Beyond that, consistency is lost. In XMLRPC calls, it might be useful to know the username or blog ID of the request, whereas uses in revisions.php can call for the post, the current revision, or the two revisions being compared.

For what it's worth, I also prepared a patch that uses a different filter in each of the three files impacted by this patch, but that seemed like more of a pain for users.

#10 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#11 in reply to: ↑ 9 @westi
11 years ago

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

Replying to ethitter:

As 22289.patch demonstrates, there are a variety of places where WP_POST_REVISIONS is used, and the data that is available and useful in each situation varies. As such, I opted to use compact() to ensure that a single filter is available, context about the filter calls is provided, and whatever data is available in each context is passed as well.

From call to call, the wp_post_revisions filter will always provide a context array key in the additional data it passes, and a full post object is generally available. Beyond that, consistency is lost. In XMLRPC calls, it might be useful to know the username or blog ID of the request, whereas uses in revisions.php can call for the post, the current revision, or the two revisions being compared.

For what it's worth, I also prepared a patch that uses a different filter in each of the three files impacted by this patch, but that seemed like more of a pain for users.

I'm not a big fan of the current patch it feels a bit too "clever" and also a bit too "complex" I think abstracting the current code into two functions as I suggested in #comment:1 might make the changes simpler and more understandable.

I think it is worth passing the post type into the filter in all cases and re-working the code to make the post_type available in the case where it currently isn't.

#12 @westi
11 years ago

Also, noting here that we should consider allowing for _wp_post_revision_fields to specialise based on post_type too.

@ethitter
11 years ago

#13 @ethitter
11 years ago

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

22289.2.patch simplifies the approach for this ticket, following @westi's advice. It introduces the wp_revisions_to_keep filter, which lets users specify a number of revisions to store based on post type.

Backwards compatibility is achieved by first checking the value of the WP_POST_REVISIONS constant. If true, PHP's INF constant is used to indicate that all revisions should be stored. If WP_POST_REVISIONS is false or a positive integer, that value's numeric representation is used. If, for whatever reason, either WP_POST_REVISIONS or the value returned via wp_revisions_to_keep is negative, _doing_it_wrong is invoked and the number of revisions to be stored is set to zero.

With the patch applied, all unit tests present as of 1219/tests passed, save one pertaining to media that failed against a clean checkout of trunk as of r23460.

Last edited 11 years ago by ethitter (previous) (diff)

@ethitter
11 years ago

#14 @ethitter
11 years ago

Freshed the patch per @nacin's suggestions in IRC.

Now, the entire post object is available to the filter, and -1 is used to indicate an unlimited number of revisions should be kept.

@ethitter
11 years ago

#15 @ethitter
11 years ago

Refreshed the patch to account for code reorganization in r23466.

@SergeyBiryukov
11 years ago

Minor phpdoc and formatting fixes

#16 @markjaquith
11 years ago

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

In 23818:

Take revision control out of the realm of a pure constant. Make it filterable.

  • New filter: wp_revisions_to_keep

props ethitter, SergeyBiryukov. fixes #22289.

Note: See TracTickets for help on using tickets.