Php_codesniffer: Generic.ControlStructures.DisallowYodaConditions.Found false positive

[bkdotcom]

Usage of Yoda conditions is not allowed; switch the expression order
source="Generic.ControlStructures.DisallowYodaConditions.Found"

if (is_array($val)
    && array(get_class($val[0]), $val[1]) == array('someNamespace\\className', 'method')
) {

Yoda this is not

It's specifically complaining about line 2...

it still complains after switching the expression order (perhaps because it's now a Yoda condition)

if (is_array($val)
    && array('someNamespace\\className', 'method') == array(get_class($val[0]), $val[1])
) {

ah... the sniff must simply see array(x) == array(x) and assumes the left array is "static"... not looking that it contains expressions / variables ?

@gmponos

[gsherwood]

The original authors of the sniff added these test cases as being Yoda conditions:

if(array($key => $val) === $value){}
if(array($key => $val) == $value){}

if([$key => $val] === $value){}
if([$key => $val] == $value){}

They match your use case, although are a much simpler form.

@gmponos or @dereuromark Do you have any opinion here on the shared code?

[dereuromark]

Looks like false positive due to the complex condition.
Aparr from that i recommend to write simpler checks, by assigning to vars before hand maybe.

[gmponos]

Hello there...

The original authors of the sniff added these test cases as being Yoda conditions:

if(array($key => $val) === $value){}
if(array($key => $val) == $value){}

if([$key => $val] === $value){}
if([$key => $val] == $value){}

@gsherwood well for me the case that the author describes is a totally different one from the tests I wrote... There should have been a test like this:

if(array('a') === array('b')) {}

I didn't add a test like this because I was not expecting the condition I wrote above where one array equals another.. or in other words where two arrays are initialized inside an if condition. What I would expect is something like:

if (is_array($val)
    && get_class($val[0]) === 'someNamespace\\className' && $val[1] === 'method'
) {

So yes.. I agree as well with @dereuromark that the code in my eyes is pretty complex. Nevertheless I understand that each one has each own codestyle... and the case that the @bkdotcom is still a case.. so a test should exist and the code should NOT report this as Yoda and if in the future an EnforceYodaSniff is created then this case should NOT be reported there as well.

I will try to see if I can fix the sniff.. but if someone has more spare time be my guest.

[gsherwood]

well for me the case that the author describes is a totally different one from the tests I wrote...

Then I don't understand why this is a Yoda condition:

if(array($key => $val) === $value){}

But this isn't:

if($foo === $value){}

The tests indicate that the simple act of defining an array with a value in it causes the condition to be a possible Yoda. Yet comparing 2 normal variables doesn't. Are you able to explain this for me please?

[dereuromark]

I dont think those are, at least not if you don't apply too strict definition.
The basic idea of yoda is to invert "intuitive comparison of variable against some non-variable".
And why having it is a bad idea - as human error comes with non-intuitive coding.

So if you have two variables, then it doesnt matter too much.
But in the first case some would argue that you compare the variable against an array (of sth), and as such it becomes a bit more yoda.

Strictly speaking you could also order the variables in terms of non-yoda.
Intuitively the last one being operated on is the variable you want to check, and the other one is the one you want to check against.
But this is often not needed, as here we as humans can still easily track it either way.
The main issue is only with scalar vs variable usually.

[gsherwood]

The main issue is only with scalar vs variable usually.

That's what I figured. But when I implemented a fix, it flagged those test cases I posted as not being Yoda conditions because they only consisted of variables so I wanted to check.

The change I have ready to go would cause the following test cases to change from confirmed Yoda conditions to not being Yoda conditions as they an entirely comprised of variables:

if(array($key => $val) === $value){}
if(array($key => $val) == $value){}

if([$key => $val] === $value){}
if([$key => $val] == $value){}

[gmponos]

But in the first case some would argue that you compare the variable against an array (of sth), and as such it becomes a bit more yoda.

That was my thinking...

That's why comparing an array vs an array didn't make any sense to me.

Edit: IMHO the sniff should not only check scalar.. but ofc this is your decision at the end of the day..

[bkdotcom]

another false positive (not array related):

if ($this->cfg['some_closure']() == 2) {
}

[gsherwood]

I've committed a change for this. If anyone is able to test the code in master, please let me know how it goes.

[gsherwood]

I'm going to go ahead and close this so I can release it. We can continue discussions if other controversial code snippets come up.