Php_codesniffer: Feature request/usage: limiting value assignment in conditional clauses

[titpetric]

Examples of problematic assignments that I think should be caught with a sniff in conditionals:

Setting literal values (T_STRING, T_NUMBER)

Example: if ($userId = 12)

Setting literal values inside a conditional clause almost never (arguably?) has the intended results. Since people generally put the expected value on the right side of the check, a missing = from == results in a possibly destructive bug.

I'd want to error on literal value assignments in conditionals fully, while expressions may be still allowed depending on additional rules (contd.).

Chained conditional assignment

Example: if (!$check->isAllowed($domain) && (($report = $check->isAbuser($domain)) !== false))

This code is problematic, as the value of $report may have an unexpected state depending on what isAllowed() returns (null if domain is allowed). Coalescing possible values with empty() is one way to be more resilient to the problem, but that's hard to enforce.

• don't allow combining conditions and assignment,
• don't allow multiple assignments in conditional clauses

---

The above being said, there are still valid use cases where assignment is done in a conditional clause like while or if:

• directory traversal: while (($file = readdir($dir)) !== false)
• single row fetching from db: if ($user = $api->get($userId)) {

Does anybody have more information about such a sniff existing? I couldn't track anything down in the docs, and testing around Squiz which is supposedly the strictest didn't come up with any warning/error regarding the above.

[jrfnl]

@titpetric While not as specific as what you are describing above, there is the Generic.CodeAnalysis.AssignmentInCondition sniff which will forbid any assignment in condition and has two error codes FoundInWhileCondition and Found. The while condition has a separate error code as - generally speaking - that is the only control structure where an intentional assignment in condition will make sense and the separate error code allows for excluding that code while still forbidding all other assignments in conditions.

Does that help ?

[titpetric]

@jrfnl it might! Thanks for the hint, I’ll go sleuthing why and how I managed to skip this rule.

[djibarian]

ESLint has the option to ignore the expression if in parentheses, so if you are intentionally assigning and testing at the same time you can express so with if(($userId = 12)). It would be good to have this option in Generic.CodeAnalysis.AssignmentInCondition.

[jrfnl]

@djibarian That sounds like arbitrarily giving an undocumented meaning to superfluous parentheses. I don't think that's a good idea.

[djibarian]

I’ve been using that pattern for years and then discovered that ESLint has that option, so I don’t think I’m alone. Maybe it results more obvious like if(!($userId = 12)) but the pattern is the same.

[jrfnl]

@djibarian The fact that you have been using it for years is not an argument. That pattern can cause PHP cross-version compatibility issues as the behaviour/interpretation in PHP itself of redundant parentheses has changed ... See: https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/UPGRADING#L51-L63

[djibarian]

I’m not argumenting that I’ve been using the pattern for years and that’s why it's right. I’m saying that the option to skip the test if the expression is in parentheses is already in ESLint, which is a tool used by way more people than only me, so it has to be a reason for it.

And I wasn’t referring to PHP itself. I have used the same pattern in other languages assuming that it's innocuous, as it should be. Whether it has or have had another meaning in PHP doesn’t matter, if you use it to indicate that you are doing it consciously.

In the other hand, the link you provided states that parentheses in newer PHP versions don’t have any special meaning anymore, so adding the parentheses to current code wouldn’t hurt. It would only hurt if you are sniffing and changing old code, which is not a good idea per se.

But still think that an additional option to take the parentheses into account wouldn’t be a bad idea, simply less verbose than /* phpcs:ignore blah */ if you know what you’re doing. Anyway, thanks for pointing me out to that strange PHP behaviour. I didn’t know about it.

[titpetric]

I resolved my reported issue with @jrfnl's advice by excluding FoundInWhileCondition:

~

error


~

If you want to disable assignment checks, I suggest you follow a similar path. PHPCSFixer (and possibly others) also tend to remove superfluous parentheses in conditional statements.

PHP has a tendency to introduce breaking changes between minor versions, and keeping your code free of such hacks is always preferred. Even the cited example comes from people explicitly trying to hack together a work-around to undesired behavior.

Use a per-line, per-file comment, or ultimately configure (or in your case - disable) the checks explicitly. I was trying to thread a middle-ground path by allowing single assignment checks, but exceptions to rules tend to invite bugs, so disabling assignments in if clauses works for me.

[djibarian]

There are other cases similar to the while exception which are equally legitimate. For example, if(is_null($row = getData())), so for now I’m not going to use the sniff at all based on the amount of changes I would have to do to a codebase which (in my case) rarely created a problem for that reason.

Analysing the before and after in that codebase I can’t see a real improvement in legibility, specially if I have to disable the rule in a per line basis for special cases. I understand your points of view both @jrfnl and @titpetric, but also understand that the current implementation lacks some flexibility present in other tools. Thanks for the discussion though.