phpinspectionsea icon indicating copy to clipboard operation
phpinspectionsea copied to clipboard

[FR] Array and string offset validity: new settings

Open orklah opened this issue 5 years ago • 3 comments

Description:

When we activate the "Array and string offset validity" inspection in our project, we get thousands of errors. The main reason is because we have a lot of methods who return mixed results like "array|false" or like "RandomObject[]|int[]" and a parameter to choose whether we want ids or RandomObjects in return.

This is bad code, but it's generally working well.

However, the inspection doesn't do the difference between "array|false" and "bool" for the following code: $foo['bar']

One is a bad practice, or at worst a missing type check, the second is clearly an error.

There is also no special case for "mixed" like we have on the "Foreach source to iterate over" inspection.

I would like to introduce two new settings in this inspection:

  • Trigger when none of the type can be offsetted (as opposed to the current behaviour: Trigger when at least one type can't be offsetted )
  • Trigger when one of the types is "mixed"

This would allow to activate this inspection for us more gradually and to catch real issues earlier, and then work on the type checks

Thanks

orklah avatar Feb 25 '19 20:02 orklah

I'm not sure ours is exactly the same, but close. As the submitter indicated, though, we have functions that can return something other than just an array, but to keep it simple, let's say

@return array|false (array if there is a value, false if there is no value)

So we do this:

if($returnValue === false){

$returnValue = [];

$returnValue[1] = 'test'; <--- Inspection warning here for validity

}

Clearly, we are setting the variable to an array, so regardless of the annotations, the variable cannot be anything but an array. Why do annotations override explicit code?

So you may say, why not use a different variable? Because these are cache patterns, we have hundreds of them, and we think this way is clean and easy to read:

image

laurinkeithdavis avatar Jul 20 '19 13:07 laurinkeithdavis

Also, probably should be a separate issue, but this inspection is not obeying the severity level, and also is not obeying suppression when the variable is a static property:

image image

laurinkeithdavis avatar Jul 20 '19 13:07 laurinkeithdavis

What in the world? Of the issue that it was not obeying suppression, I have to suppress 2 different ways?

image

laurinkeithdavis avatar Jul 20 '19 13:07 laurinkeithdavis