Fix missing check for infinite posts_per_page

[LC43]

Hi,

the class description says "Flag returning high or infinite posts_per_page.". It was missing this last check: '-1' === $val, which i added it here:

if ( $val > $this->posts_per_page || '-1' === $val ) {
	return 'Detected high pagination limit, `%s` is set to `%s`';
}

This infinite (-1) limit is worse that high fixed limit. Like other have mentioned, this is a good thing to check.

This will fix #1379 in cases where the value is not a variable.

I also introduced a new test:

$query_args['posts_per_page'] = '-1'; // Bad.

Not sure if i should add more cases, as they are missing.

Also, i didn't touch the wrong indentation on the last tests. There's a space at the beginning of each line. Can i remove it?

// @codingStandardsChangeSetting WordPress.WP.PostsPerPage posts_per_page 50
 $query_args['posts_per_page'] = 50; // OK.
 $query_args['posts_per_page'] = 100; // Bad.
 $query_args['posts_per_page'] = 200; // Bad.
 $query_args['posts_per_page'] = 300; // Bad.
// @codingStandardsChangeSetting WordPress.WP.PostsPerPage posts_per_page 200
 $query_args['posts_per_page'] = 50; // OK.
 $query_args['posts_per_page'] = 100; // OK.
 $query_args['posts_per_page'] = 200; // OK.
 $query_args['posts_per_page'] = 300; // Bad.

All tests look good:

$phpunit --bootstrap="./Test/phpcs3-bootstrap.php" --filter WordPress ./vendor/squizlabs/php_codesniffer/tests/AllTests.php
PHPUnit 6.5.5 by Sebastian Bergmann and contributors.

................................................................. 65 / 80 ( 81%)
...............                                                   80 / 80 (100%)

Tests generated 580 unique error codes; 52 were fixable (8.97%)

Time: 2.58 seconds, Memory: 36.00MB

OK (80 tests, 0 assertions)

thanks, let me know if i missed something.

Fixes #1638

[GaryJones]

This is checking a value that is a string, not a negative integer.

[LC43]

hi, i checked it, and $val is always converted to string. not sure why or where, didn't look into it deeply. see below.

that's why the tests pass, either with -1 and with '-1'

[LC43]

the token is parsed into a string here:

// this->tokens
23: - // "PHPCS_T_MINUS"
24: 1 // "T_LNUMBER"
25: , // "PHPCS_T_COMMA"

// call: WordPress/AbstractArrayAssignmentRestrictionsSniff.php L170
// valStart: 23
// valEnd: 25
$val            = $this->phpcsFile->getTokensAsString( $valStart, ( $valEnd - $valStart ) );

[jrfnl]

@LC43 Thank you for your willingness to contribute.

Unfortunately, the PR as it stands is, IMO, not the way to go.

First of all, there is already a sniff available which checks for posts_per_page being set to -1: the WordPress.VIP.PostsPerPage sniff.

That sniff originally contained the check for "high posts_per_page" too, but the sniff was split in PR #1260 / WPCS 1.0.0 in response to a proposal for this made in issue #1157.

Since then, the VIP team at Automattic have requested to have the VIP sniffs deprecated and - in the near future - removed from WPCS. See #1309, #1409 and the PR which eventually deprecated the sniffs #1410.

If your problem is that - except for the (deprecated) VIP ruleset, WPCS currently does not include the sniff in runs, you can - for now - choose to (re-)activate the deprecated sniff by adding it in your own custom ruleset and setting the severity to 5.

If that's not the issue and you think that the deprecated sniff should be retained, then I think a discussion is needed first, whether WPCS should still contain a sniff checking for infinite posts per page going forward.

If there is enough support for that, there are a couple of options:

• Revert the splitting of the VIP sniff and have the complete code of the "old" sniff in the WP category (with the VIP sniff extending that sniff until it is removed).
• Leave the VIP sniff as a stand-alone sniff, but un-deprecate it. This will be problematic once the whole VIP category is removed, however, that's solvable as the sniff could be moved to the WP category with a name adjustment and the deprecated VIP one could extend the "new" sniff until it is removed.

All in all, your current proposal can not be accepted as is and I think an issue should be opened to discuss what you're really trying to achieve here and what would be the best way to go about it.
This PR should, IMO, be closed until there is clarity about that.

the class description says "Flag returning high or infinite posts_per_page."

That is a left over from before the sniff was split and should be corrected to "Flag requesting high number of posts_per_page."

This will fix #1379 in cases where the value is not a variable.

Issue #1379 relates to the deprecated WordPress.VIP.PostsPerPage sniff, not to the sniff you have adjusted for this PR.

[LC43]

hi @jrfnl, no problem, thanks for taking your time to review this.

I'll open a ticket as soon as i have better understanding of these deprecations.