This module claims to support php 5.4 and up but when I run the latest version on php 5.6 it fails with a syntax error.

$ php --version
PHP 5.6.39 (cli) (built: Dec  7 2018 08:27:47)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies
    with Xdebug v2.5.5, Copyright (c) 2002-2017, by Derick Rethans
$ composer show sirbrillig/phpcs-variable-analysis
name     : sirbrillig/phpcs-variable-analysis
descrip. : A PHPCS sniff to detect problems with variables.
keywords :
versions : * v2.6.2
type     : phpcodesniffer-standard
license  : BSD 2-Clause "Simplified" License (BSD-2-Clause) (OSI approved) https://spdx.org/licenses/BSD-2-Clause.html#licenseText
source   : [git] https://github.com/sirbrillig/phpcs-variable-analysis.git a31046c32e95cff4062856a3eb78770dd1bc1e48
dist     : [zip] https://api.github.com/repos/sirbrillig/phpcs-variable-analysis/zipball/a31046c32e95cff4062856a3eb78770dd1bc1e48 a31046c32e95cff4062856a3eb78770dd1bc1e48
path     : /.../vendor/sirbrillig/phpcs-variable-analysis
names    : sirbrillig/phpcs-variable-analysis

autoload
psr-4
VariableAnalysis\ => VariableAnalysis/

requires
php >=5.4.0

requires (dev)
dealerdirect/phpcodesniffer-composer-installer ^0.4.4
limedeck/phpunit-detailed-printer ^3.1
phpunit/phpunit ^6.5
sirbrillig/phpcs-import-detection ^1.1
squizlabs/php_codesniffer ^3.1
$ composer lint
> phpcs
PHP Parse error:  syntax error, unexpected '?' in /.../vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php on line 1062
PHP Stack trace:
PHP   1. {main}() /.../vendor/squizlabs/php_codesniffer/bin/phpcs:0
PHP   2. PHP_CodeSniffer\Runner->runPHPCS() /.../vendor/squizlabs/php_codesniffer/bin/phpcs:18
PHP   3. PHP_CodeSniffer\Runner->init() /.../vendor/squizlabs/php_codesniffer/src/Runner.php:70
PHP   4. PHP_CodeSniffer\Ruleset->__construct() /.../vendor/squizlabs/php_codesniffer/src/Runner.php:332
PHP   5. PHP_CodeSniffer\Ruleset->registerSniffs() /.../vendor/squizlabs/php_codesniffer/src/Ruleset.php:216
PHP   6. PHP_CodeSniffer\Autoload::loadFile() /.../vendor/squizlabs/php_codesniffer/src/Ruleset.php:1146

Parse error: syntax error, unexpected '?' in /.../vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php on line 1062

Call Stack:
    0.0024     230080   1. {main}() /.../vendor/squizlabs/php_codesniffer/bin/phpcs:0
    0.0061     709800   2. PHP_CodeSniffer\Runner->runPHPCS() /.../vendor/squizlabs/php_codesniffer/bin/phpcs:18
    0.0176    1318656   3. PHP_CodeSniffer\Runner->init() /.../vendor/squizlabs/php_codesniffer/src/Runner.php:70
    0.0221    1685624   4. PHP_CodeSniffer\Ruleset->__construct() /.../vendor/squizlabs/php_codesniffer/src/Runner.php:332
    0.0326    1731848   5. PHP_CodeSniffer\Ruleset->registerSniffs() /.../vendor/squizlabs/php_codesniffer/src/Ruleset.php:216
    0.0594    4048304   6. PHP_CodeSniffer\Autoload::loadFile() /.../vendor/squizlabs/php_codesniffer/src/Ruleset.php:1146

Script phpcs handling the lint event returned with error code 255

I think that the issue comes from https://github.com/sirbrillig/phpcs-variable-analysis/blob/master/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php#L1062 which uses the ?? syntax. I think that this is a new syntax that only showed up in php 7+

