Undefined Variable Rule does not understand static class variables.

[matweew]

• PHPMD version: 2.8.0
• PHP Version: 7.2.25
• Installation type: composer
• Operating System / Distribution & Version: Debian 9.6

Current Behavior

The new feature "Undefined Variable Rule" introduced in https://github.com/phpmd/phpmd/pull/497 is not working with static class variables, such as

class CmsBlock
{
    protected static $cmsBlockBuffer = [];

    protected function getCmsBlockById(string $cmsBlockKey): array
    {
        if (isset(static::$cmsBlockBuffer[$cmsBlockKey])) {
            return static::$cmsBlockBuffer[$cmsBlockKey];
        }
     
        return static::$cmsBlockBuffer[$cmsBlockKey] = $this->findCmsBlockByKey($cmsBlockKey);
    }
}

PhpMd triggers error message Avoid using undefined variables such as '$cmsBlockBuffer' which will lead to PHP notices.

Expected Behavior

No error messages.

Steps To Reproduce:

Explain all the steps you did to create this bug so we can reproduce it.

Checks before submitting

• [+] Be sure that there isn't already an issue about this. See: Issues list
• [+] Be sure that there isn't already a pull request about this. See: Pull requests
• [+] I have added every step to reproduce the bug.
• [+] If possible I added relevant code examples.
• [+] This issue is about 1 bug and nothing more.
• [+] The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".

[kylekatarnls]

I ran some tests. All is fine as long as the static variable is not an array. So the probable cause is PDepend evaluate the $array[$key] before static::$array, so it looks for a local variable named $array instead of the static property static::$array. The fact that the error is not raised when wrapping the property in parentheses (static::$cmsBlockBuffer) tends to confirm this hypothesis.

[Axent96]

I ran some tests. All is fine as long as the static variable is not an array. So the probable cause is PDepend evaluate the $array[$key] before static::$array, so it looks for a local variable named $array instead of the static property static::$array. The fact that the error is not raised when wrapping the property in parentheses (static::$cmsBlockBuffer) tends to confirm this hypothesis.

I can approve the hypothesis with parentheses.

[dereuromark]

Is there any update on this one?

[kylekatarnls]

I have a wide refactorization in progress because the new rules about variables revealed problems in the way we detect variables then detect if they are related to a read or write operation that are actually in PDepend for a very long time.

This should fix this error but it will take time.

[Axent96]

the same problem:

PHPMD version: 2.8.2 PHP Version: 7.4.3 Installation type: composer

[kylekatarnls]

Hi @matweew @Axent96 and @dereuromark!

Could you please try to install "phpmd/phpmd": "dev-feature/issue-714-fix-self-and-static-properties-access" in your composer.json to test the fix #781 with your applications?

Thanks,

[xatham]

Hello @kylekatarnls . any chance of this getting a patch (tag)? I guess not because it sounded like a lot of, possibly breaking, changes.

[kylekatarnls]

2.9.0 will only contain 1 new feature: Add rule for PHP's @ operator. Everything else in this version will be bugfixes. So you will just have to disable 1 rule to get the same features as in 2.8.2 as if 2.9.0 would have been a 2.8.3.

[t-heuser]

It says that in 2.9.0 it's fixed but I'm still getting the same error on static variables which are arrays when using 2.9.0 and PHP 7.3.22.

[kylekatarnls]

Hi @oneserv-heuser I get no error with PHPMD 2.9.0 and the code chunk given in this issue. Can you provide a chunk to reproduce your issue?

[t-heuser]

@kylekatarnls It happens when you have an array of objects and try to access a specific element and calling a function on it. Here is an simplified example:

class Test
{
    protected static $test;

    public function test(): void
    {
        self::$test[0]->getTest();
    }
}

Or is it intended behavior?

[kylekatarnls]

It's not. I see the difference with the initial issue, return self::$test[0]; does not trigger the error while return self::$test[0]->foo(); triggers it because the ->foo() is parsed before the self::.

