Add new sniff to ban the use of compact() function

[pbogut]

Adds new generic sniffer for banning usage of compact() function.

Using this function often is not playing nice with static analysers and LSPs and makes refactoring harder.

I've added sniffer to detect if compact() is used to create array. If function call contains only strings that could be valid variable names then it can be replace automatically by array use compact('a') will be fixed as ['a' => $a].

Not sure if this should be added to package.xml? And I'm also not sure if Arrays is good namespace for it. It is function after all, but it is just used to build array. I'm happy to change whatever is needed.

[jrfnl]

I've done a cursory review of the sniff without an opinion on whether it should be included in PHPCS.

I would note that I'm not sure it's a good idea to include a fixer in this sniff unless you are certain that every possible code situation has been covered. As things are, I would consider this a pretty risky fixer.

Not sure if this should be added to package.xml?

Yes, all files need to be listed in package.xml.

And I'm also not sure if Arrays is good namespace for it.

See my suggestion inline.

[jrfnl]

I don't think Arrays is the correct category as that only contains sniffs which are looking for array syntax.

I'd suggest using the PHP category.

[jrfnl]

Looks like this needs fixing to allow for modern PHP:

$obj?->compact('a');

[pbogut]

Yes, thanks. I've added that, plus few other things.
I'm now questioning if I should look back for things that indicates it's not actually builtin function call, or maybe should change approach and list things that indicates it is function call.

So what I mean by that is if its better to check (as it is now) :

 if ($tokens[$prev]['code'] === T_OBJECT_OPERATOR ... ) {
     return;
 }

Or maybe would be better something like:

if ($tokens[$prev]['code'] !== T_EQUAL ...) {
    return;
}

It might be more future proof if changed, but not sure if I wont end up with long list of allowed tokens.

[jrfnl]

In my experience the current approach of bowing out of specific tokens is the correct one.
Changing the logic around would make the sniff much less stable as new tokens get added regularly (either via PHP itself or in PHPCS to account for changes in PHP) and this sniff would need to be updated each time that happens.

[jrfnl]

What about namespaced functions ?

MyNamespace\compact('a');
namespace\compact('a');

Also what about these ?

class Foo {
    public function compact( $param = 'a' ) {}
    public function &compact( $param = 'a' ) {}
}

$obj = new Compact('a');

[pbogut]

I've added them to the test case, they should not produce sniff errors.
I've also added \compact() as this is valid function call and should produce error.

[jrfnl]

Maybe declare this regex as a class constant ?

[pbogut]

I did that, thanks for suggestion.

[jrfnl]

I see quite some code in this test case file which doesn't seem to be testing anything in the sniff... ?

Here are some more test cases to take into account:

compact(...$names);
compact( 'prefix' . $name, '$name' . 'suffix', "some$name");
compact(...get_names('category1', 'category2'));

// Last test in the file.
// Live coding/parse error.
compact( 'a', 'b'

See this as an invitation to get a little more creative with the tests and to make sure the sniff handles those situations correctly.

[pbogut]

Most of these were already covered as sniff requires only strings and commas within parenthesis. I've added them to test case. Last one was actually causing error so I fixed that as well.

[jrfnl]

I'm not sure why this test is included ?

[pbogut]

Fair enough, will remove. (I've copied some testes from different sniff as I started to work on it).

[jrfnl]

I suggest adding tests with the below (function names in PHP are case-INsensitive):

COMPACT('a');
Compact('a');

[pbogut]

Added and fixed.

[pbogut]

Thank you for review, will add more tests for cases you listed and check code suggestions. As for the fixer, it is checking if there is:

1. Open parenthesis after function name
2. Close parenthesis after open one
3. Anything in between is ONLY string (that should be valid PHP variable name), empty token (white spaces, comments) or comma ,.

If any of the above is not true then error would not be fixable.

[jrfnl]

As for the fixer, it is checking if there is:

1. Open parenthesis after function name
2. Close parenthesis after open one
3. Anything in between is ONLY string (that should be [valid PHP variable name](https://www.php.net/manual/en/language.variables.basics.php)), empty token (white spaces, comments) or comma `,`.

If any of the above is not true then error would not be fixable.

So that means that only plain strings would be taken into account, while per the documentation of compact(), the function allows for receiving arrays as well ?

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.

Source: https://www.php.net/manual/en/function.compact.php#refsect1-function.compact-parameters

[pbogut]

So that means that only plain strings would be taken into account, while per the documentation of compact(), the function allows for receiving arrays as well ?

For fixing it yes. It will still be reported if its compact containing an array, but it would not be fixed automatically.

My reasoning was that simpler fixer the better, and most cases in the wild I've seen were just using simple string arguments. Otherwise would have to check for arrays (short and long ones) and array values (that may have keys specified). And also nested arrays. For example this: compact(['c' => array(['b' => 'a'])]) is valid compact call and would produce ['a' => $a] array.

Do you think its wrong way of looking at it?

[jrfnl]

Do you think its wrong way of looking at it?

No, I concur that is probably the sensible way of doing it to prevent the fixer from getting overly complicated/making incorrect fixes.