Since this code must work in PHP 5.6, we cannot use typed arguments (at least not simple types like bool, string, etc.) or return types in the code. It'd be nice to add phpdoc to all the functions so that their types are clear.

Here's a screenshot of using your project:

I added this standard to my composer, and the DealerDirect plugin you recently added support for seemed to register it. Note, however, that the standard is simply the vendor name, and not the vendor/package name, as per the other standards (which have had support for the DealerDirect plugin for a while now). One obvious issue, is that should you come up with a new repo for a different PHPCS standard, there will be a conflict.

Here's the relevant part of my .phpcs.xml.dist:

<rule ref="ObjectCalisthenics"/>
<rule ref="phpcs-variable-analysis.CodeAnalysis.VariableAnalysis"/>
<rule ref="PHPCompatibility"/>
<config name="testVersion" value="7.1-"/>
<config name="minimum_supported_wp_version" value="4.7"/>
<rule ref="WordPress">
	<exclude name="WordPress.VIP"/>
</rule>

Again, note that I need to add the full path to the sniff for your repo, unlike in other standards. I would expect to just be able to use <rule ref="phpcs-variable-analysis"/>, or <rule ref="VariableAnalysis"/> (as an updated top-level name of the standard), so that any future sniffs you add would be automatically used.

I suspect the issue is the directory layout - see PHPCompatibility/PHPCompatibility#446 for relevant discussion of what PHPCompatibility had to do (they were also trying to maintain BC, but you may feel that your project is young enough to fix the layout, bump the major version number and move on).

If you look at the repos for PHPCompatibility and WPCS they have a structure of:
{repo}->StandardName->Sniffs+ruleset.xml.

ObjectCalisthenics has it as:
{repo}->src->StandardName->Sniffs+ruleset.xml.

However, your repo here doesn't have that StandardName level. It is simply:
{repo}->Sniffs+ruleset.xml.

If you moved your Sniffs/, Tests/ and ruleset.xml all into a new top-level VariableAnalysis/ directory, then I think the issue will just about be solved, and you'll be consistent with the other standards.

this code
[$a, $b] = [1, 2];
cause fail warnings

Variable $a is undefined.
Variable $b is undefined.

version is: 2.1.2

This package currently seems not to be well-equipped for anonymous classes. 🙂

$object = new class {
    public $foo; // "Variable $foo is undefined"

    public function __construct()
    {
        $this->foo = 'bar'; // "Variable $this" is undefined
    }
};

(Thanks for the great package by the way! I tend to get pretty mechanical when reporting bugs, but I really should sprinkle some gratefulness here and there. ❤️)

If there is code like:

$hello = 'world';
$hello = 'abc';
echo $hello;

the first $hello declaration is not reported as "unused" even though it should, since it isn't used & this line is useless.

This case often happens when refactoring (of course in a more complicated way, with more code between the lines)

An example from WordPress phpunit tests producing false-positive reports of undefined variables for both functional lines:

// $q is defined above.
wp_parse_str( $q, $ss );
$this->assertEquals( $ss, wp_parse_args( $q ) );

While in the first case, the $ss is not being defined at the point of wp_parse_str call, it's being created and populated inside the function, in the second case, the $ss has already been populated.

Should be easy to fix the lint issues by defining $ss as an empty array prior this code, but the notation in the example feels like a commonly used idiom in PHP (not only for wp_parse_args, but also for parse_str and other functions with similar variable creation capability).

How about a new sniff, that checks for the length of the variable name, and gives a warning if it's below or above certain (changeable in phpcs.xml.dist) values?

Give exceptions for variables in for () statements?

There may already be a sniff in the PHPCS built-in standards that does this.

Variable variables are considered unused when only appearing on the left-hand side of an assignment.

This is considered fine:

function fn()
{
    $foo = true;
    $var = 'foo';
    return $$var;
}

While this says $var is unused:

function fn()
{
    $foo = true;
    $var = 'foo';
    $$var = false;
}

