squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

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

Open pbogut opened 1 year ago

pbogut commented 1 year ago

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.

pbogut commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.