WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 14 months ago

#40510 closed enhancement (fixed)

REST API: Post Revisions: Adding support for pagination

Reported by: benoitperson Owned by: flixos90
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:
PR Number:

Description (last modified by flixos90)

The post revisions REST endpoint currently has no support for pagination. While it works well for most usages with fairly low counts of revisions (WordPress.com limits the number at 25 for instance), on posts with a revision count in the hundreds (or even thousands), this can quickly yield unreasonably large (Mb+ of pure text) and long (several seconds on my "test" instances) responses.

The API already supports pagination for a vast majority of ressources (posts, pages, categories, tags, users) and is normalised around using 2 integer parameters: page (default: 1)and per_page (default: 10).

I wonder if there is any reason why pagination was never implemented on that endpoint? Is it something potentially worth working on? Considering some of the past issues around high memory usage for the revisions page of wp-admin [0][1], it seems like getting a way to only load (for instance) the most recent revisions would be an interesting win to provide a smoother experience with revisions.

[0] https://core.trac.wordpress.org/ticket/34560
[1] https://core.trac.wordpress.org/ticket/24958

PS: To provide some (more) context, there is an ongoing effort to support revisions in Calypso: https://github.com/Automattic/wp-calypso/pull/12733 from which this enhancement request emerged.

Attachments (6)

40510.diff (6.9 KB) - added by flixos90 21 months ago.
40510.2.diff (13.8 KB) - added by adamsilverstein 21 months ago.
40510.3.diff (26.8 KB) - added by birgire 21 months ago.
40510.4.diff (28.3 KB) - added by flixos90 16 months ago.
40510.5.diff (29.4 KB) - added by flixos90 16 months ago.
40510.6.diff (29.6 KB) - added by flixos90 16 months ago.

Download all attachments as: .zip

Change History (28)

#1 @danielbachhuber
3 years ago

I wonder if there is any reason why pagination was never implemented on that endpoint?

Probably an oversight, to be honest.

Is it something potentially worth working on?

Go for it!

#2 @rmccue
3 years ago

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

Seems to just be something that was missed. Let's add it. :)

@flixos90
21 months ago

#3 @flixos90
21 months ago

  • Description modified (diff)
  • Keywords has-patch needs-unit-tests added; needs-patch removed
  • Milestone changed from Future Release to 5.0
  • Owner set to flixos90
  • Status changed from new to assigned

Per today's REST API chat, supporting pagination and generally allowing a bit more flexibility with the revisions controller would also come in handy for Gutenberg.

40510.diff adds support for the following query parameters for revisions:

  • exclude
  • include
  • offset
  • order (default 'desc')
  • orderby (default 'date')
  • page (default 1)
  • per_page (default not provided, falling back to using -1 with WP_Query, meaning no limit, for BC with current behavior)
  • search

The implementation aligns closer with how the posts controller works. WP_Query is directly used instead of calling wp_get_post_revisions(), to be able to run a SELECT FOUND_ROWS() query and get the result. Since the function is only a simple wrapper for a query object anyway which sets default arguments, this can easily be handled in the controller itself.

The revisions controller will now return X-WP-Total and X-WP-TotalPages headers and pagination link headers (if applicable) for collection requests, just how the posts controller does.

This ticket was mentioned in Slack in #core-restapi by flixos90. View the logs.


21 months ago

#5 @adamsilverstein
21 months ago

This looks good @flixos90! Tested with the JS client and I see all of the expected headers and pagination functionality.

In 40510.2.diff I added a cast to (int) for $request['page'];use and update the fixtures. (phpunit --group=restapi-jsclient or grunt precommit generates this file). Some tests to validate all of the new features would be nice.

Last edited 21 months ago by adamsilverstein (previous) (diff)

#6 @birgire
21 months ago

This seems to be in a good progress.

The posts rest controller handles out-of-bounds cases for the page parameter. I think it's originally from here:
https://github.com/WP-API/WP-API/commit/789de83a3c5b4f8d6c6cf5fd63408702833c0cb5

The comments and users rest controllers handles the number and offset in a similar way.

I played around with the offset and page parameters in 40510.2.diff and noticed that for out-of-bound values, we loose the information for X-WP-Total and X-WP-TotalPages as they only show 0.

Here's an example for a post with two revisions and per_page as 1:

page=1:
    X-WP-Total 2
    X-WP-TotalPages 2
    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=2>; rel="next"

page=2:
    X-WP-Total 2
    X-WP-TotalPages 2
    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=1>; rel="prev"

page=3:
    X-WP-Total 0
    X-WP-TotalPages 0
    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=0>; rel="prev"

Here we see what happens with the out-of-bound value 3 for page.

The reason is that the found_posts and max_num_pages properties of WP_Query are not calculated for empty results set. I'm not sure why that has to happen, because it's useful information.

We can see that here:

https://core.trac.wordpress.org/browser/tags/4.9.4/src/wp-includes/class-wp-query.php#L2828

as there is no $this->set_found_posts( $q, $limits ); call, but even if it were, there's an empty posts check within that method too.

