Skip to content

Check private properties on universal crate objects #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Check private properties on universal crate objects #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3b8246
Fixed 'bug' in example data
JeroenVanOort Jan 9, 2020
a13ed5b
Handle native properties on universal object crates
JeroenVanOort Jan 9, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Reflection/Php/UniversalObjectCratesClassReflectionExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ public function setBroker(Broker $broker): void

public function hasProperty(ClassReflection $classReflection, string $propertyName): bool
{
if ($classReflection->hasNativeProperty($propertyName)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for subclasses of stdClass. Ask $classReflection->isSubclassOf.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only for subclasses of stdClass? I'm trying to understand but I don't at the moment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because only stdClass behaves like this. We cannot be sure how others implement __get/__set behaviour.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand you correctly when I think that this is a use case?

class Foo { private $bar = 43; public function __get($name) { return 42; } } $foo = new Foo(); $foo->bar; // 42, not 43 nor access to a private property

Shouldn't we solve this by looking at visibility? Because even if Foo were a subclass of stdClass, this would still result in an error. And it would be broader than just sublcasses of stdClass.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a valid use-case I'm talking about.

If $bar was public, UniversalObjectCratesClassReflectionExtension isn't even executed.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm starting to get this, although I don't think we should check if the class we're working on is a subclass of stdClass, because those don't appear to behave different from a class that is not a subclass of stdClass:

class A extends \stdClass { private $bar = 43; public function __get($name) { return 42; } } class B extends \stdClass { private $bar = 43; } class C { private $bar = 43; public function __get($name) { return 42; } } class D { private $bar = 43; } $a = new A(); $b = new B(); $c = new C(); $d = new D(); echo $a->bar; // 42, not 43 echo $b->bar; // Cannot access private property B::$bar echo $c->bar; // 42, not 43 echo $d->bar; // Cannot access private property D::$bar

Instead of checking if a native property exists, we should probably check if the __get method doesn't. Otherwise, $a->bar and $c->bar would falsely be reported as problematic when A and B are configured as universal crate objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class A is an edgecase we don't have to care about. (extends stdClass + custom __get)
Class B is the case I want to get solved in this PR.
Class C currently works (and should not get broken by this PR).
Class D does not have __get method, should not be added to universalObjectCrateClasses.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also solve A with one more if statement but it's not necessary.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C will not get falsely reported because C does not extend stdClass.
return false;
}

return self::isUniversalObjectCrate(
$this->broker,
$this->classes,
Expand Down
6 changes: 5 additions & 1 deletion tests/PHPStan/Analyser/AnalyserIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,13 @@ public function testAnonymousClassesWithComments(): void
public function testUniversalObjectCrateIssue(): void
{
$errors = $this->runAnalyse(__DIR__ . '/data/universal-object-crate.php');
$this->assertCount(1, $errors);
$this->assertCount(3, $errors);
$this->assertSame('Parameter #1 $i of method UniversalObjectCrate\Foo::doBaz() expects int, string given.', $errors[0]->getMessage());
$this->assertSame(19, $errors[0]->getLine());
$this->assertSame('Parameter #1 $i of method UniversalObjectCrate\Foo::doBaz() expects int, string given.', $errors[1]->getMessage());
$this->assertSame(36, $errors[1]->getLine());
$this->assertSame('Access to private property UniversalObjectCrate\Foo::$name.', $errors[2]->getMessage());
$this->assertSame(36, $errors[2]->getLine());
}

public function testCustomFunctionWithNameEquivalentInSignatureMap(): void
Expand Down
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/data/universal-object-crate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ public function doBaz(int $i): void

function () {
$foo = new Foo('foo');
$foo->doBaz($foo->name); // not reported, is mixed here
$foo->doBaz($foo->name); // reported, private property
};
2 changes: 1 addition & 1 deletion tests/PHPStan/Reflection/Php/data/universal-object-crates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class DifferentGetSetTypes

public function __get($name): DifferentGetSetTypesValue
{
$this->values[$name] ?: new DifferentGetSetTypesValue();
return $this->values[$name] ?: new DifferentGetSetTypesValue();
}

public function __set($name, string $value): void
Expand Down