($foo is marked unused in both cases, but I guess there's no way to avoid that without deeper program flow analysis.)

I've been fiddling around with VSCode plugins, phpcs config, and integrating this plugin for several hours now. I've never used phpcs before and find the whole ecosystem quite confusing. It would be really helpful to see a step-by-step guide to get Variable Analysis working in VSCode. Simply put, I'd like to see VSCode highlight variable typos as soon as they happen. Thanks.

Originally reported in Automattic/phpcs-neutron-standard#73 by @beaucollins

The following example reports an undefined variable warning for $static_member_var when it should not:

class ClassWithLateStaticBinding {
    public static $static_member_var;
    static function method_with_late_static_binding() {
        $called_class = get_called_class();
        echo $called_class::$static_member_var;
    }
}

Hi there,
Sorry If I am completely missing the point but is there any way to also allow phpcs-variable-analysis to flag undefined method calls, like:

function do_that( $foo, $bar ) { ... }

doThat( 'foo', 'bar' ); //<- throws error as doThat doesn't exist

another_method_with_callback( 'foo', doThat ) // < also throws error as it's calling doThat

If not possible, perhaps anyone knows of an alternative?

Thanks in advance

This test case will show a warning where none should be shown:

class Person extends Being {
  function getName() {
    return parent::$name;
  }
}

Both self::$name; and Being::$name; are ignored correctly.

Simple sample:

$nav_menu = 'hello world';
$selector = 'good morning';
$nav_menu = preg_replace( '/(hello \w+)/', "$selector$1", $nav_menu, 1 );

It will report the $1 as undefined variable. within preg_replace variables that match $\d+ should only be reported as undefined, if there is no matching group in the search of the regex.

The tests should be able to run with a simple call to composer test.

Sometimes I have to only use the keys of an array. Using an foreach loop is faster than first doing array_keys() and then looping.
However it will always report the $value as unused (bc it is unused).

If would be nice if it would NOT report the $value as unused if the $key is used:
(or if that is too complicated, maybe not at all if its a key => value foreach loop)

foreach ( $array as $key => $value ) {

@DavidRothstein suggested this, and I think it might be a good idea. When an abstract class has methods, those methods may have arguments but no function body (essentially a function signature). We may want to ignore those unused variable warnings.

Example:

<?php

abstract class MasterTestCase {
  protected function doSomething( int $foo, array $bar ) { // this will report two warnings
  }
}

Originally described in #84.

Variables in PHP may have a dynamic symbol name, and are called variable variables. For example, the following function prints hello, which is a string stored in the variable $greeting, even though that variable is never explicitly mentioned:

function usedVariableVariable() {
  $greeting = 'hello';
  $varName = 'greeting';
  echo $$varName;
}

Here's the process I would imagine using to identify such a variable for the purposes of this sniff:

1. Is the current variable defined? Good.
2. Is the current variable preceded by a T_DOLLAR or a T_DOLLAR and a T_OPEN_CURLY_BRACKET? If so, it's a variable variable.
3. Resolve the value of the current variable (this is extremely hard).
4. Process the T_DOLLAR as though it were a variable with the symbol name of the resolved current variable.
5. Start again at 1 since there may be several levels.

Step 3 is very difficult because it requires executing the PHP code or a deep static analysis. Maybe there's something else we can do, though?

Following is perfectly valid PHP 5.5+ code:

$data = [
    ['foo', 'Foo'],
    ['bar', 'Bar'],
];

foreach ($data as list($name, $label)) {
    printf('<div id="%s">%s</div>', $name, $label);
}

However, it raises the following warnings:

------------------------------------------
WARNING | Variable $name is undefined.
WARNING | Variable $label is undefined.
------------------------------------------

Split from #37

When I try to run composer install on a package that requires this package, I get the following error:

    - sirbrillig/phpcs-variable-analysis dev-master requires roave/security-advisories dev-master -> satisfiable by roave/security-advisories[dev-master] but these conflict with your requirements or minimum-stability.

This was introduced in #5

To reproduce, have a composer.json with these requirements:

    "require-dev": {
        "sirbrillig/phpcs-variable-analysis": "dev-master"
    }

Hi again,

Regarding to the previous issue #79, now I have found a new bug with static variables, you can reproduce it with this code:

<?php

class MyClass extends MyForms
{
    private static $defaultClass = 'OtherClass';

    final public static function getForm($class = false)
    {
        if ($class !== false) {
            return new $class();
        }

        return new self::$defaultClass();
    }
}

Please check this capture.

Take the following code as an example:

<?php
namespace Whatever;

use Symfony\Component\HttpKernel\Exception\HttpException;
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
use Laravel\Lumen\Exceptions\Handler as ExceptionHandler;

final class LumenExceptionHandler extends ExceptionHandler {
    public function render($request, \Exception $e) {
        $status = 500;
        if ($e instanceof HttpExceptionInterface) {
            $status = $e->getStatusCode();
        }

        return response()->json(['message' => $e->getMessage()], $status);
    }
}

The render function takes 2 arguments. A request and an exception. We have to accept these two arguments because we're overwriting a method and we want to match it's signature.

In our implementation, we don't actually care about the $request param. PHPCS gives me a warning because $request is unused.

I want to be able to tell PHPCS that this argument is unused and that this is OK.

With Rubocop (a ruby linter), I can prefix an argument with _ to mark it as unused and to let the linter know that this is OK. Here's the relevant doc: https://rubocop.readthedocs.io/en/latest/cops_lint/#lintunusedmethodargument

We could have something like this:

public function render($_request, \Exception $e) {
  // ...
}

This would not generate any warnings since we've explicitly said that the argument should be unused.

The use of $this inside a closure should be fine, due to automatic binding. I'm not sure why there is an example explicitly marking this as a warning.

Here's a simple example that should report no warnings.

class classWithClosure() {
  public function test_get_source_information_from_theme() {
    doSomething( function() {
      return $this->thing;
    } );
  }
}

I think it's fair to allow $this to be allowed in traits since the trait has access to the rest of the class to which it is included.

$closure = function() {
    // Using $this here is fine.
    if ($this->something) {
        function nestedFunctionDeclaration() {
            // Using $this here is not as the nested function is a closed scope in the global namespace.
            if ($this->something) {
                // Do something.
            }
        }
    }
};

The use of $this within the nestedFunctionDeclaration() function is not reported.

I have a fix ready for this, but need a decision on #108 before I can pull it.

function name( $param ) {
    $var = 'something';
    require_once __DIR__ . '/views/my-view.php';
}

The above code currently generated an Unused function parameter and an Unused variable $var warning, however, as there is a require in the function, the parameter and the variable may well be (and are) used in the included file.

I'd suggest lowering the severity in that case to 4. That means that in a normal PHPCS run it will not show up, though can be made to show with --severity=4.

Alternatively a separate error code or a configurable property could possibly be used to prevent/silence these type of false positives.

Note: the below code sample should still throw an error/warning.

function name() {
    require_once __DIR__ . '/views/my-view.php';

    $var = 'something'; // Unused and as declared *after* the include, it cannot be used in the included file.
}

Say I have this function:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal = $newVal;
}

I will get the following message:

Unused global variable $myGlobal.

It seems like this is an erroneous message. Or at least it should be configurable.

Currently, I'm able to get around the warning this way:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal;
  $myGlobal = $newVal;
}