I'm on it.

[Axent96]

I have the same problem: PHPMD version: 2.9.1 PHP Version: 7.4.9 Installation type: composer

if (isset(self::$var[$cacheHash][$url])) { $models[$url] = self::$var[$cacheHash][$url]; }

<violation beginline="277" endline="277" rule="UndefinedVariable" ruleset="Clean Code Rules" priority="3">
  Avoid using undefined variables such as '$var' which will lead to PHP notices.
</violation>
<violation beginline="278" endline="278" rule="UndefinedVariable" ruleset="Clean Code Rules" priority="3">
  Avoid using undefined variables such as '$var' which will lead to PHP notices.
</violation>

[koekaverna]

@kylekatarnls Hello there!

PHPMD version: 2.9.1 PHP Version: 7.4.11 Installation type: composer

I have similar problem with Symfony\Component\HttpFoundation\Response There is

    public static $statusTexts = [
        100 => 'Continue',
        101 => 'Switching Protocols',
        102 => 'Processing',            // RFC2518

So I have error when use Response::$statusTexts[$statusCode].

[a-menshchikov]

Hi there! Is there any news about this issue?

[tvbeek]

@a-menshchikov As you see it is reopened. @kylekatarnls is assigned but that was from before the reopening so I'm not sure he will pick it up. Feel free to create a PR to fix this or wait till someone else has the time to do it 😃

[char101]

The difference between SomeClass::$var and SomeClass::$var['key'] is that collectPropertyPostfix in UndefinedVariable.php only recognize SomeClass::$var, so we need to handle ASTArrayIndexExpression too:

diff --git a/src/main/php/PHPMD/Rule/CleanCode/UndefinedVariable.php b/src/main/php/PHPMD/Rule/CleanCode/UndefinedVariable.php
index 510e0c2d..e928436a 100644
--- a/src/main/php/PHPMD/Rule/CleanCode/UndefinedVariable.php
+++ b/src/main/php/PHPMD/Rule/CleanCode/UndefinedVariable.php
@@ -18,6 +18,7 @@
 namespace PHPMD\Rule\CleanCode;

 use PDepend\Source\AST\ASTArray;
+use PDepend\Source\AST\ASTArrayIndexExpression;
 use PDepend\Source\AST\ASTClass;
 use PDepend\Source\AST\ASTPropertyPostfix;
 use PDepend\Source\AST\ASTUnaryExpression;
@@ -274,6 +275,9 @@ class UndefinedVariable extends AbstractLocalVariable implements FunctionAware,

         foreach ($properties as $property) {
             foreach ($property->getChildren() as $children) {
+                while ($children instanceof ASTArrayIndexExpression) {
+                    $children = $children->getChild(0);
+                }
                 if ($children instanceof ASTVariable) {
                     $this->addVariableDefinition($children);
                 }

[mshamaseen]

2022 and this issue is still not fixed in version 2.11.1

[tvbeek]

@mshamaseen the beauty of opensource is that everyone can contribute 😃 Feel free to create a PR with a fix and test.

It is 2022 and everyone is welcome to contribute and improve the tools that are provided for free. I love that because the maintainers doesn't have an unlimited amount of time, energy and motivation so they need to prioritize.

[AJenbo]

If this feature is important to you, you could also hire a freelancer to work on it

[mshamaseen]

I totally understand, maintaining an opensource package is not an easy task and very time-consuming too, we are all thankful for this great opensource project and we are wishing for it to progress further.

on the other hand, what I don't understand is the upset from someone telling the users not to expect bugs to be fixed, I believe that everybody has the right to know about the status of an opensource project and whether it is maintained or not, people should understand that they need to hire freelancers for this project if they want it to be maintained before starting using it.

[bandicsongor]

(working) workaround for Axent96's example:

$_var =& self::$var;
if (isset($_var[$cacheHash][$url])) { $models[$url] = $_var[$cacheHash][$url]; }

(define by reference is optional, in my case was necessary, in this case isn't)