Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The current \DrupalPractice\Sniffs\CodeAnalysis\VariableAnalysisSniff sniff has many problems.
For example:
#2936066: Warning "Unused bound variable" appears even if the bound variable is used.
#3065679: False positive for unused variable when variable is passed by reference in closure
#2876245: Incorrect "variable is undefined" when using array destructuring, for example list( ) syntax
And there are other things it recommends against which are not best practice. For example:
foreach($entities as $id => $entity) {
$entity->doSomething();
}
It'll complain about $id not being used BUT it is often helpful for others and future-selves to document what an array is keyed by this way.
Proposed resolution
As suggested in #3106216: Remove unused variables from core, Coder should use the PHP_CodeSniffer VariableAnalysis library to detect any unused variables, and replace its own rule with the library one.
Github PR: https://github.com/pfrenssen/coder/pull/111
Comments
Comment #2
apadernoComment #3
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedwe need the change for 8.x version of the coder module. @kiamlaluno
Comment #4
apadernoComment #5
klausiThere is also https://github.com/slevomat/coding-standard with lots of useful sniffs for modern PHP code bases as discussed in #2876245: Incorrect "variable is undefined" when using array destructuring, for example list( ) syntax, but it requires PHP 7.2.
Not sure how Coder could use this package since it is a standalone PHPCS standard. Drupal core could just install it and reference it from its phpcs.xml.dis file?
Comment #6
apadernoPlease ignore this comment.
Comment #7
alexpottMerging with the duplicate I've opened and now will close.
Comment #8
Arkener CreditAttribution: Arkener as a volunteer commentedI'm unsure if Coder should include sirbrillig/phpcs-variable-analysis as a dependency or if Drupal core should include it as a dev-dependency and add the rule to its own ruleset definition. Including the external plugins as a ruleset within Coder would allow us to add tests to check for compatibility with Drupal coding standards, which is a big advantage. It would also allow us to rely on other plugins for other sniffs. It would also make it easier for contrib modules to adopt the proper coding standards without including a phpcs.xml file.
Regarding the choice of https://github.com/slevomat/coding-standard or https://github.com/sirbrillig/phpcs-variable-analysis, as Klausi already stated, Slevomat currently has a dependency on PHP7.1+ which means Coder can't include it without also increasing its PHP dependency. It does however provide more sniffs which might be useful in the future when adding new sniffs or maybe even to replace / extend existing sniffs. It however does not include a sniff to check for the usage of undefined variables, which Sirbrillig does. The Sirbrillig "undefined variable" currently does have an issue regarding direct variable-as-array assignments, which in my opinion should be addressed before we consider enabling the "undefined" portion of this sniff.
Comment #9
alexpott@Arkener check out the PR - https://github.com/pfrenssen/coder/pull/111 it already silences the undefined sniff. I really don't think core should include this. Coder is currently supplying a rule. But core cannot use it because it is broken. If unused variables is not the purview of coder then fine core can include the rule. But that makes no sense whilst you are providing a rule. Using sirbrillig/phpcs-variable-analysis means coder can drop a load fo custom code and benefit from upstream fixes and improvements. And sure at some point we might change to another library to do the same task but that's fine too.
Comment #10
Arkener CreditAttribution: Arkener as a volunteer commented@alexpott apparently I overlooked the part of the PR where you excluded the undefined part of the sniff. I agree that the current sniff doesn't properly work and will most likely just result in more bugs in the future. Replacing this with an upstream available sniff would be the best idea here. The current solution does however provide checks for undefined variable, which would be lost when implementing the library with the undefined rule disabled.
I also agree that we should just include this new sniff in Coder and replace the old one. This would unsure that core, but also contrib modules can easily run the sniffs available within DrupalPractice to check the status of their code, without creating custom phpcs.xml files to include extra sniffs from external libraries.
Comment #11
klausiI'm fine with the approach and I think that is clever to integrate an external sniff as our own. Thumbs up from me!
Comment #13
Arkener CreditAttribution: Arkener as a volunteer commentedMerged!, thanks for your work on this @alexpott
Comment #15
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAmended issue title to show what was actually done.
Comment #16
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedFor info, in SirBrillig issue 98 https://github.com/sirbrillig/phpcs-variable-analysis/issues/98 a good solution is found and this has been implemented in pull 205 https://github.com/sirbrillig/phpcs-variable-analysis/pull/205 on 10 Sep 2020
The fix is marked with 3.0 milestone (currently on 2.9.2, but 3.0.0-beta7 is available as at 10 Sept)