Switch to sirbrillig/phpcs-variable-analysis #450
Conversation
GaryJones
force-pushed
the
feature/variable-analysis
branch
from
58e6957 to
21655fe
Compare
There was a problem hiding this comment.
I added the custom fork of the upstream repository to VIPCS due to backward compatibility, as we have been on old version of PHPCS and WPCS for a long time.
That said, if we can switch to the upstream repository now, let's do it!
I haven't really tested this change, but can see that the tests have been adjusted. So as long as those are all still passing, 
One question, tho. WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable becomes VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable. Is there a way to create an "alias", so this won't break the // phpcs:ignore WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable comments?
Not as far as I know. The deprecation handles references in custom rulesets, but not ignore / disable references. |
GaryJones
force-pushed
the
feature/variable-analysis
branch
from
21655fe to
6d33c19
Compare
|
Noting that VA 2.6.3+ requires PHP 5.6, so a composer install may need to ignore platform requirements to be able to test PHP 5.4 and PHP 5.5. |
GaryJones
force-pushed
the
feature/variable-analysis
branch
from
6d33c19 to
f3ed636
Compare
GaryJones
force-pushed
the
feature/variable-analysis
branch
2 times, most recently
from
4097c7a to
d1a4c29
Compare
|
Refactored and fixed up tests and updated to use VA I tested ignoring platform requirements so that VA could be installed on PHP 5.4 and 5.5, but a) it fails due to the PHP 5.6+ code in VA, and b) ignoring platform requirements make no sense when this is a non-dev dependency. I suspect my comment isn't going to result in VA moving back to PHP 5.4 due to the code in it, so we'll have to bump the minimum PHP for VIPCS back up (having only just dropped it) to PHP 5.6. |
|
Before merging, we should check the changes here against the proposals made in #449, or at least open a follow-up PR which the extra changes, within the same release. |
Surprise! 😁 sirbrillig/phpcs-variable-analysis#191 I'll still need to backport it to the 2.x branch though. Coming soon... |
|
@sirbrillig You are a wonderful human - thank you! |
GaryJones
force-pushed
the
feature/variable-analysis
branch
from
d1a4c29 to
298dcc9
Compare
Introduces a new dependency of `sirbrillig/phpcs-variable-analysis` which is a more maintained version of the existing `VariableAnalysis` sniff in VIPCS. Requires ^2.8.3, since 2.6.3 – 2.8.2 had PHP 5.6 as the minimum version, but 2.8.3 changes it down to PHP 5.4 as the minimum. The new sniff is added to the `WordPressVIPMinimum` ruleset. `WordPress-VIP-Go` ruleset inherits this, and maintains its dropping of the severity for `UnusedVariable` and `UndefinedVariable` violations. The ruleset tests have set the previously undefined variables (or changed some variable names), so that violations on each line are still specific to the thing being tested, and doesn't include extra violations due to the use of throwaway variables. One of the reasons for changing was the previously false positive with respect to the use of `$this` inside closures inside class methods. A ruleset test has been added to cover this, and it correctly does not get flagged in the ruleset test. The old `VariableAnalysis` has been marked as deprecated, until it can be removed in the next major release of VIPCS. This will stop any breakage of setups which reference `WordPressVIPMinimum.Variables.VariableAnalysis` in a custom ruleset. If called, it throws a warning, and then passes control to the new VariableAnalysis sniff. The deprecation notice is excluded if the sniff is not explicitly included, and the duplicate violation messages are also silenced. The previously used `VariableAnalaysisHelper.php` has been removed completely, as no-one should have been referencing that directly. Fixes #449.
GaryJones
force-pushed
the
feature/variable-analysis
branch
from
298dcc9 to
a896729
Compare
Unused variable was deprecated on this version Automattic/VIP-Coding-Standards#450 Note: users that have this pattern enabled will see a deprecation notice
Introduces a new dependency of
sirbrillig/phpcs-variable-analysiswhich is a more maintained version of the existingVariableAnalysissniff in VIPCS.The new sniff is added to the
WordPressVIPMinimumruleset.WordPress-VIP-Goruleset inherits this, and maintains its dropping of the severity forUnusedVariableandUndefinedVariableviolations.The ruleset tests have set the previously undefined variables (or changed some variable names), so that violations on each line are still specific to the thing being tested, and doesn't include extra violations due to the use of throwaway variables.
One of the reasons for changing was the previously false positive with respect to the use of
$thisinside closures inside class methods. A ruleset test has been added to cover this, and it correctly does not get flagged in the ruleset test.The old
VariableAnalysishas been marked as deprecated, until it can be removed in the next major release of VIPCS. This will stop any breakage of setups which referenceWordPressVIPMinimum.Variables.VariableAnalysisin a custom ruleset. If called, it throws a warning, and then passes control to the new VariableAnalysis sniff. The deprecation notice is excluded if the sniff is not explicitly included, and the duplicate violation messages are also silenced.The previously used
VariableAnalaysisHelper.phphas been removed completely, as no-one should have been referencing that directly.Fixes #449.