How to detect dynamic declarated fields on objects with codesniffer in PHP

Question

After a refactoring, we had something like this in one of our classes:

class FooBar
{
    // $foo was $bla before
    private $foo;

    public function setBlubbOnArrayOnlyOnce($value)
    {
        // $this->bla was forgotten during refactoring. Must be $this->foo
        if(!isset($this->bla['blubb'])) {
             $this->foo['blubb'] = $value;
        }
    }
}

So in the end $this->foo['blubb'] was always set, not only once. This happens because of the magic methods of PHP. We don't want it to be possible to access fields dynamically, so I thought I just add a codesniffer rule. But I didn't found any and asked me why.

PHPStorm shows a field declared dynamically notice there, but I want this to automatically fail with codesniffer (or something similar) during our deployment cycle.

Has anybody an idea on this? Is there a good rule? Should I write my own and how? Or would it be bad practice to disable it?

Disclaimer: We use tests, but sometimes you miss things... It would be good to prevent this in the first place. Also, please don't come up with overwriting the magic methods. I don't want to have a trait/abstract whatever in every class.

Answer 1

This is not a codesniffer or phpstorm problem. And you cant want fix this problem with codesniffer or IDE. IDE, codesniffer, phpdocumentor, etc. -- this is "statically" analyse. And for dynamic analyse you can use e.g. phpunit.

If you want check existence of property you must use property_exists() function.

class X
{
    public function __get($name)
    {
        $this->{$name} = null;
        return $this->{$name};
    }
}

$x = new X();
var_dump(property_exists($x, 'foo')); // false
var_dump($x->foo); // NULL
var_dump(property_exists($x, 'foo')); // true

Or may be you can use reflection for property http://php.net/manual/en/class.reflectionproperty.php

If you want check for "isset" you must known:

var_dump(isset($x), $x); // false + NULL with notice
$x = null;
var_dump(isset($x), $x); // false + NULL
unset($x);
var_dump(isset($x), $x); // false + NULL without notice

When you shure for this case of check you can use isset()

But you should always first check for existence of property. Otherwise you can have undefined behaviour of your code.

Answer 2

After a refactoring

It would be good to prevent this in the first place.

You can only catch these kind of refactoring errors by running tests after each refactoring step. This error will also bubble up, because foo['blubb'] is set to a specific value and this should cause an unwanted effect in another test - not only in the test for the setter logic.

We use tests, but sometimes you miss things...

Yes, its quite common that the coverage is not high enough. That's why having a good test coverage is the starting point for all refactorings.

These two lines were not "green" in your coverage report:

   if(!isset($this->bla['blubb'])) {
       $this->foo['blubb'] = $value;

Also, please don't come up with overwriting the magic methods. I don't want to have a trait/abstract whatever in every class.

You have excluded it, but that's one way to catch the properties: by using the magic function __set() (for inaccessible vars) or property_exists() or the use of Reflection* classes to find.

---

Now, that its too late, you want another tool to catch the error, ok:

The tool would need to parse the PHP file and its parents (because of variable scope) and find $this->bla without a prior public|private|protected variable (class property) declaration. This will not indicate the exact type of error, just that "bla" was accessed without declaration.

Its possible to implement this as a CodeSniffer rule.

You can also give http://phpmd.org/ or https://scrutinizer-ci.com/ a try. And, in case you are using PHP7: https://github.com/etsy/phan

tl;tr

Its complicated to determine the exact error and its context without running, evaluating and analyzing the underlying code. Just think about "dynamic variable names" and you know why: you don't even know the name of the property by looking at the source-code, because its build dynamically during program flow. A static analyzer wouldn't catch that.

A dynamical analyzer has to track all things, here $this-> accesses and would take the context into account: !isset(x). Context evaluation can find lots of common coding mistakes. In the end you can build a report: saying that $this->bla was accessed only 1 time and that indicates either that

• a dynamically declared property was introduced, but never re-used, with the suggestion that you might drop it or declare it as class property
• OR with added context evaluation: that and when this variable was accessed from inside a isset() - that a non-existing key of a non-declared property was accessed, without a prior set(), etc.

Answer 3

Now in 2017, you'are looking for tool PHPStan. I link short intro I wrote for first time users.

It does exactly what you need!