WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 39 hours ago

#44321 new enhancement

REST API: Expose revision count and last revision ID on Post response

Reported by: danielbachhuber Owned by:
Milestone: 4.9.7 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

A Post in the Gutenberg needs access to:

  • Total number of revisions.
  • Last revision ID.

This data is important for revisions UI in the Post Settings section.

At the moment, the data is exposed in a pretty hacky manner:

"ping_status": "open",
"revisions": {
    "count": 2,
    "last_id": 3
},
"slug": "foo-bar-post",

Is there a better way of exposing this data? I don't have a strong opinion but maybe it'd be better on _links.

See https://github.com/WordPress/gutenberg/pull/3233/files#diff-9dfc2b70ad1c71918441461179054b0bR363

From https://github.com/WordPress/gutenberg/issues/3258

Attachments (2)

44321.1.diff (1.2 KB) - added by kadamwhite 2 weeks ago.
Add a count property to the version-history link specifying the number of available revisions, and a _link using the "predecessor" relation to specify the most recent revision.
44321.2.diff (3.3 KB) - added by danielbachhuber 39 hours ago.

Download all attachments as: .zip

Change History (11)

#1 @kadamwhite
2 weeks ago

Spoke to @rmccue about this a bit and here's my take: First, I would have expected that revision count would be handled similarly to comment count, as it's a similar type of question. Now, we don't seem to _expose_ comment count at all, sooo... maybe we should address that here, too :)

I agree that this feels like a task for _links. Looking at the web linking RFC, it's spec-compliant to add new key/value pairs to a link object:

Target attributes are a set of key/value pairs that describe the link or its target; for example, a media type hint. This specification does not attempt to coordinate their names or use, but does provide common target attributes for use in the Link HTTP header.

This means that count: ## would be appropriate for addition to both the version-history and replies properties.

For last_id, we could also jam that into the _link for the version history relation; OR we could specify it as a new link. I'm slightly biased towards the latter but the former is certainly lighter weight.

To validate an assumption: I haven't checked this, but I'm guessing the last revision ID refers to the parent revision of the published post, NOT a revision that is equal to the published post?

@kadamwhite
2 weeks ago

Add a count property to the version-history link specifying the number of available revisions, and a _link using the "predecessor" relation to specify the most recent revision.

#2 @kadamwhite
2 weeks ago

  • Keywords has-patch added; needs-patch removed

44321.1.diff executes the plan described above, by adding a count to the version-history link and adding a new link using the "predecessor" relation.

#3 @danielbachhuber
2 weeks ago

To validate an assumption: I haven't checked this, but I'm guessing the last revision ID refers to the parent revision of the published post, NOT a revision that is equal to the published post?

Yes. What you've implemented with the patch is the same as what's already in Gutenberg.

44321.1.diff looks good to me. My only concern is that wp_get_post_revisions() might be too heavy for our needs, but I'm not sure whether we have a reasonable alternative (aside from a direct SQL query).

#4 @kadamwhite
2 weeks ago

Agreed, but I'm not sure how best to practically validate that question about the weight of wp_get_post_revisions. I'd like to avoid a custom SQL query if possible, feels like overkill!

Tagging @adamsilverstein and @karmatosed as the listed Revisions component maintainers, any idea if there's a lighter-weight way to get this data?

#5 @birgire
2 weeks ago

One possible improvement that comes to mind is:

$revisions = wp_get_post_revisions( $post->ID, array( 
	'fields' => 'ids',
) );

instead of:

$revisions = wp_get_post_revisions( $post->ID );

since the patch in 44321.1.diff only needs the count and the ID.

Here's the result of few test runs:

100 revisions:
--------------
time: 0.011561
#qs: 2

time: 0.011199
#qs: 2

time: 0.011316
#qs: 2

200 revisions:
--------------
time: 0.020648
#qs: 2

time: 0.021641
#qs: 2

time: 0.021116
#qs: 2

100 revisions (fields=ids):
--------------
time: 0.001606
#qs: 1

time: 0.001734
#qs: 1

time: 0.001875
#qs: 1

200 revisions (fields=ids):
--------------
time: 0.002118
#qs: 1

time: 0.002110
#qs: 1

time: 0.002875
#qs: 1

where we test it with variations of:

global $wpdb;

$post_id           = self::factory()->post->create();
$revisions_count   = 100;
		
foreach( range( 1, $revisions_count ) as $rng ) {
	wp_update_post( array(
		'post_content' => $rng,
		'ID'           => $post_id,
	) );
}
		
$wpdb->timer_start();
$query_count = $wpdb->num_queries;

$revisions = wp_get_post_revisions( $post_id, array( 
	'fields'         => 'ids',
) );

printf( 'time: %f' . PHP_EOL, $wpdb->timer_stop() );
printf( '#qs: %d' . PHP_EOL, $wpdb->num_queries - $query_count );

So from this quick testing, for 100-200 revisions, it seems to reduce the number of queries from 2 to 1 and runs order of magnitude times faster.

One could also try with some extra posts in the database, e.g.:

self::factory()->post->create_many( 1000 );

I also skimmed through wp_count_posts(), but it didn't seem usable here. Just imagine if WP_Query would support a count attribute :-)

Last edited 2 weeks ago by birgire (previous) (diff)

#6 @TimothyBlynJacobs
9 days ago

I believe this should be predecessor-version not predecessor, https://tools.ietf.org/html/rfc5829#section-3.5

#7 @danielbachhuber
9 days ago

+1 to using 'fields' => 'ids' for more performance.

We should also only expose this data when context=edit (which is a deficiency in the current Gutenberg implementation).

#8 @danielbachhuber
40 hours ago

We should also only expose this data when context=edit (which is a deficiency in the current Gutenberg implementation).

Ah, this isn't possible because $request isn't passed through to prepare_links().

I suppose it's a non-issue to include the revision id in public data. In theory, this could be deduced by iterating ids on the revisions endpoint and looking at 401 vs 404 response headers. However, the revision count isn't public data. I don't see any issues with disclosing it, but something worth noting at the least.

#9 @danielbachhuber
39 hours ago

  • Keywords has-unit-tests added; dev-feedback needs-unit-tests removed
  • Milestone changed from 4.9.8 to 4.9.7

In 44321.2.diff:

  • Uses 'fields' => 'ids' when fetching revisions to improve performance.
  • Includes id attribute in predecessor to avoid needing to parse a URL.
  • Adds unit tests.
Note: See TracTickets for help on using tickets.