False positive for Squiz.PHP.DisallowComparisonAssignment.AssignedComparison

[LLyaudet]

Describe the bug

Squiz.PHP.DisallowComparisonAssignment.AssignedComparison
yields a false positive on the following code:

$this->s_dirname = (
  isset($path_parts['dirname']) ? $path_parts['dirname'] : ''
);

-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------
LINE 183: ERROR The value of a comparison must not be assigned to a variable (Squiz.PHP.DisallowComparisonAssignment.AssignedComparison)
-----------------------------------------------------------------------------------------------------------------------------------------
   181:  ······$this->s_filepath·=·$s_filepath;
   182:  ······$path_parts·=·pathinfo($this->s_filepath);
>> 183:  ······$this->s_dirname·=·(
   184:  ········isset($path_parts['dirname'])·?·$path_parts['dirname']·:·''
   185:  ······);
-----------------------------------------------------------------------------------------------------------------------------------------

Code sample

<?php

$arr = [];
$s = isset($arr['a']) ? $arr['a'] : 'b';

Custom ruleset

Not needed

To reproduce

Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs --report=code --standard=SQUIZ test.php`
3. See error message displayed

FILE: /home/laurent/test.php
------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------
LINE 1: ERROR [ ] Missing file doc comment
------------------------------------------------------------------------------------------------------------
>> 1:  <?php
   2:  
   3:  $arr·=·[];
------------------------------------------------------------------------------------------------------------
LINE 4: ERROR [x] Equals sign not aligned with surrounding assignments; expected 3 spaces but found 1 space
LINE 4: ERROR [ ] The value of a comparison must not be assigned to a variable
LINE 4: ERROR [ ] Inline IF statements are not allowed
LINE 4: ERROR [ ] Implicit true comparisons prohibited; use === TRUE instead
------------------------------------------------------------------------------------------------------------
   2:  
   3:  $arr·=·[];
>> 4:  $s·=·isset($arr['a'])·?·$arr['a']·:·'b';
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------
Time: 37ms; Memory: 8MB

Expected behavior

No error "The value of a comparison must not be assigned to a variable".

Versions (please complete the following information)

Operating System	Ubuntu 26.04
PHP version	8.5.4
PHP_CodeSniffer version	4.0.4
Standard	Squiz
Install type	Composer global

Additional context

Please confirm

• I have searched the issue list and am not opening a duplicate issue.
• I have read the Contribution Guidelines and this is not a support question.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the 4.x branch of PHP_CodeSniffer.

[jrfnl]

@LLyaudet I'm not sure I understand what you are reporting as the behaviour you describe is by design. The sniff was updated in 2015 to explicitly also flag ternary comparisons in response to squizlabs/PHP_CodeSniffer#616.

[LLyaudet]

Hello @jrfnl

I finally understood (again?) the way PHPCS and its developpers want us to use PHPCS.
Choose a set of standards, use them with minimum configuration to claim compliance.
Hence, Squiz linked two unrelated things without option to unlink them,
because they define their standard.

Now here is my way to use PHPCS or other similar tools:

• enable all checks and sniffs, etc.,
• correct all errors,
• if you cannot correct all errors in your code, without betraying your own conventions,
  -- try to customize the problematic sniffs
  -- if customization is not available
  --- disable the sniff

Please, don't take what follows personaly.

Hence, because of my way of using it,
that tries to extract the biggest possible part of relevant checks from one given linter;
the fact that all things are linked with a lot of links that are feature bug from my point of view,
implies that work must be redone to benefit of some subpart of some sniff.
It leads to duplicate code between standards or between standards and other code bases.
It leads to bad naming of sniff, because a "standard" cannot be questionned,
and only atomic and reusable sniffs can have simple names.

Now for this particular case, below is the start of my thoughts about this subject,
before I realized it is hopeless.
I think code quality, avoiding bugs.
I don't think "standards".

The ternary operator doesn't assign the result of a comparison.
The result of a comparison operator is a boolean.
The meaning I understand for this sniff is to forbid to assign the result of "$a == $b", "$a === $b", "$a < $b", "$a <= $b", "$a > $b", "$a >= $b".
But what I assign in my code is not a boolean.
It is just the result of the switch of the ternary operator that happens to be a string.
Ternary operator has always been called ternary operator and not ternary comparison.
It does contain an implicit comparison of first operand to value true. But that's it.
Bad naming.
Moreover, assigning the result of the ternary operator like I do in my example code, is 90 % of uses of ternary operator.
It makes no sense to sabotage this sniff with that check on top, unless the intent is to have all programmers using the ternary operator to disable this sniff.
I think it is quite a snake move to do it that way.
If you give freedom to people, you don't link things like that.

Freedom:

• sniff to forbid ternary operator
• sniff to forbid assigning the result of a comparison

Snake:

• sniff to forbid assigning the result of a comparison, that also includes the ternary operator, and no option of the sniff can disable that.

Sorry for the rant, but atomic design is the way to go to be productive,
you can redefine standards as sets of configurations on top of atomic sniffs,
you can do anything,
you can avoid angry issues that would have never happened ;),
etc.

Have a nice day, best regards,
Laurent Lyaudet

[jrfnl]

Hi @LLyaudet

I finally understood (again?) the way PHPCS and its developpers want us to use PHPCS.

That's not necessarily true (anymore).

I do try to stay true to what the original developer had in mind for this package - and try to keep BC-breaks for end-users caused by changes to sniffs to a minimum. Having said that, I don't like most of the "do everything for some construct" sniffs as found in this repo either. Unfortunately, changing those means BC-breaks, so is not an easy road. So instead, most new sniffs I create (in PHPCSExtra) are far more modular and do only one specific thing and when they do that one thing for, for instance, multiple constructs, they allow you to customize based on the error codes.

At some point, years away from now, we may reach a point where most of the sniffs in this repo are deprecated/removed in favour of far more modular building blocks for standards via external standards.

Sorry for the rant, but atomic design is the way to go to be productive,

I 100% agree, but I can't change the choices made in the past (without causing BC-breaks). I can only try to improve things going forward.

So to come back to your gripe with the assignments of ternary comparisons, the only thing I can currently think of as some sort of solution, would be to differentiate in the error code of the sniff.
I.e. the current error code is Squiz.PHP.DisallowComparisonAssignment.AssignedComparison (and Squiz.PHP.DisallowComparisonAssignment.AssignedBool). I can imagine changing the sniff code a little to have the error related to assignments from a ternary be called AssignedTernaryComparison.
That way, you could exclude that error code without having to remove the sniff.

It would be a BC-break, but a small one and one which - in my opinion - would be acceptable to be included in the next minor.

What do you think ?

[LLyaudet]

Hi @jrfnl ,

Thanks for your proposal.
Why not avoid BC-break by adding:

<rule ref="Squiz.PHP.DisallowComparisonAssignment">
    <properties>
        <property name="allowTernaryOperatorResultAssignment" value="true" />
    </properties>
</rule>

like what is here?:
https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties
If you can code a custom error message like your proposal,
it means you can detect easily this case in the code base,
and then you could directly suppress it using the property above
only in the relevant case,
instead of suppressing all of Squiz.PHP.DisallowComparisonAssignment.AssignedComparison in all generating cases.
Clearly, if there is no problem I don't see,
it seems to be the way to go.
I created a PR with this approach,
#1482
it seems it should work without trouble.
The PR is not already mergeable.
But I think I can finish it quite quickly.
You can also modify it.
It is late right now in France.
I'll be back on it tomorrow.

Have a nice day, best regards,
Laurent Lyaudet

[jrfnl]

@LLyaudet Modular error codes are always preferred over properties, so I will not accept a PR adding a property when an error code could solve the same problem.

Also, as a point of order - please don't open a PR until it is ready for review.

[LLyaudet]

Hello @jrfnl ,

Ok sorry for the trouble.
In my point of view avoiding a breaking change was preferred to a modular error code, when the same major version has to be kept.
I use semantic versioning for my projects.
And thus, the feature you suggest would need to bump the version of PHPCS to 5.x.x.
Personally, I also try to do the long term best choice,
and code in a way that will converge to the best source code in the fastest possible way.
Because I don't really care about keeping the major version low,
since I have few users of my libraries.
In your case, if you upgrade to 5.x.x, you'll have to provide an update guide for your users,
and handle a lot of "formalities".
I just wanted to avoid that to you.
If you do as you intended, please don't merge it with the 4.x.x,
because users of PHPCS will not trust you anymore if you introduce a breaking change in the same major version.
Semantic versioning is not always used,
but users of packages do assume it most of the time.
Let me know if you want me to code another PR for the feature with a modular error code, or if you prefer to do that yourself.
I'll avoid to open another PR that is not ready for review.

Have a nice day, best regards,
Laurent Lyaudet

[jrfnl]

@LLyaudet I appreciate your opinion about versioning, but the versioning scheme used for PHPCS is documented and allows for what I proposed: https://github.com/PHPCSStandards/PHP_CodeSniffer#versioning