Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#34660 new defect (bug)

Query breaks when nopaging = false and posts_per_page = -1

Reported by: mgibbs189's profile mgibbs189 Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Query Keywords: has-patch needs-docs needs-unit-tests
Focuses: Cc:

Description

Whenever nopaging = false and posts_per_page = -1, the SQL query breaks because the LIMIT clause is set to: LIMIT 0, -1

This patch ensures that nopaging is set to TRUE whenever posts_per_page = -1.

Attachments (1)

query.php.patch (538 bytes) - added by mgibbs189 8 years ago.

Download all attachments as: .zip

Change History (8)

@mgibbs189
8 years ago

#1 @mgibbs189
8 years ago

  • Keywords has-patch added

#2 @swissspidy
8 years ago

This was recently discussed in #34474. Quoting @boonebgorges:

What seems to be breaking the query in your case is that you're passing 'nopaging' => false along with 'posts_per_page' => -1. Generally, if WP_Query sees 'posts_per_page' => -1, it'll set nopaging to true.

I don't think there's really anything we should do in WP_Query to account for this case, where you've passed contradictory parameters. (How would we decide which one you "really" meant?)

#3 @mgibbs189
8 years ago

@swissspidy This was discovered from a compatibility issue between 2 plugins.

While this is an edge-case, these are both built-in query arguments, and a little extra error handling can't hurt. Whenever posts_per_page = -1, it doesn't make sense to use nopaging anyways.

#4 @boonebgorges
8 years ago

  • Keywords needs-docs needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

While this is an edge-case, these are both built-in query arguments, and a little extra error handling can't hurt. Whenever posts_per_page = -1, it doesn't make sense to use nopaging anyways.

You might also say that when nopaging=true, posts_per_page should be ignored. That's what I meant when I asked "How would we decide which one you really meant".

That being said, it's fine to me if one wins out over the other. But before we do that, let's have (a) unit tests describing the order of precedence, and (b) documentation, either inline or in the docblock, that explains how it works.

#5 follow-up: @mgibbs189
8 years ago

You might also say that when nopaging=true, posts_per_page should be ignored.

@boonebgorges I appreciate the response, but that's an entirely separate issue.

This ticket addresses the specific case when posts_per_page = -1 and nopaging = false. In this scenario, nopaging must be ignored to prevent an SQL error.

This is because -1 isn't a valid SQL limit, and forcing nopaging = true is the only way to bypass the LIMIT clause. Know what I mean?

#6 @mgibbs189
8 years ago

@boonebgorges Regarding the other scenario:

When nopaging = true, an elegant solution could be to override posts_per_page, similar to how showposts currently does it. E.g.

if ( isset($q['nopaging']) && $q['nopaging'] ) {
    $q['posts_per_page'] = -1;
}

IMO, I'd love to see nopaging disappear (like with showposts), but that's for another day :)

#7 in reply to: ↑ 5 @boonebgorges
8 years ago

Replying to mgibbs189:

You might also say that when nopaging=true, posts_per_page should be ignored.

@boonebgorges I appreciate the response, but that's an entirely separate issue.

This ticket addresses the specific case when posts_per_page = -1 and nopaging = false. In this scenario, nopaging must be ignored to prevent an SQL error.

This is because -1 isn't a valid SQL limit, and forcing nopaging = true is the only way to bypass the LIMIT clause. Know what I mean?

I understand the technical issue. What I'm saying is this. nopaging = false says "paginate results". posts_per_page = -1 says "don't paginate results". They are the opposite of each other. Your patch says that, in such cases, posts_per_page = -1 will always win. We could make the opposite decision, namely that nopaging = false should always win in such cases. (I said nopaging = true in my comment above, but I was getting confused by double-negatives. My bad.)

I agree that it's probably correct for posts_per_page = -1 to take precedence, because (a) nopaging kinda stinks, and (b) nopaging is disabled by default. But it's something that needs documentation, and especially tests that describe the decision we're making.

We may also want to distinguish between ! isset( $q['nopaging'] ) and false === $q['nopaging'], because they have pretty different meanings in terms of the default WP_Query params.

Note: See TracTickets for help on using tickets.