Query breaks when nopaging = false and posts_per_page = -1

[mgibbs189]

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.

[swissspidy]

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?)

[mgibbs189]

@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.

[boonebgorges]

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.

[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?

[mgibbs189]

@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 :)

[boonebgorges]

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.