Then I started to look at the corresponding tests and 40510.3.diff is a suggestion that:

  • Handles out-of-bound values for offset and page in the same way as the posts, comments and users rest controllers do.
  • Adds the rest_revision_invalid_offset_number error, similar to the existing rest_revision_invalid_page_number error.
  • Introduces tests for the revision query parameters:
    • offset
    • page
    • per_page
    • search

The tests for the posts rest controller where really helpful, but I tried to break them up into single test methods.

The patch also adds another revision and the total revision count to the current fixtures, to make it more usable by the newly added tests. I was hesitated to do this, but I ended up doing it to reduce the new code lines ;-) Using data providers might also be considered.

Last edited 21 months ago by birgire (previous) (diff)

@birgire
21 months ago

@flixos90
16 months ago

#7 @flixos90
16 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Thanks for the unit tests and improvements on this @birgire!

40510.4.diff is a minor update. The offset parameter should also trigger an error if it is equal to the number of available elements because this already means it is out of bound (contrary to page which starts counting at 1, offset starts at 0), so I changed the > to >=. I also switched around the order of the error checks for offset and page and only execute the latter if offset is not given. Offset takes precedence over page: If offset is present, page doesn't do anything at all, so there's no point in triggering an error if page is too large in such a case.

#8 @birgire
16 months ago

@flixos90 nice touch adding the phpcs:ignore for the filter name.

I just skimmed quickly through 40510.4.diff and noticed:

  • The patch should not include changes to tests/qunit/fixtures/wp-api-generated.js.
  • There seems to be a debug relic of return true in get_items_permissions_check() in class-wp-rest-revisions-controller.php.

@flixos90
16 months ago

#9 @flixos90
16 months ago

40510.5.diff removes the accidentally contained return true in there. It also sets the orderby to date ID for backward-compatibility with how wp_get_post_revisions() works.

@flixos90
16 months ago

#10 @flixos90
16 months ago

40510.6.diff fixes unit test failures.

Last edited 16 months ago by flixos90 (previous) (diff)

#11 @flixos90
16 months ago

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

In 43584:

REST API: Support pagination, order, search and other common query parameters for revisions.

The original REST API revisions controller relied on wp_get_post_revisions(), getting all revisions of a post without any possibility to restrict the result. This changeset replaces that function call with a proper WP_Query setup, replicating how wp_get_post_revisions() works while offering parameters to alter the default behavior.

Props adamsilverstein, birgire, flixos90.
Fixes #40510.

#12 @flixos90
16 months ago

In 43585:

REST API: Fix failing tests after [43584].

See #40510.

#13 @flixos90
16 months ago

In 43586:

REST API: Fix failing tests after [43584] and [43585].

See #40510.

#14 @flixos90
16 months ago

@pento Regardless of all this mess I made here... I think I recall this being helpful / asked for for Gutenberg. If so, it might make sense to backport to 4.9.9 - correct me if I'm wrong.

#15 @pento
15 months ago

  • Keywords fixed-major commit added
  • Milestone changed from 5.0 to 4.9.9
  • Resolution fixed deleted
  • Status changed from closed to reopened

Yah, let's backport this to 4.9.9.

#16 @SergeyBiryukov
15 months ago

In 43647:

Tests: Restore restapi group on WP_Test_REST_Revisions_Controller.

See #40510.

#17 @SergeyBiryukov
15 months ago

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

In 43648:

REST API: Support pagination, order, search and other common query parameters for revisions.

The original REST API revisions controller relied on wp_get_post_revisions(), getting all revisions of a post without any possibility to restrict the result. This changeset replaces that function call with a proper WP_Query setup, replicating how wp_get_post_revisions() works while offering parameters to alter the default behavior.

Props adamsilverstein, birgire, flixos90.
Merges [43584-43586], [43647] to the 4.9 branch.
Fixes #40510.

#18 @birgire
15 months ago

Thanks for the commit @SergeyBiryukov


Ps: first item of https://core.trac.wordpress.org/ticket/40510#comment:8

This also came up here #39122.

#19 @pento
14 months ago

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

[43648] can be merged to the 5.0 branch, it also will need to be reverted from the 4.9 branch.

#20 @danielbachhuber
14 months ago

  • Milestone changed from 4.9.9 to 5.0

#21 @SergeyBiryukov
14 months ago

In 43715:

REST API: Revert [43648] from the 4.9 branch.

This change is out of the 4.9.x scope, and will be reintroduced in 5.0.x.

See #40510.

#22 @SergeyBiryukov
14 months ago

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

In 43716:

REST API: Support pagination, order, search and other common query parameters for revisions.

The original REST API revisions controller relied on wp_get_post_revisions(), getting all revisions of a post without any possibility to restrict the result. This changeset replaces that function call with a proper WP_Query setup, replicating how wp_get_post_revisions() works while offering parameters to alter the default behavior.

Props adamsilverstein, birgire, flixos90.
Merges [43584-43586], [43647] to the 5.0 branch.
Fixes #40510.

Note: See TracTickets for help on using tickets.