Skip to content
This repository was archived by the owner on Sep 24, 2018. It is now read-only.

Avoid a duplicate query for the comment count - #2015

Merged
danielbachhuber merged 7 commits into
developfrom
comment-count-query
Jan 13, 2016
Merged

danielbachhuber merged 7 commits into
developfrom
comment-count-query

Conversation

@rmccue

@rmccue rmccue commented Jan 13, 2016

Copy link
Copy Markdown
Member

Back in 768a282 (#1197), we introduced the regular pagination headers for comment queries. However, apparently we missed that WP_Comment_Query has SQL_CALC_FOUND_ROWS built-in (but off by default), so we're doing a complex query twice here.

Attached patch uses $query->found_comments and $query->max_num_pages instead of redoing the query.

@rmccue

rmccue commented Jan 13, 2016

Copy link
Copy Markdown
Member Author

This was introduced originally in #1209, since (as the failing test shows) out-of-bounds pages will give back 0 for SQL_CALC_FOUND_ROWS. Unfortunately, this hurts common-case performance (in-bounds requests) to support an edge case (out-of-bounds requests).

Changed this to using the normal query on in-bounds requests, and only running the secondary query if we're out of bounds. Updating other controllers now.

@rmccue

rmccue commented Jan 13, 2016

Copy link
Copy Markdown
Member Author

@WP-API/amigos #reviewmerge

@danielbachhuber

Copy link
Copy Markdown
Member

Looks good 👍 And well-covered by existing tests.

danielbachhuber added a commit that referenced this pull request Jan 13, 2016
Avoid a duplicate query for the comment count
@danielbachhuber
danielbachhuber merged commit 01464fb into develop Jan 13, 2016
@danielbachhuber
danielbachhuber deleted the comment-count-query branch January 13, 2016 13:15
@danielbachhuber

Copy link
Copy Markdown
Member

Related:

Although, in this case, I think this PR is still better than what we had before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants