Replace, improve, or drop VariableAnalysis

[GaryJones]

What problem would the enhancement address for VIP?

The VariableAnalysis sniff isn't up to the same quality as some of the other sniffs.

The unit tests for this sniff are severely lacking - there's basically one test.

The code isn't written in the same way as the rest of the sniffs; there is a helper class, which suggests that the one sniff class is trying to do too much. It is based on https://github.com/illusori/PHP_Codesniffer-VariableAnalysis (last updated 6 years ago), and then updated a few times since by @david-binda .

Describe the solution you'd like

The choices:

Replace

I'm wondering whether making use of https://github.com/sirbrillig/phpcs-variable-analysis by A11n @sirbrillig would be a better route in the long term. It too is forked from the same original code (and is therefore also not written in a typical way), but is active (last updated 6 days ago at the time writing), and has considerably more tests. We could require it as a require dependency, and then reference it in the VIPCS ruleset(s).

I had a quick look at sniffs in the standards built-in to PHPCS, but didn't see anything that would come close.

Improve

It basically needs a rewrite, and many more unit tests. It probably needs splitting into separate sniffs that target different areas of "variable analysis".

The current checks are:

• Variable Redeclaration
• Variable Undefined (twice)
• Self/Static Outside Class (twice)
• Unused Variable (twice)

Drop

If the rest of the checks it does are not that useful, or not useful for following critical rules for WPCOM and VIP-Go, then it could be considered to be dropped.

• Unused variables can be detected by tools like PHP Mess Detector, PHPStan and other static analysis tools.
• Self/Static outside a class seems like something PHP lint would possibly pick up. - Variable undefined seems like unit tests should pick this up.
• Variable redeclaration doesn't seem like a big issue.

All of them are things that an inspection in an IDE would flag (static analysis), and none of them seem to be VIP-specific, and so any consideration should be that any sniffs go elsewhere - upstream, or be left to @sirbrillig's package etc.

Leave As Is

The final option is to do nothing. However, there are already bug reports about it (#223), and I wonder if it's a false sense of security, if the code can't be trusted (and with virtually no tests, I don't trust it).

cc @sboisvert @tomjn @gudmdharalds

[rogertheriault]

+1 for replace - this actually happened, PHPCS flagged it using this rule during final review, and we avoided a likely outage...

$str_replace( $foo, $bar, $bah );

It does have issues and lacks tests but the sniff itself is valuable and we can't always rely on folks to use alternate tools

[sboisvert]

@rogertheriault what exactly did it flag? Perhaps I'm missing something obvious in

$str_replace( $foo, $bar, $bah );

Personally I don't see it as a high importance sniff. While it might be good upstream as a general sniff these are all things I'd mark as <= 4 warning as per https://vip.wordpress.com/documentation/phpcs-review-feedback/

While it's nice to have, I'd vote to remove it.

[sirbrillig]

$str_replace( $foo, $bar, $bah );

I imagine it's because the sniff marked $str_replace as an undefined variable (it should have been a function symbol instead: str_replace).

[rogertheriault]

Exactly - with PHP 7.x, you get a fatal

Fatal error: Uncaught Error: Function name must be a string 

[GaryJones]

Is that something php -l (and therefore the CI bot) would pick up?

[gudmdharalds]

No, php -l would not pick this up.

[rogertheriault]

Simple test:

Rogers-MacBook-Pro:clients roger$ cat foo.php
<?php

$a = $foo('bar');

Rogers-MacBook-Pro:clients roger$ php -l foo.php 
No syntax errors detected in foo.php

Rogers-MacBook-Pro:clients roger$ php foo.php 

Fatal error: Uncaught Error: Function name must be a string in /Users/roger/clients/foo.php:3
Stack trace:
#0 {main}
  thrown in /Users/roger/clients/foo.php on line 3

Rogers-MacBook-Pro:clients roger$ php -v
PHP 7.1.16 (cli) (built: Mar 31 2018 02:59:59) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies

[tomjn]

I'm in favour of a 2 stage process, replace, then improve, with the former being a much higher priority, and the latter being a longterm nice to have. In the meantime, the file can probably stand for a hefty dose of extraction prior to its break up

[david-binda]

Thanks for opening the discussion! @sirbrillig 's version is being used internally on WordPress.com and thus receives enough attention and support. I personally find the variable analysis being valuable when reviewing code - it helps preventing some bad bugs which would otherwise be easily missed.

That said, if replace for @sirbrillig 's version is an option, I'm all for it!