ForbiddenGlobalVariableVariable false positive

[glensc]

<?php

$client_id = "dcid";
$dcid = 123;

function f() {
        global $client_id;
        global $$client_id;

        return $$client_id;
};

var_dump(f());
#> int(123)

Global with variable variables is not allowed since PHP 7.0
(PHPCompatibility.PHP.ForbiddenGlobalVariableVariable.Found)

added via #110, bugfixed #316

however, this code is valid for php 5.x and 7.x (tested):

$ php53  -r '$client_id="dcid"; $dcid=123; $f=function() {global $client_id; global $$client_id; var_dump($$client_id); return $$client_id; }; $f();'
int(123)

$ php72  -r '$client_id="dcid"; $dcid=123; $f=function() {global $client_id; global $$client_id; var_dump($$client_id); return $$client_id; }; $f();'
int(123)

the behavior for references changed, but it is not deprecated:
http://php.net/manual/en/migration70.incompatible.php#migration70.incompatible.variable-handling.global

global only accepts simple variables
Variable variables can no longer be used with the global keyword. The curly brace syntax can be used
to emulate the previous behaviour if required:

function f() {
    // Valid in PHP 5 only.
    global $$foo->bar;

    // Valid in PHP 5 and 7.
    global ${$foo->bar};
}

[jrfnl]

@glensc Thanks for reporting this. I'm trying to figure out what is and what isn't valid now anymore in PHP 7.0+ to adjust the sniff. I'm also looking at overlap between this sniff and the VariableVariable sniff, as this sniff should not need to warn about things covered in the other sniff.

I've been looking at the changelog and the RFC and your examples and am not a 100% sure what to sniff for exactly. The question is basically "What are simple variables ?", as those are still allowed, anything else isn't.

I suppose we'll need to run some tests, but if anyone has some input which can be helpful, that would be appreciated.

What I suspect, is:

function testThis() {
    // Simple variables, still allowed:
    global $var, $var['key'], $var->key, $$var, $$$var;

    // Complex variables, not allowed since PHP 7.0
    //    => anything where a variable variable is combined with array or object access:
    global $$var['key'], $$var->key, $$var->key['key'],
        $$var::$staticvar, $$var::$staticvar['key'];

    // Complex variables using curly braces are fine
    // in both PHP 5.x as well as PHP 7.0+:
    global ${$var['key']}, ${$var->key}, ${$var->key['key']},
        ${$var::$staticvar}, ${$var::$staticvar['key']};
}

@glensc If you have any more information which I haven't found, that would be appreciated.

@wimg Any thoughts on this ?

Refs:

• http://php.net/manual/en/migration70.incompatible.php#migration70.incompatible.variable-handling.global
• https://wiki.php.net/rfc/uniform_variable_syntax#global_keyword_takes_only_simple_variables
• http://php.net/manual/en/language.variables.variable.php
• http://php.net/manual/en/language.variables.scope.php <- The section about the global keyword doesn't mention the change... ?

[jrfnl]

Ok, been running some tests with this.

Adjusted examples:

function testThis() {
    // Variables not allowed in global statement in ANY PHP version.
    // These will cause a parse error in every version tested on 3v4l.org.
    global $var['key'], $var->key;

    // Simple variables, still allowed:
    global $var, $$var, $$$var;

    // Complex variables, not allowed since PHP 7.0
    //    => anything where a variable variable is combined with array or object access:
    // The last two also give a parse error in PHP 5.2 and lower
    global $$var['key'], $$var->key, $$var->key['key'],
        $$var::$staticvar, $$var::$staticvar['key'];

    // Complex variables using curly braces are fine
    // in both PHP 5.x as well as PHP 7.0+:
    // The last two will give a parse error in PHP 5.2 and lower
    global ${$var['key']}, ${$var->key}, ${$var->key['key']},
        ${$var::$staticvar}, ${$var::$staticvar['key']};
}

More examples are welcome so we have a good set of test cases.

[jrfnl]

Another question which popped up in my mind: the PHP 7.0 manual upgrading section on this states explicitly that:

As a general principle, using anything other than a bare variable with global is discouraged.

While what we're talking about above would be an error, should the sniff throw a warning when anything but bare variables are encountered ?

[jrfnl]

@glensc I've opened PR #564 to address this. Please have a look and let me know what you think.

[glensc]

i'll just 👍, not going to dig into code details. good work!