Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#29552 closed enhancement (worksforme)

Slight tweak to no_found_rows logic in WP_Query

Reported by: dmchale's profile dmchale Owned by: boonebgorges's profile boonebgorges
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Query Keywords: reporter-feedback needs-testing has-patch
Focuses: performance Cc:

Description (last modified by johnbillion)

It seems that edge cases could introduce times when a query WOULD have limits but NOT want them (when the query was passed nopaging=true or post_per_page=-1). When this happened, SQL_CALC_FOUND_ROWS would be invoked in the query and hinder performance of the query.

Code change still allows for a developer to override no_found_rows with their own value, but now checks the nopaging and post_per_page variables to possibly set no_found_rows to true anyway. By using one of those 2 variables to ask for EVERYTHING, we know the developer does not want/need pagination. This forces the issue so that by the time we get to the line...

if ( !$q['no_found_rows'] && !empty($limits) )

...we don't find ourselves WITH limits, and a False value for no_found_rows, in spite of the fact that the developer had asked for "all".

Attachments (2)

29552.patch (782 bytes) - added by dmchale 9 years ago.
29552.tests.patch (2.2 KB) - added by boonebgorges 9 years ago.

Download all attachments as: .zip

Change History (10)

@dmchale
9 years ago

#1 @boonebgorges
9 years ago

  • Keywords reporter-feedback added

Thanks for the patch, dmchale.

It seems that edge cases could introduce times when a query WOULD have limits but NOT want them (when the query was passed nopaging=true or post_per_page=-1)

I can't reproduce this. Check out the last two tests in 29552.tests.patch. They declare nopaging=true and posts_per_page=-1, and in each case, you can see that the request query does *not* contain SQL_CALC_FOUND_ROWS. When you have LIMIT clauses, WP_Query does not append the SQL_CALC_FOUND_ROWS keyword. See https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php?annotate=blame#L3376. See also https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php?annotate=blame#L3611 to see how found_posts is set in these cases.

Can you say more about your "edge cases"?

#2 @johnbillion
9 years ago

  • Description modified (diff)
  • Keywords needs-testing added
  • Version changed from trunk to 1.5

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


8 years ago

#4 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to boonebgorges
  • Status changed from new to assigned

Assigning to myself to review the tests. If they are passing, I'll commit them and close the ticket as invalid, pending further details. If they're not passing, then maybe we have a bug on our hands.

#5 @DrewAPicture
8 years ago

  • Keywords has-patch added

#6 @ajv9540
8 years ago

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

#7 @ocean90
8 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

#8 @boonebgorges
8 years ago

  • Milestone 4.6 deleted
  • Resolution set to worksforme
  • Status changed from reopened to closed

[37600] missed this ticket (fudged the commit message).

Tests seem OK to me, so I'm going to close as wontfix. If anyone has steps to reproduce, please feel free to reopen with details.

Note: See TracTickets for help on using tickets.