phpstan: Don't complain about undefined property fetch if __set exists

Given:

class A {
  /** @var string[] */
  private $data = [];

  public function __get(string $var) : ?string {
    return $this->data[$var] ?? null;
  }

  public function __set(string $var, string $value) {
    $this->data[$var] = $value;
  }
}

$a = new A();
$a->foo = "bar";
echo $a->foo;

Expected: No errors

Actual:

 ------ ------------------------------------------ 
  17     Access to an undefined property A::$foo.  
 ------ ------------------------------------------

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 19 (15 by maintainers)

Most upvoted comments

This is a pattern we use at Vimeo a lot:

class A {
  protected $foo;

  public function __get(string $var) : ?string {
    if (property_exists($this, $var)) {
        return $this->$var;
    }
    
    throw new Exception('property doesn’t exist');
  }

  public function __set(string $var, string $value) {
    if (property_exists($this, $var)) {
        $this->$var = $value;
        return;
    }
    
    throw new Exception('property ' . $var . ' doesn’t exist');
  }
}

And I imagine it’s used elsewhere too. Requiring @property annotations to prevent Access to protected property A::$foo errors seems slightly backwards, as PHP doesn’t have any problem running the code.

muglug on Jan 5, 2018

You can make it the right way and tell PHPStan which magic properties exist and which do not with an extension. If you don’t want to bother with this, you can use ignoreErrors key or universalObjectCrateClasses. There’s plenty of solutions, but saying that anything with __set defines every prioperty is wrong and would make PHPStan less useful.

On Fri, 5 Jan 2018 at 21:09, Ilija Tovilo notifications@github.com wrote:

@muglug https://github.com/muglug I see. I’d still consider this an anti-pattern for the reasons listed. You should probably use your setters to update updated_properties.

I think what it comes down to is your strategy. PHPStan probably yields more false positives than other static analyzers. The question is if you want err on the side of caution and report these cases or not.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/phpstan/phpstan/issues/738#issuecomment-355653161, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGZuHicK6r0JMqbEID1RMtiLhga0u8Kks5tHoF-gaJpZM4RT0Pi .

–

Ondřej Mirtes

ondrejmirtes on Jan 5, 2018

@muglug You cannot find out which properties are valid and which aren’t unless you actually run the code. __get and __set could preg_match the name of the property and only allow it in certain situations.

FWIW you can also always create your own reflection class and just allow all properties if your class implements the __get and __set methods. This has several disadvantages though:

It could hide bugs. Again, the __set method could only allow the property bar and throw in all other instances
You lose type-safety
You lose autocompletion in your IDE

IMHO you should avoid using the __get and __set methods whenever possible. Why not just expose an array?

iluuu1994 on Jan 5, 2018