But this is not ideal.

I see that it is possible to allow certain variables to not be flagged if they are unused: https://github.com/sirbrillig/phpcs-variable-analysis#customization

I would love to have this same option for undefined variables. I am specifically interested in the $post variable, which is used liberally in the WordPress loop.

foreach ( $recipe_json as $k => $v ) {
	if ( false !== strpos( $k, '_rendered' ) ) {
		unset( $recipe_json[ $k ] );
	}
}

In this statement, $v is flagged as unused. I guess this is the same as #65?

I've this method in a class:

public function render() {
    global $app
    require($this->getFilename());
}

This raises this warning:

Unused global variable $app. (VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable)

I'm using the $app in the included file though. How should I fix this without using an (in this case unwanted) ignore-regexp?

In v1.0, the following detected $context as an unused variable (warning):

// deep in an array
'is_needed' => function ( $context ) {
	return true;
},

Since v2.0, this doesn't seem to be seen. Likewise, the following unused $foo is also not reported:

class Foo {
	/**
	 * Bar.
	 */
	public function bar( $foo ) {
		echo 'Foo::bar()';
	}
//...
}

Even a simple function doesn't report it:

function test($foo) {

}

The following does report $bar as unused, so I'm confident that the VariableAnalysis is installed correctly and registered:

function test($foo) {
	$bar = '';
}

