Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#44321 closed enhancement (fixed)

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

Reported by: danielbachhuber's profile danielbachhuber Owned by: pento's profile pento
Milestone: 4.9.8 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests commit fixed-major
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 (3)

44321.1.diff (1.2 KB) - added by kadamwhite 6 years 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 6 years ago.
44321.3.diff (3.4 KB) - added by danielbachhuber 6 years ago.

Download all attachments as: .zip

Change History (23)

#1 @kadamwhite
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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 6 years ago by birgire (previous) (diff)

#6 @TimothyBlynJacobs
6 years ago

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

#7 @danielbachhuber
6 years 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
6 years 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
6 years 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.

#10 @danielbachhuber
6 years ago

In 44321.3.diff:

#11 @ocean90
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

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


6 years ago

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


6 years ago

#14 @pbiron
6 years ago

We discussed this during the 4.9.8 bug scrub today.

Is this ready to land in 4.9.8? Beta is scheduled for July 17 (next Tues). Any chance it will be ready?

#15 follow-up: @danielbachhuber
6 years ago

  • Keywords commit added
  • Owner set to pento
  • Status changed from new to assigned

@pbiron Yes, this is ready to land. It's in Gutenberg as of https://github.com/WordPress/gutenberg/pull/7495

#16 in reply to: ↑ 15 @pbiron
6 years ago

Replying to danielbachhuber:

@pbiron Yes, this is ready to land. It's in Gutenberg as of https://github.com/WordPress/gutenberg/pull/7495

Thanx for the quick response!

#17 @pento
6 years ago

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

In 43439:

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

So that REST API clients can show appropriate UI for a post's revisions, it needs to know how many revisions the post has, and what the latest revision ID is.

Props kadamwhite, danielbachhuber, birgire, TimothyBlynJacobs.
Fixes #44321.

#18 @pento
6 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopened for backport.

#19 @pento
6 years ago

In 43441:

REST API: Update the test fixture generator following [43439]

As [43439] added data that contains object IDs, it can cause wp-api-generated.js to be unnecessarily regenerated.

Regenerating our list of fixtures that need normalising rectifies this.

See #44321.

#20 @pento
6 years ago

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

In 43442:

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

So that REST API clients can show appropriate UI for a post's revisions, it needs to know how many revisions the post has, and what the latest revision ID is.

Merge of [43439] and [43441] to the 4.9 branch.

Props kadamwhite, danielbachhuber, birgire, TimothyBlynJacobs, pento.
Fixes #44321.

Note: See TracTickets for help on using tickets.