WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#38378 closed defect (bug) (fixed)

Remove the `filter` parameter in the Post Controller

Reported by: websupporter Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch needs-refresh
Focuses: Cc:

Description

As discussed in https://github.com/WP-API/WP-API/issues/2799 the team decided to drop support for the filter parameter. It introduced an "inconsistent mess" and "exposing WP_Query through filter has and will continue to be difficult to support."

Attachments (3)

38378.diff (8.0 KB) - added by websupporter 3 years ago.
38378.2.diff (8.3 KB) - added by websupporter 3 years ago.
38378.3.diff (8.1 KB) - added by websupporter 3 years ago.
but of course we need to set the posts per page if given…

Download all attachments as: .zip

Change History (12)

@websupporter
3 years ago

#1 @websupporter
3 years ago

  • Keywords has-patch added

#2 @websupporter
3 years ago

  • Keywords needs-refresh added

Will update the patch. Lines 169-170 need to be included.

@websupporter
3 years ago

#3 @websupporter
3 years ago

Our calculation of the max. pages needed posts_per_page to be set in our $query_args. In order to overwrite filter we did so before. When I deleted the filter stuff, I also deleted this, which produced a bug.

Since we always make sure, we add stuff only, when it is given in the schema like

<?php
if ( isset( $registered['per_page'] ) ) {
    $args['posts_per_page'] = $request['per_page'];
}

I thought it might not be the best to rely on posts_per_page is set. In 38378.2.diff I use the $posts_query->query_vars instead:
$max_pages = ceil( $total_posts / (int) $posts_query->query_vars['posts_per_page'] );

@websupporter
3 years ago

but of course we need to set the posts per page if given...

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


3 years ago

#5 @kadamwhite
3 years ago

Technically if somebody has de-registered per_page as a valid parameter, they're probably extending this to write their own endpoint and have voided their pagination warrantee in the process. I'm not sure it makes sense to me to special-case this, when so many other properties that the API depends upon for proper functioning are whitelisted in the same general way using $registered. I would defer to @rmccue and @joehoyle on this, though.

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


3 years ago

#7 @rachelbaker
3 years ago

  • Owner set to rachelbaker
  • Status changed from new to accepted

I started working on this earlier, and am finishing up with some test fixes. I will have this committed soon.

#8 @rachelbaker
3 years ago

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

In 38968:

REST API: Remove experimental filter wrapper parameter from the Posts Controller class.

Hiding WP_Query params under the filter key (instead of allowing them to be top-level params) was one of our biggest complaints from users of v1 of our REST API. This walks back the re-introduction of the filter param during Beta 15, which introduced an "inconsistent mess" and "exposing WP_Query through filter has and will continue to be difficult to support." See https://github.com/WP-API/WP-API/issues/2799.

Props websupporter, rachelbaker.
Fixes #38378.

#9 @netweb
3 years ago

  • Milestone changed from Awaiting Review to 4.7
Note: See TracTickets for help on using tickets.