37 | WARNING | [ ] Unused variable $bar. (VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable)

The relevant bit of my .phpcs.xml.dist:

<rule ref="VariableAnalysis"/>

When useing a variable reference in an anonymous function and not using the reference in an expression, but only as the left hand of an assignment, the variable is incorrectly spotted as "unused".

This is best explained by example:

$num = 0;

(function () use (&$num) {
    // "Unused bound variable $num"
    $num = 1;
})();

echo $num; // 1

FWIW, this behaviour only occurs on variable references passed through the use construct, not on function arguments:

$num = 0;

(function (&$num) {
    // This is fine
    $num = 1;
})($num);

echo $num; // 1

Hi,

I have the following error at the beginning of each PHP file:

[Internal.Exception] An error occurred during processing; checking has been aborted. The error message was: Argument 2 passed to VariableAnalysis\Lib\Helpers::findVariableScope() must be an instance of VariableAnalysis\Lib\int, integer given, called in /vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php on line 670 and defined in /vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Lib/Helpers.php on line 167

I'm using the latest version of PHPCS.

Thanks.

let's say I have this: (this is a Wordpress specific example, but can happen anywhere, but since it's so prominently used in WP on every action/filter it helps outline the significance of this)

add_action( 'delete_post_meta', 'check_thumbnail_updated_post_meta', -1000, 3 );
function check_thumbnail_updated_post_meta( $meta_id, $post_id, $meta_key ) {
    echo $post_id;
    echo $meta_key;
}

