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

PHP 7.0+ - Only variables should be passed by reference #2550

Closed msiwy closed 5 years ago

msiwy commented 5 years ago

The following snippet generates a notice in PHP 7.0+.

$site = end( explode( '/', $path ) ); # PHP Notice: Only variables should be passed by reference

This is a new notice as of PHP 7.0. I tested against all included standards and was unable to have this detected. I think it would be useful to include a Sniff to detect this, thank you.

jrfnl commented 5 years ago

@msiwy There is the Generic.Functions.CallTimePassByReference sniff, but it would still not detect this.

This particular issue is not something PHPCS can easily detect as to be able to do so, it would need to know of all functions used whether they return by reference or not.

This may be possible to figure out for PHP native methods, but for userland functions and methods which would not be loaded during the PHPCS run, it would be neigh impossible.

This may be something to report to the PHPCompatibility standard (external PHPCS standard) to see if, at least for the PHP native functions, it can be covered in one of the sniffs there.

Regarding the relevant PHP changes:

Your code sample is not affected by the change in parentheses handling which you originally linked to.

msiwy commented 5 years ago

@jrfnl

Gotcha, thank you for the clarification on the origin of the issue (and quick response).

Do you think PHP native method detection would be a useful enhancement for PHPCS? If not I will request the enhancement via PHPCompatability and close this issue. I may end up adapting PHPCompatabiliy's LanguageConstructs/NewEmptyNonVariableSniff.php to handle more cases (including userland functions).

Edit: And upon closer inspection, I notice you are the author of NewEmptyNonVariableSniff.php haha

jrfnl commented 5 years ago

Do you think PHP native method detection would be a useful enhancement for PHPCS?

I'm not sure, it may be going a bit far for something which is a bit of an odd-duck in the PHPCS native sniffs anyway, but ultimately it's up to @gsherwood to decide whether that is desirable.

Pull requests to add enhancement for this to PHPCompatibility would be welcome, though I think the sniff in PHPCompatibility which this should go into, would be the Syntax/CallTimePassByReference sniff, not the NewEmptyNonVariable sniff.

gsherwood commented 5 years ago

Do you think PHP native method detection would be a useful enhancement for PHPCS?

PHPCS is focused around coding standards rather than detecting coding errors, so this isn't something I'd put on the roadmap.

I'm going to close this issue so that is clear, but I would still consider including a sniff to check for coding errors like this if someone wrote a well-tested one.

aik099 commented 3 years ago

@jrfnl , I've checked Generic.Functions.CallTimePassByReference and it is supposed to catch method(&$variable) calls instead of usage of non-variables as pass-by-reference arguments to built-in PHP functions.

@gsherwood , any idea how to detect a function call and easily get all passed arguments? What token, should I listen for or maybe there are similar sniffs (that operate on function calls and their arguments)? Not sure if the sniff would massively slow down any coding standard it's used by though.

gsherwood commented 3 years ago

@aik099 There is a sniff that checks function calls, so there is a bit of code at the top of the process() method that tries to detect that it is actually a call.

But there is no easy way to gather the function all arguments.

jrfnl commented 3 years ago

Well, there is, just not in PHPCS itself: https://phpcsutils.com/phpdoc/classes/PHPCSUtils-Utils-PassedParameters.html

aik099 commented 3 years ago

... There is a sniff that checks function calls ...

@gsherwood , what is the name of that sniff?

gsherwood commented 3 years ago

@gsherwood , what is the name of that sniff?

Sorry, I had it open and in a tab and forgot to link :(

https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/PEAR/Sniffs/Functions/FunctionCallSignatureSniff.php