[Implement sniff] Warn about short ternary usage in themes

[dingo-d]

Basic Info	-
Rule type:	Warning
Sniff Category:	Pulled from WPCS

Rule:

Core handbook specifies that short ternaries shouldn't be used.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#ternary-operator

Short ternaries look like

$some_result = $some_value ?: 'default';

In this case the value of $some_result will be $some_value unless the $some_value evaluates to false, in that case, the value of $some_result will be the string 'default'.

Inspecting the usage in the theme directory I saw that most authors are using it wrongly - they assume that the value of the array key will be used if it exists

$output = isset( $input['key'] ) ?: 'default';

However, the value of $output will never be $input['key'].It will either be default string in case the key doesn't exist or true if it does. Which is (in the majority of cases) not intended behavior.

The intended behavior is (probably)

$output = $input['key'] ?: 'default';

But if the $input['key'] doesn't exist, the above will throw an undefined index notice. Most likely the operator that the author intended to use is null coalesce (??).

Decision needed:

A decision is needed whether we'd include this in the first place or no?

If we include it, it could be downgraded to warning to be a caution for theme authors. On the other hand, reviewers who see this warning could report this to an author, even if they really used this with a correct intention. That could cause confusion during the review process. Although it would be a good way for both authors and reviewers to distinguish the difference between the short ternary (?:) and null coalesce operators (??).

To do:

• [ ] Add the rule in the Theme Review handbook to the Requirements page if the rule is agreed upon to be required.
• [ ] Add the rule in the Theme Review handbook to the Recommended page if the rule is agreed upon to be recommended.
• [ ] Add existing sniffname sniff to the ruleset.

[joyously]

?? usage would influence the minimum PHP version for the theme, wouldn't it?

[dingo-d]

Once core moves to PHP 7.0 (beginning of the next year) this shouldn't have to be an issue imo.

[joyously]

Once core moves to PHP 7.0

But this is for themes, not core. And themes can run on different versions of core, with different versions of PHP.

[jrfnl]

Both ?: (PHP 5.3+) usage, as well as ?? (PHP 7.0+) usage influence the minimum PHP version for a theme.

If I remember correctly, the "default" presumption is that themes support the current WP version + up to three versions back. That would currently mean WP 5.0 - 5.3, meaning the "normal" minimum PHP version for a theme would still need to be PHP 5.2, though this can be changed of course with the Requires PHP: header.

[joyously]

Yes, but considering that WP doesn't check the Requires PHP header for themes yet, the theme still needs to make sure that it doesn't fatal. And a warning to the reviewer would be good to have.

[jrfnl]

Well, the PHPCompatibilityWP ruleset which is included in TRTCS already does so as the minimum PHP version for that is set to 5.2.

[dingo-d]

The decision from the triage: include this sniff but change it so that it throws a warning instead of an error.

This shouldn't be too hard, just extend the sniff and override the process_token method

[jrfnl]

No need to extend the sniff, this can simply be changed from the ruleset <type>warning</type>.

[dingo-d]

Even better 🙂