Implement the extra rules as suggested in #607. #637

Merged
merged 2 commits into from Jan 8, 2017

Projects

None yet

3 participants

@jrfnl
Contributor
jrfnl commented Jul 26, 2016

To be merged after 0.10.0 has been released so we can test & fine-tune before it's released.

Fixes #607.

@jrfnl jrfnl added this to the 0.11.0 milestone Jul 26, 2016
@GaryJones

There's about 8 instances where the else is correctly not used, and a pre-if default is set. However, there are two instances where else is used.

- $callback = ( isset( $group['callback'] ) && is_callable( $group['callback'] ) ) ? $group['callback'] : array( $this, 'callback' );
+ if ( isset( $group['callback'] ) && is_callable( $group['callback'] ) ) {
+ $callback = $group['callback'];
+ } else {
@GaryJones
GaryJones Sep 15, 2016 Member

This else could be set as the default before the if.

callback = ...
if ( ... ) {
    callback = ...
}
@jrfnl
jrfnl Dec 27, 2016 Contributor

Rebased & fixed.

WordPress/Sniff.php
- $end = ( 0 === $start ) ? count( $tokens ) : $tokens[ $start ]['scope_closer'];
+ if ( 0 === $start ) {
+ $end = count( $tokens );
+ } else {
@GaryJones
GaryJones Sep 15, 2016 Member

This else could be set as default before the if.

WordPress-Extra/ruleset.xml
+ <rule ref="Squiz.PHP.NonExecutableCode"/>
+ <rule ref="Squiz.Operators.IncrementDecrementUsage"/>
+ <rule ref="Squiz.Operators.ValidLogicalOperators"/>
+ <rule ref="Squiz.PHP.DisallowComparisonAssignment"/>
@GaryJones
GaryJones Sep 15, 2016 Member

This basically seems to forbid the use of ternary conditions?

@jrfnl
jrfnl Sep 15, 2016 Contributor

It's been a while since I looked at this one. Will check & get back to you after the weekend (conference).

@jrfnl
jrfnl Dec 27, 2016 Contributor

Sorry for the silence. Finally had a chance to look at this in properly.

By the looks of it, it does (forbid assignments using ternary conditions), though it is more specifically intended to catch things like $var = ($foo === $bar).
See the unit tests.
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Tests/PHP/DisallowComparisonAssignmentUnitTest.inc
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Tests/PHP/DisallowComparisonAssignmentUnitTest.php

Would it be an idea to lower the error level for this sniff from error to warning ?

@jrfnl jrfnl was assigned by GaryJones Sep 15, 2016
@GaryJones
Member

I think the ternary issue is the most contentious issue here - even the basic $a = $b ? $c : $d; throws an error. I'm generally fine with the rest.

@jrfnl
Contributor
jrfnl commented Jan 3, 2017

@GaryJones So what would you suggest - remove that sniff altogether (for now, possibly to be revisited later), or lower the errors from that sniff to warnings ?

@GaryJones
Member

I'd say remove for now, but definitely have discussion for future review.

@jrfnl
Contributor
jrfnl commented Jan 3, 2017

@GaryJones Ok, I've had another look and I think it's not just Squiz.PHP.DisallowComparisonAssignment we'd need to exclude in that case, but also Squiz.PHP.DisallowInlineIf which also looks at those kind of statements.
See: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Tests/PHP/DisallowInlineIfUnitTest.inc

Either way, we're still not covering the example as used in the handbook which manages to bypass both these sniffs: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#clever-code

Shall I just remove both for now and open a new PR for these two sniffs ?
In that case, I will re-do this PR so as to remove the code adjustments which were made based on these two rules (and move those to the other PR).

jrfnl added some commits Jan 3, 2017
@jrfnl jrfnl Implement some extra existing rules from upstream 2cf93d4
@jrfnl jrfnl Fix issues detected by the new rules in the WPCS code base.
b7ad6ca
@GaryJones
Member

Shall I just remove both for now and open a new PR for these two sniffs ?

I think that's the way to go for now.

@jrfnl
Contributor
jrfnl commented Jan 3, 2017

@GaryJones Ok, thanks for working with me on this. I've rebased & re-done the commits. This PR now only contains the rules which are without opinion on ternaries.
The more controversial part of the PR has been moved to PR #768

+ return;
+ }
+ }
+ unset( $prev, $pprev );
@grappler
grappler Jan 3, 2017 Contributor

If $prev is false then $pprev is never defined.

@jrfnl
jrfnl Jan 3, 2017 Contributor

Correct, but that doesn't matter for an unset(). No notices will be thrown and if the variable is set, it will be correctly unset.

- $token = $tokens[ $stackPtr ];
+ $tokens = $phpcsFile->getTokens();
+ $token = $tokens[ $stackPtr ];
+ $token_content = strtolower( $token['content'] );
@GaryJones
GaryJones Jan 4, 2017 Member

Is this $token_content not unrelated to the rest of the changes going on here? i.e. does it deserve it's own issue (to describe whatever bug this strtolower() is fixing) and PR?

@jrfnl
jrfnl Jan 4, 2017 Contributor

The quite convoluted if() statement below it was triggering the Squiz.PHP.DisallowMultipleAssignments sniff (twice), this probably slipped in as I was looking to simplify the code.

Function names are case-insensitive in PHP, so should always be lowercased before comparisons are done.
I'm happy to move that bit to another PR (I will probably include it in one of the review PRs which I've mentioned in some recent issues in that case).

@@ -167,7 +167,8 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
}
$whitelisted_cache = false;
- $cached = $wp_cache_get = false;
+ $cached = false;
+ $wp_cache_get = false;
@GaryJones
GaryJones Jan 4, 2017 Member

WordPress/Sniff.php, you have split out the multiple assignments as A = X, and then B = A.

Here, you have a pattern of A = X and B = X.

Is there any reason for this inconsistent approach? (There may well be, and that's fine if so)

@jrfnl
jrfnl Jan 4, 2017 edited Contributor

Oh, there's a quite simple explanation for that:
In WordPress/Sniff.php we're assigning the output of a function call to a variable and if a second variable needs the same value, there is no need to repeat the function call (efficiency).
In the above code, we're assigning the PHP native constant false, which carries no overhead in contrast to the function call.

[Edit]: same goes for the function call assignment in WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php

@GaryJones
GaryJones Jan 4, 2017 Member

I think there could also be an aspect of semantics here too - the last pointer IS the end of the line, where as the cached thing is not the same as the WP cache get thing. If so, then the current code satisfies that, so all good.

@jrfnl
Contributor
jrfnl commented Jan 7, 2017

As all the more contentious extra rules have now been (re)moved from this PR, can this be merged ?

@GaryJones GaryJones merged commit 99bb6b7 into develop Jan 8, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@GaryJones GaryJones deleted the WPCS/feature/issue-607-extra-rules branch Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment