WordPress.org
Get WordPress

Make WordPress Core

Changes between Initial Version and Version 1 of Ticket #40510, comment 6


Ignore:
Timestamp:
04/02/2018 10:05:38 AM (7 years ago)
Author:
birgire
Comment:

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #40510, comment 6

    initial v1  
    11This seems to be in a good progress.
     2
     3The ''posts'' rest controller handles out-of-bounds cases for the {{{page}}} parameter. I think it's originally from here:
     4https://github.com/WP-API/WP-API/commit/789de83a3c5b4f8d6c6cf5fd63408702833c0cb5
     5
     6The ''comments'' and ''users'' rest controllers handles the {{{number}}} and {{{offset}}} in a similar way.
    27
    38I played around with the {{{offset}}} and {{{page}}} parameters in [attachment: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}}}.
    … …  
    712{{{
    813page=1:
    9     [X-WP-Total] => 2
    10     [X-WP-TotalPages] => 2
    11     [Link] => <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=2>; rel="next"
     14    X-WP-Total 2
     15    X-WP-TotalPages 2
     16    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=2>; rel="next"
    1217
    1318page=2:
    14     [X-WP-Total] => 2
    15     [X-WP-TotalPages] => 2
    16     [Link] => <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=1>; rel="prev"
     19    X-WP-Total 2
     20    X-WP-TotalPages 2
     21    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=1>; rel="prev"
    1722
    1823page=3:
    19     [X-WP-Total] => 0
    20     [X-WP-TotalPages] => 0
    21     [Link] => <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=0>; rel="prev"
     24    X-WP-Total 0
     25    X-WP-TotalPages 0
     26    Link <http://wp.localhost/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F3%2Frevisions&per_page=1&page=0>; rel="prev"
    2227
    2328}}}
    2429
    2530Here we see what happens with the out-of-bound value {{{3}}} for {{{page}}}.
    26 
    2731
    2832The 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.
    … …  
    3438as 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.
    3539
    36 
    37 The post rest controller handles this, but only for the {{{page}}} parameter.
    38  
    39 
    4040Then I started to look at the corresponding tests and [attachment:40510.3.diff] is a suggestion that:
    4141
    42 - Handles out-of-bound values for {{{offset}}} and {{{page}}}.
     42- Handles out-of-bound values for {{{offset}}} and {{{page}}} in the same way as the ''posts'', ''comments'' and ''users'' rest controllers do.
     43- Adds the {{{rest_revision_invalid_offset_number}}} error, similar to the existing {{{rest_revision_invalid_page_number}}} error.
    4344- Introduces tests for the revision query parameters:
    44  - offset
    45  - page
    46  - per_page
    47  - search
     45 - {{{offset}}}
     46 - {{{page}}}
     47 - {{{per_page}}}
     48 - {{{search}}}
    4849
    49 The tests for the post rest controller where really helpful, but I tried to break them up into single test methods.
     50The tests for the ''posts'' rest controller where really helpful, but I tried to break them up into single test methods.
    5051
    51 The patch also adds another revision and the total revision count to the current fixtures, to make it more usable by the tests. I was hesitated to do this, but I ended up doing this as it seems cleaner way ;-)
     52The 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.