Replace VariableAnalysis Sniff with VariableAnalysis package

[GaryJones]

Follow-on to #236.

We currently have a VariableAnalysis sniff, but it has some issues. It currently provides the following violations:

• WordPressVIPMinimum.Variables.VariableAnalysis.VariableRedeclaration: Warning, severity 5, "Redeclaration of %s %s as %s."
• WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable: Warning, severity 5, "Variable %s is undefined."
• WordPressVIPMinimum.Variables.VariableAnalysis.UnusedVariable: Warning, severity 5, "Unused %s %s."
• WordPressVIPMinimum.Variables.VariableAnalysis.SelfInsideClosure: Error, severity 5, "Use of {$err_desc}%s inside closure."
• WordPressVIPMinimum.Variables.VariableAnalysis.SelfOutsideClass: Error, severity 5, "Use of {$err_desc}%s outside class definition."
• WordPressVIPMinimum.Variables.VariableAnalysis.StaticInsideClosure: Error, severity 5, "Use of {$err_desc}%s inside closure."
• WordPressVIPMinimum.Variables.VariableAnalysis.StaticOutsideClass: Error, severity 5, "Use of {$err_desc}%s outside class definition."

In #236, it was decided to replace this with https://github.com/sirbrillig/phpcs-variable-analysis (cc @sirbrillig) - this provides the following violations:

• VariableAnalysis.CodeAnalysis.VariableAnalysis.VariableRedeclaration: Warning, severity 5, "Redeclaration of %s %s as %s."
• VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable: Warning, severity 5, "Variable %s is undefined."
• VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable: Warning, severity 5, "Unused %s %s."
• VariableAnalysis.CodeAnalysis.VariableAnalysis.SelfOutsideClass: Error, severity 5, "Use of self::%s outside class definition."
• VariableAnalysis.CodeAnalysis.VariableAnalysis.StaticOutsideClass: Error, severity 5, "Use of static::%s outside class definition."

There is no SelfInsideClosure or StaticInsideClosure violations, as these appear to be allowed now in modern versions of PHP. The rest of the detection process is more maintained, and handles PHP 7 code better. Switching to it would remove some common false positives.

We could also take this opportunity is taken to silence the UnusedVariable violation. It's severity is already dropped down to 1 for WordPress-VIP-Go, which is the same as what the bot runs at (though this may change in the future). From a VIP perspective, unused variables are not important.

Undefined variables, however, will be flagged in PHP 8, so we should maintain this violation (WordPress-VIP-Go sets it as severity 3), even if the message is changed to highlight this incompatibility with PHP 8.

False Positives

The following snippet is often highlighted by the existing sniff as $this being undefined when in a closure in a class. That should no longer be flagged by the new package.

class MyClass {
	function my_function() {
		return function() {
			$this->my_callback();
		};
	}

	function my_callback() {}
}

The old sniff should be kept and marked as deprecated, to avoid breaking compatibility with custom rulesets, until the next major release of VIPCS when it can be removed.

[simonwheatley]

Noted in JIRA as [GH-3323]

[sirbrillig]

Let me know if there's anything I can do to help!

FWIW, VariableAnalysis.CodeAnalysis.VariableAnalysis.VariableRedeclaration was part of the forked package, but I don't know if it actually works; I have no tests for it and I've never seen it show up in practice. I've informally considered it deprecated and I don't list it in the documentation.

[GaryJones]

I think variable redeclaration might come up for something like list( $foo, $foo ) = [ 'a', 'b' ];.

[mikeyarce]

Came across another false positive:

$test[] = [
  'post_id' => 1,
  'isbn'    => 1,
  'status'  => 1,
  'success' => 1 ? 'true' : 'false',
];

Warning: Variable $test is undefined (WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable).

[sirbrillig]

I know you're not using my sniff yet, but FWIW that's a tricky one, since technically $test is undefined in that case. Even though PHP allows you to create variables when they're being used, but it's hard to know in a sniff if those shortcuts are intentional or accidental. See the conversation here: sirbrillig/phpcs-variable-analysis#98 (comment)