compact($var) as code smell or probable bug?

[rk]

I've had this happen semi-frequently when writing code. I'll start with...

my_func('foo', 'bar', [
  'something' => $something,
]);

...and realize it would be better served as a compact() call. So I rewrite it and then get slammed with bugs because...

$something = [
    ['my' => 'struct'],
    ['my' => 'other'],
];

my_func('foo', 'bar', compact($something));

🤦‍♂️

This is caused by there being 2 valid usages of compact():

compact() takes a variable number of parameters. Each parameter can be either a string containing the name of the variable, or an array of variable names. The array can contain other arrays of variable names inside it; compact() handles it recursively.

// I usually only see this usage
compact('var1', 'var2', ...)

and

// But compact will try (recursively) to find any variables recursively
compact('var', ['var1', 'var2', $other, ...], ...);
compact(['foo', 'bar', 'baz']);

So the code I passed is valid PHP—except for the fact its an array of arrays. Could we have an inspection that ensures that compact calls are either string ...$vars or string[] $vars? It would be nice to catch those occasional places where I forget to pass a string instead of the var, before I run my tests.

I think the usage could fall under the Probable bug category if you pass an invalid value or key (I tried to create examples below), or even Code Smells if you pass an array as the only argument (but I'm opinionated on that one). I didn't realize the 2nd usage (with an array) was valid till today.

---

Examples:

Good 👍

compact('varname', 'my_thing');

Good 👍

compact(['varname', 'my_thing']);

Good 👍

$args = ['varname', 'my_thing']
compact($args);

Good 👍

$this_totally_exists = true;
$my_thing = 'this_totally_exists';
compact('varname', $my_thing); // $my_thing treated as $$my_thing here

Good 👍

$this_totally_exists = true;
$my_thing = 'this_totally_exists';
$args = ['varname', $my_thing]
compact($args); // $my_thing treated as $$my_thing here

Bad 👎

compact($imaginary); // Variable isn't defined 🤦‍♂️ 

Bad 👎

$array = [[],[],[]];
compact($array); // array of arrays 🤦‍♂️ 

Bad 👎

$array = ['foo','bar',[]];
compact($array); // array of keys with a bad/invalid key 🤦‍♂️ 

There might be more bad usage examples, but I haven't thought of any more than these. That whole recursive check on arrays is weird and dangerous, IMO.

[kalessil]

@rk : which plugin do you use (ultimate or extended)?

[rk]

I'm currently using Extended.

Is this already an inspection in Ultimate?

[kalessil]

Nope, i need to make decision where this should go to. Let's put it into Extended, Ultimate has enough features pending implementation.

[rk]

Thanks for your work on this project, Vlad. I recently became a patron via Patreon, but haven't gotten up to the level to get Ultimate yet. Even so, PHP Inspections Extended has saved me from many errors and oversights, and has made my life as a programmer easier!

[kalessil]

Oh, huge thanks for the support! No worries, just poke me if you decide to upgrade =) Happy to help, and thank you for reporting back issues: much appreciated!