Add check for post_per_page to -1 in PostsPerPage sniff

[rebeccahum]

Is your feature request related to a problem?

Since -1 means no limit, it would be nice to add a check for -1 being passed in as well in WordPress.WP.PostsPerPage.posts_per_page_posts_per_page

Rationale

The PostsPerPage sniff already gives a warning for high pagination - that is, over 100 by the public property default.

While for small sites, a value of -1 may only return a few results, it is still more efficient to limit the number to 10, or 50, or 100, than it is to set it to -1. A value of -1 is lazy and doesn't scale.

Describe the solution you'd like

We can have a public property of $allow_unlimited_posts to enable/disable the check for those who want to set an unlimited posts_per_page. Whether this property has an initial value of true or false is open to discussion, but adding it and using it as part of the check allows it to be skipped if necessary.

[jrfnl]

This used to be in a VIP sniff which we have just removed at the request of the VIP team.

If a developer made a conscious choice to use -1, this generally will be in a plugin where this is considered a feature, so I see no reason why WPCS should flag this.

[GaryJones]

I've just updated the OP with my thoughts from my discussion with @rebeccahum that didn't get added initially.

WordPress.com VIP has requirements regarding no limit queries. The VIPCS uses the WPCS PostsPerPageSniff as a means to flag queries with posts_per_page or numberposts set to over 100, and we have our own NoPaging sniff to catch the nopaging arg.

What we don't have is, is a way to capture posts_per_page or numberposts with a value of -1, which is arguably worse than a value of 101.

A developer may have made a conscious choice to choose -1, or they did it out of laziness and inexperience.

If you feel that -1 should not be flagged by default, then that's fine, a public property can be set accordingly. But I do think that improving the sniff to make it more flexible to allow that for those who do want to flag it, is a benefit without any significant drawbacks to everyone else.

Myself or @rebeccahum would happily do the PR for this. Sure, we could write our own sniff for VIPCS, but that limits it to those folks using VIPCS, rather than all those who are using WPCS by itself or one of the standards built on top of WPCS, and who have an eye on performance considerations.

[jrfnl]

1. Why was this not raised when the VIP sniffs were being deprecated ? That would have been the right time to address this.
2. The sniff has been deprecated for six months before being removed and in that time, we haven't had any complaints about removing it either, aside from Enforcing equal white space between associative arrays key => value is not readable in many cases #1513 which IMO is based on a misunderstanding (docs should have been updated, not the sniff).

As clearly nobody else seemed to think it was a bad idea to remove that sniff, I consider this a VIP issue, not a WPCS issue.

Implementation-wise, introducing a new property for something which used to be a separate errorcode feels even more inconsistent, than removing something and then adding it back.

[GaryJones]

For my convenience, here is the PostsPerPageSniff at WPCS 0.14.1, before it was amended and deprecated in 1.0.0.

To answer your questions:

1. I didn't work for VIP then, and so wasn't as aware of their requirements as I am now. I also wasn't as aware of all of what the individual WPCS sniffs did, or appreciate what the split in this particular sniff would mean once the VIP sniffs were deprecated.

   I can't speak for the rest of the VIP team not raising it at the time, but I suspect that the loss of this particular check was simply not noted amongst the large commit that brought over many of the deprecated VIP sniffs, but opted to reference the WPCS PostsPerPage sniff.

2. There was a comment in November 2018 regarding the lack of check for -1.

   And I created a ticket related to the use of -1, that was originally in the VIPCS repo in October 2018, so the desire to have the check has been present.

   As I'm sure you'll appreciate though - not getting violations for something that was reporting before, is not as obvious as getting them for ones that weren't being reported before, so a lack of complaints isn't a good metric here. To be fair, we don't see many complaints about any sniffs in the issues here.

This issue isn't a complaint though, but a request to improve WPCS to make it more flexible without any drawbacks to other users. Other than the history which brought us to this point, and your dislike of Automattic / VIPCS, do you have any reason for not supporting this enhancement?

Implementation-wise, introducing a new property for something which used to be a separate errorcode feels even more inconsistent, than removing something and then adding it back.

If there's a more suitable way to make this optional, from the code we currently have in develop, then I'm open to suggestions.

[jrfnl]

your dislike of Automattic / VIPCS,

@GaryJones WTF ? What sort of nonsensical personal attack is that ? I have nothing against VIPCS or Automattic, I do object to commercial companies making forceful demands of open source volunteers without putting any resources towards those projects.
And yes, certain employees involved with the VIPCS project have had a tendency to behave like that in the WPCS space, which cannot be said about any of the other big WP agencies.

