Repository navigation
False positive for Squiz.PHP.DisallowComparisonAssignment.AssignedComparison #1481
Description
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:
- Create a file called
test.phpwith the code sample above... - Run phpcs --report=code --standard=SQUIZ test.php`
- 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.xbranch of PHP_CodeSniffer.
Activity
jrfnl commentedon Aug 14, 2026
@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 commentedon Aug 14, 2026
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:
-- 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:
Snake:
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 commentedon Aug 14, 2026
Hi @LLyaudet
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.
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(andSquiz.PHP.DisallowComparisonAssignment.AssignedBool). I can imagine changing the sniff code a little to have the error related to assignments from a ternary be calledAssignedTernaryComparison.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 ?
Update DisallowComparisonAssignmentSniff.php PHPCSStandards#1481
LLyaudet commentedon Aug 14, 2026
Hi @jrfnl ,
Thanks for your proposal.
Why not avoid BC-break by adding:
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 commentedon Aug 14, 2026
@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 commentedon Aug 15, 2026
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 commentedon Aug 15, 2026
@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
LLyaudet commentedon Aug 15, 2026
Ok, no problem.
Would you accept a PR from me for the new modular error code, or do you prefer to handle it yourself?
If you prefer to handle it yourself, may I suggest
Squiz.PHP.DisallowComparisonAssignment.AssignedTernaryOperatorinstead ofSquiz.PHP.DisallowComparisonAssignment.AssignedTernaryComparison?Since the true name is ternary operator and that the word comparison is already in
DisallowComparisonAssignment.Best regards,
Laurent Lyaudet