Eslint has a handling for this (no-unused-vars with after-use, you mention this here: #21 (comment)) which would be exactly what we also would need here in this php check.
So basically if any of the variables is used, then it can discard previous errors for unused vars in this call.

A variable defined in a foreach statement and never used is not found.

$foo = [1, 2, 3];
// The next line should report an unused variable
foreach ( $foo as $bar ) {
  echo 'hi';
}

tldr: The global scope is not scanned because there's no token to indicate when it ends.

Hey,

phpcs: PHP_CodeSniffer version 3.4.1 (stable) by Squiz (http://www.squiz.net)
sirbrillig/phpcs-variable-analysis: v2.6.0

It looks like anonymous classed aren't fully supported.
I did notice this issue: #52 which is referenced in this pr: https://github.com/sirbrillig/phpcs-variable-analysis/pull/53/files

Both changes in VariableAnalysis/Lib/Helpers.php and VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php are in my local installation.

However, this heavily simplified file:

<?php
declare(strict_types=1);

namespace Test;

class Test {
    public function getTest() {
        return new class() {
            protected $property;

            public function __construct() {
                $this->property = 'my-property';
            }

            public function getProperty() {
                return $this->property;
            }
        };
    }
}

Shows the following error: 13 | WARNING | Variable $property is undefined.

A var_export($token) on vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Lib/Helpers.php:196 shows:

array (
  'code' => 320,
  'type' => 'T_VARIABLE',
  'content' => '$property',
  'line' => 13,
  'column' => 23,
  'length' => 9,
  'level' => 3,
  'conditions' =>
  array (
    16 => 361,
    25 => 346,
    39 => 'PHPCS_T_ANON_CLASS',
  ),
)

I'm not that knowledgable about the inner workings of the code sniffer, but maybe you can reproduce the issue and possibly fix it?

Thanks anyway!

Hi,

I think this bug was inserted in the last release, the problem occurs with some variables defined as static, please check this capture.

In the following example (derived from an atoum test), $service is reported as being undefined.

$this->given(
  $service = new Service(),
  $service->something()
);

function abc() {
    global $test;
    list( $test, $example ) = my_function( $_POST );

    return $example;
}

will report:
Unused global variable $test.

But it isn't unused, since we assign a new value to it.

Add phpunit.xml.dist rather than hard-coding the phpunit configuration into the scripts section of composer.json.

h/t @GaryJones

I like the idea of allowing some variables only in some files. Other linteres have a /* global */ special comment, where you can define variables which are passed into the file:
https://eslint.org/docs/rules/no-undef
https://www.jslint.com/help.html#global

I know phpcs-variable-analysis has an validUnusedVariableNames option. But this is for all files and can not be set on a per-file basis.

Traits can define properties which are currently marked as undefined.

Example:

trait JsonInjectable
{
    protected $jsonReader; // "Variable $jsonReader is undefined"
}

In composer.json, the license is given as BSD, but this is not valid. The value should be one of the values from https://spdx.org/licenses/, and as can be seen, there are several variations of "BSD".

Code speaks a thousand words.

function some_function( $var_1 ) { // VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable

    $var_2 = get_something(); // VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable

    do_something( get_defined_vars() ); // this should negate above warnings.
}

The static, self and $this inside closures defined inside class scope in PHP prior to 5.4.0 produced fatal errors. But PHP 5.4.0 automatically bounds current class' scope to closure, making $this, self, and static available inside of the function's scope.

The logic for flagging self inside closures was introduced 8 years ago ( 20 Jul 2011
), prior the release of PHP 5.4.0 (which happened on 01 Mar 2012 ).

Following code is valid in PHP 5.4+:

<?php

class My_Class {

    protected static $output = 'test';

    public static function method() {
        return function() {
            echo self::$output;
        };
    }
}

$object = new My_Class;
$function = $object->method();
$function();

See https://3v4l.org/B43Uj and https://3v4l.org/sdJ6Q with code examples and their processing in different PHP versions (works as expected in PHP 5.4+).

Perhaps it's time to stop flagging those usages?

Code speaks a thousand words. Replace and with the other logical operators (or, xor), and the same error will yield.

$get_something_and_do_stuff = true;

$get_something_and_do_stuff
	and $something = get_something()
	and $something === 'something' // VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable
	and do_stuff();

Variable $something is undefined.
(VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable)

This is tricky, however, because the logical operators may prevent or bypass assigning variables; especially when the or-operator is used in sequence.

You do not need to register/create a variable before assigning indexes. Instead, you can directly create a variable as an array. You can also see this in PHP's array example 12.

$a['index'] = 'value';
print_r( $a ); // Array ( [index] => value )

$b[] = 'value';
print_r( $b ); // Array ( [0] => value )

$c['index'][]['deep'] = 'value';
print_r( $c ); // Array ( [index] => Array ( [0] => Array ( [deep] => value ) ) )

However, this warning is thrown at the variable assignment, and whenever later you reuse the variable:

Variable $a is undefined.
(VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable)

Now, do keep in mind that PHP's documentation doesn't explicitly state if and why this would work. So, whether this is a best practice or not is up for debate, but I don't think this specific warning should be thrown—it should be mitigated elsewhere.

As for examples in the wild, I found one in WordPress:

• https://github.com/WordPress/WordPress/blob/7112ee71329533c9a168e4736582f537e30f7068/wp-admin/edit-comments.php#L286

The following example marks a warning on line 5, but it should not.

<?php

class MyClass {
    public function myFunc($meta, $callback) {
        $get_meta_callback = $callback;
        return self::$get_meta_callback( $meta  );
    }
}

Split from #8