All the same, for this issue I'm trying to establish if this is something which should be solved in WPCS, same as I would do with any issue raised.
Especially as it is something which is clearly contentious as it was deprecated and removed before.

So far, I have only seen arguments backed by a VIP requirement. If there is demand for this by a larger part of the WP community, other than VIP, or a strong technical argument, it's worth considering, but I have not seen that so far.

I suggest leaving this issue open for a while to see if more people - outside of VIPCS - are interested in this.

[dingo-d]

For me it's a tricky issue: On one hand I never allow my colleagues to use posts_per_page => -1, as it truly is detrimental to query performance (and a noticeable one, especially when dealing with larger amount of posts). I got faster query when setting this parameter to 10000 than when it's -1. This is my argument for leaving it (personal preference). I even modified the sniff to take in the account the limit that you could override in your phpcs.xml.dist.

On the other hand, who are we to limit what a plugin/theme author wants to do? Maybe they need this to filter some data out, in which case they really do need all posts returned, and this will prevent any issues if the entity they are querying out grows larger than an arbitrary large number they set in posts_per_page at one point in time.
Setting it as 10000 is high, but someone can have 10001 custom data in the database, and that 1 entry will be left out.

I'm fine in leaving it out of WPCS, if you need it in your own coding standard, either pull it from VIP CS, or create your own sniff (what I'll probably do) 🙂

[GaryJones]

What sort of nonsensical personal attack is that ?

I'm sorry you felt that was a personal attack. I've misunderstood your feelings, and for that, I sincerely apologise.

[GaryJones]

On the other hand, who are we to limit what a plugin/theme author wants to do?

I understand it's contentious, and I'm not advocating that it be enabled by default.

I even modified the sniff to take in the account the limit that you could override in your phpcs.xml.dist.

Right - that would be another good improvement. 100 was presumably decided from the historical VIP decision, but there's no reason that is a perfect suggestion for every site.

My request here is to make the sniff more flexible - and I can't see any technical argument why this idea would be rejected. We've got public properties for things like whitespace in array alignment, yet the ideas for improvement for PostsPerPage are arguably more important in terms of making a difference to site performance.

[jrfnl]

Right - that would be another good improvement. 100 was presumably decided from the historical VIP decision, but there's no reason that is a perfect suggestion for every site.

That number is already configurable and has been since WPCS 0.14.0...

[rebeccahum]

I do object to commercial companies making forceful demands of open source volunteers without putting any resources towards those projects.

Hi Juliette!
My apologies if that is how it has come off in the past, definitely not how it was meant to be conveyed as! However, I'm happy to make PRs for any of the issues I flag (if they get accepted). 😄

[LC43]

hi, i mentioned this some time before #1531

i never got the time to get back to this tho..

[NielsdeBlaauw]

My 2 cents:

A check for -1 is useful, as the problems that come from a bad usage of this often are

1. hard to spot

• They result in errors that are not caught in PHP, such as gateway timeouts or memory issues
• Can happen at any time, without changes to code. Something might work correctly one day, and be broken the next when a certain threshold is hit.

2. hard to debug

• A system with slightly better specs might make the problem not reproducible.
• For memory issues, the error message does not directly point to the -1, but to the place where it allocated the memory, making it hard to find the root-cause.
• For timeouts PHP might not say anything, and the webserver simply shows it killed a process.

We can have a public property of $allow_unlimited_posts to enable/disable the check

This doesn't feel right. I agree with @jrfnl here.

[GaryJones]

Instead of adding a boolean flag, @jrfnl suggested it could be an array of values that are checked for.

I had a really quick play with the sniff, and by adding:

	public $limits = array(
		array( '>', 100 ),
	);

...and:

		foreach ( $this->limits as $limit ) {
			eval( '$result = ' . "$val $limit[0] (int) $limit[1];" );
			if ( $result ) {
				return 'Detected disallowed pagination limit, `%s` is set to `%s`';
			}
		}

...in the callback method, I was able to replicate the behaviour. More work would need to be done to make this transition smooth though, to support $posts_per_page and $limits, so I've not done a PR.

That would allow other packages to override with a custom class and property like:

	public $limits = array(
		array( '>', 100 ),
		array( '===', -1 ),
	);

...or add the properties into the XML config file.

I've not seen any other requests for having multiple limits or values though, besides this one for -1, so it does seem like building in redundant flexibility, rather than allowing for a dev decision on the special case of -1.