False positive Generic.ControlStructures.DisallowYodaConditions

[jrfnl]

Repost from squizlabs/PHP_CodeSniffer#2962:

The Generic.ControlStructures.DisallowYodaConditions reports a Usage of Yoda conditions is not allowed; switch the expression order error for the below code, which IMO is incorrect (the error, not the code).

$var = ( (string) function_call( 'text' ) ) === '0';

---

@umherirrender provided a second code sample:

Another one:

					return ( $group && $pos ) ?
						( $pos % $group ) === 0 && ( $pos < 0 === $group < 0 ) :
						!$pos;

It seems that a boolean condition with two < 0 is not handled correctly by the sniff.

Work around: Add () around each part of the condition

---

@jlherren provided a third:

I'm also experiencing this, here's a very short example:

$a = 1 * (1 + 1) !== 0;

---

[rodrigoprimo]

@jrfnl, I'm working on a PR to improve code coverage for Generic.ControlStructures.DisallowYodaConditions, and I found another issue where two other false positives were reported: squizlabs/PHP_CodeSniffer#2897. Should those examples be moved here and the original issue closed?

There is also squizlabs/PHP_CodeSniffer#2830, which reports two false negatives. Do you prefer that they are moved here or do you prefer that a separate issue is created in this repository for false negatives?

[jrfnl]

@rodrigoprimo When I forked the repo, I only moved my own issues/PRs over. Other people are welcome to move their issues/PRs over themselves (and then close the original). But it's also perfectly fine for the original issue to remain as-is and open. It can still be closed at a later point in time if/when the issue would be resolved via a PR in this repo.

I agree that for the time being and the foreseeable future, it is a good idea to also search the Squizlabs repo for related issue if/when reviewing a sniff.
Linking the issues, like you just did in your comment, should be sufficient. I don't think there is a need to copy the code samples. After all, the original issue may contain more info/discussion than just the code samples.

As you are working on code coverage, I don't expect your PR to address these tickets. If during your work on the code coverage you would come across more false positive/negative situations, feel free to add to this ticket if it's about the same type of false positive or to open a new ticket if the situation is different enough.
If the new ticket would overlap with one of the aforementioned tickets in the Squizlabs repo, be sure to link that ticket to yours.

Does that help ?

[rodrigoprimo]

That helps! I was under the wrong impression that the plan was to slowly move all the issues that are still relevant from the old repository to this one. Thanks for clarifying that this is not the case.

I don't expect your PR to address these tickets

Yes, I'm not planning to. Unfortunately, the fix for these issues doesn't seem trivial.

If during your work on the code coverage you would come across more false positive/negative situations, feel free to add to this ticket if it's about the same type of false positive or to open a new ticket if the situation is different enough.

I already have some more false positive/negative situations that I will share in a subsequent comment once I finish the test coverage PR as I might find a few more.

[rodrigoprimo]

Here are two false positives that I found while working on a PR to improve code coverage for this sniff.

String concatenation does not trigger an error, so I believe adding parentheses to the string closer to the operator should not trigger it as well:

'string' . ('concat') === $value;

I'm a bit unsure about the one below. I believe it should not trigger an error as it is a syntax error (missing closing bracket). [1,2] === value does not trigger an error but I think it should (reported in another issue for false negatives: squizlabs/PHP_CodeSniffer#2830):

[1, 2 === $value;