Implement ternary related extra rules #768

Open
wants to merge 2 commits into
from

Projects

None yet

4 participants

@jrfnl
Contributor
jrfnl commented Jan 3, 2017

Clever Code

In general, readability is more important than cleverness or brevity.

isset( $var ) || $var = some_function();

Although the above line is clever, it takes a while to grok if you’re not familiar with it. So, just write it like this:

if ( ! isset( $var ) ) {
    $var = some_function();
}

See: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#clever-code

This part is so far not really covered in WPCS. Existing sniffs which come close - but still don't cover this completely - relate to inline if statements and comparison assignments.

This PR is intended to have a discussion on whether or not to add these sniffs to the rulesets.

@jrfnl jrfnl added this to the Future Release milestone Jan 3, 2017
@JDGrimes
Contributor
JDGrimes commented Jan 3, 2017

This really has nothing to do with ternaries. Actually, it is about using the || and && operators without ternaries or an if statement. If you use a ternary or an if statement, then you are OK.

@grappler
Contributor
grappler commented Jan 3, 2017 edited

I think Squiz.PHP.DisallowInlineIf is covered best by:

Note that requiring the use of braces just means that single-statement inline control structures are prohibited.

This is documented here https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

Edit: I am getting confused by some of the technical terms. It seems like the ternary operator is ok https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#ternary-operator

@jrfnl
Contributor
jrfnl commented Jan 3, 2017

@grappler "single-statement inline control structures" are if/for/foreach/switch etc statements with only one "child" statement. PHP doesn't require braces in that case, but it is best practice to use braces anyhow:

// Basically, don't do this:
if ( false === $var ) $var++;

// Or this:
if ( false === $var )
	$var++;
else
	$var--;
@GaryJones
Member

I'm definitely in favour of adding DisallowInlineIf to Core ruleset - the Handbook explicitly covers that:

Braces should always be used, even when they are not required


My concern was with the other bit - why something like:

$method    = empty( $context['warning'] ) ? 'addError' : 'addWarning';

in our sniff needs changing to:

$fixable_method = 'addFixableWarning';
if ( empty( $context['warning'] ) ) {
 	$fixable_method = 'addFixableError';
}

to comply with the DisallowComparisonAssignment in the Extra ruleset - this is the bit that suggests that no ternary conditions would be allowed.

If you use a ternary or an if statement, then you are OK.

If @JDGrimes is right, why has some WPCS code been updated to comply with the new (potential) ruleset?

@jrfnl
Contributor
jrfnl commented Jan 4, 2017

Ok, looks like there's some confusion over the issue here.

One the one hand we have the handbook which disallows the use of control structures without braces.
That rule is actually already covered by the Squiz.ControlStructures.ControlSignature sniff and is not under discussion.

The current rule suggestions are about Clever code - code which reduces readability/ease of comprehension.

The Squiz.PHP.DisallowInlineIf sniff is the one which is most often triggered in our code base and necessitated the code changes. That one effectively disallows ternaries in combination with assignments.
See: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Tests/PHP/DisallowInlineIfUnitTest.inc - line 8 triggers this sniff

The Squiz.PHP.DisallowComparisonAssignment sniff is the one which comes closer to matching the example code in the Clever code section, even though it still doesn't match it. It also, like above, gets triggered by ternaries.
See: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Tests/PHP/DisallowComparisonAssignmentUnitTest.inc - line 3, 5, 6, 7, 8, 10, 52, 53 trigger the sniff

To make it clearer which rule necessitated which change in the current codebase, I'll annotate the changes in the diff so it's clearer what we're talking about.

+ $callback = array( $this, 'callback' );
+ if ( isset( $group['callback'] ) && is_callable( $group['callback'] ) ) {
+ $callback = $group['callback'];
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Squiz.PHP.DisallowInlineIf.Found

+ $delim = '';
+ if ( T_OPEN_SQUARE_BRACKET !== $token['code'] ) {
+ $delim = '\b';
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Triggers both sniffs

+ $end = $tokens[ $start ]['scope_closer'];
+ if ( 0 === $start ) {
+ $end = count( $tokens );
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Triggers both sniffs

+ $spaced2 = false;
+ if ( T_WHITESPACE === $tokens[ ( $token['bracket_closer'] - 1 ) ]['code'] ) {
+ $spaced2 = true;
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Squiz.PHP.DisallowComparisonAssignment

+ $severity = 'warning';
+ if ( in_array( $instance['content'], $this->errorForSuperGlobals, true ) ) {
+ $severity = 0;
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Triggers both sniffs

+ if ( empty( $context['warning'] ) ) {
+ $method = 'addError';
+ }
+
@jrfnl
jrfnl Jan 4, 2017 Contributor

Triggers both sniffs

+ $fixable_method = 'addFixableWarning';
+ if ( empty( $context['warning'] ) ) {
+ $fixable_method = 'addFixableError';
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Triggers both sniffs

+ $to_insert .= '$';
+ } else {
+ $to_insert .= '\$';
+ }
@jrfnl
jrfnl Jan 4, 2017 Contributor

Squiz.PHP.DisallowInlineIf

@@ -24,7 +24,7 @@ public function getErrorList() {
return array(
3 => 1,
17 => 1,
- 30 => ( PHP_VERSION_ID >= 50300 ) ? 0 : 1,
+ 30 => (int) ( PHP_VERSION_ID < 50300 ),
@jrfnl
jrfnl Jan 4, 2017 Contributor

Squiz.PHP.DisallowInlineIf

@JDGrimes
JDGrimes Jan 4, 2017 Contributor

IMHO, this is more clever than before. 😄

@GaryJones
Member
GaryJones commented Jan 4, 2017 edited

Thanks for the explanation @jrfnl.

I think forbidding the use of $var = ! $foo;, and $a = $b ? $c : $d; is going to unnecessarily frustrate devs to the point that they'd want to turn this particular Extras check off. I wouldn't consider them clever code or unreadable code.

FWIW, I'm generally in favour of the other ternary removals in the PR, as $b in the pattern is non-trivial, even if the sniffs don't get added to the ruleset.

@JDGrimes
Contributor
JDGrimes commented Jan 4, 2017

This really has nothing to do with ternaries. Actually, it is about using the || and && operators without ternaries or an if statement. If you use a ternary or an if statement, then you are OK.

This comment was in regard to the clever code section in the handbook, and I realize now that that may not have been clear.

The clever code section in the handbook has nothing to do with ternaries or assignments. It is about using && and || to directly chain expressions, like function calls. It is not about using them in conditions, it is about using them sans conditional structures (like if statements and ternaries). At least this is the way that I have always taken it.

So a rule which would flag these would be something like: don't allow ||, &&, etc., to be used outside of conditional structures.

jrfnl added some commits Jan 3, 2017
@jrfnl jrfnl Implement some extra existing rules from upstream - covers inline if/…
…ternary assignments
2fb0643
@jrfnl jrfnl Fix issues detected by the new rules in the WPCS code base.
a50e25e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment