Make WordPress Core

Opened 6 years ago

Last modified 2 years ago

#42947 assigned defect (bug)

REST API wrong total pages

Reported by: elvishp2006's profile elvishp2006 Owned by: spacedmonkey's profile spacedmonkey
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.1
Component: REST API Keywords: has-screenshots has-patch has-unit-tests early
Focuses: rest-api Cc:

Description

When I require posts with the draft status the 'x-wp-total' and 'x-wp-totalpages' headers come with extra values ​​being that not all drafts are from the user making a request, so when I try to pick up the second page the answer comes empty.

https://i.imgur.com/223sEE3.gif

Attachments (1)

42947.diff (3.8 KB) - added by audrasjb 2 years ago.
patch refreshed against trunk

Download all attachments as: .zip

Change History (26)

#1 @elvishp2006
6 years ago

  • Keywords has-screenshots added

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


6 years ago

#3 @schlessera
6 years ago

To make this work, I think WP would need to make two separate queries when checking for draft posts and published posts at the same time.

So, the logic would be:

If status includes draft:

  • remove draft from status and run query
  • run second query for draft status where the author is the current user
  • sum the count of both queries

@timothyblynjacobs mentioned that this might not work as expected due to capability checks, though.

#4 @wongalex
5 years ago

I can take a look at this as my first real bug and get a diff for the above out (assuming, everything functions and it fixes the total pages).

#5 @TimothyBlynJacobs
4 years ago

#49187 was marked as a duplicate.

This ticket was mentioned in PR #655 on WordPress/wordpress-develop by TimothyBJacobs.


3 years ago
#6

  • Keywords has-patch has-unit-tests added

#7 @TimothyBlynJacobs
3 years ago

I spent some time digging on this. The root cause of this issue is that a post being returned from WP_Query is not a guarantee that it will be included in the response because the current user might not have permission to read it.

We can't really do separate queries, because we can't paginate over two separate queries.

So I would say that clients should be prepared to handle responses that have less results than a full page. Additionally, they should continue to paginate as long as their is a next link, even if that page is partial or empty.

However, WP_Query does support a perm argument which can check attach a post_author clause if the user doesn't have the permissions to read/write that post status. WordPress applies this in the admin list tables using wp_edit_posts_query().

This only applies to statuses that are marked private. So it doesn't include drafts which are marked as protected, so this wouldn't help with your problem in particular, but would still be an improvement nonetheless.

The only potential issue is that this means that posts that have granted read/edit capability to posts marked private using map_meta_cap wouldn't appear in the collection endpoint by default. I don't think this is a significant issue since the same thing applies in the admin list tables. Note, the post would still be accessible via it's single get_item route.

I've opened a PR that implements that behavior.

#8 @hellofromTonya
3 years ago

  • Keywords early added
  • Milestone changed from Awaiting Review to 5.7

In talking with Timothy, moving this ticket into 5.7 and marking as early to get it into the cycle as early as possible and give more attention to it early on.

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


3 years ago

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


3 years ago

#11 @TimothyBlynJacobs
3 years ago

  • Milestone changed from 5.7 to 5.8

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

#17 @chaion07
3 years ago

  • Keywords needs-refresh added

From the [discussion]https://wordpress.slack.com/archives/C02RQBWTW/p1618946568378600 during our recent bug-scrub, we consulted that this ticket needs a refresh since it does not apply cleanly anymore

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


3 years ago

#19 @desrosj
3 years ago

  • Milestone changed from 5.8 to 5.9

This has not seen any progress during the 5.8 cycle, so punting to 5.9.

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


2 years ago

@audrasjb
2 years ago

patch refreshed against trunk

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


2 years ago

#22 @audrasjb
2 years ago

  • Keywords needs-refresh removed

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


2 years ago

#24 @spacedmonkey
2 years ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#25 @TimothyBlynJacobs
2 years ago

  • Milestone changed from 5.9 to Future Release

I'm moving this to a Future Release, there are some other moving parts that need to be looked at before this can be committed.

Note: See TracTickets for help on using tickets.