Review items and adjustments for magic reflection extension

Author: mglaman

extension.neon

@@ -35,3 +35,6 @@ services:
 	-
 		class: PHPStan\Type\ServiceDynamicReturnTypeExtension
 		tags: [phpstan.broker.dynamicMethodReturnTypeExtension]
+	-
+		class: PHPStan\Reflection\EntityFieldsViaMagicReflectionExtension
+		tags: [phpstan.broker.propertiesClassReflectionExtension]

src/Drupal/Bootstrap.php

@@ -166,6 +166,7 @@ protected function addCoreNamespaces(): void
         $this->namespaces['Drupal\\FunctionalJavascriptTests'] = $core_tests_dir . '/FunctionalJavascriptTests';
         $this->namespaces['Drupal\\Tests\\TestSuites'] = $this->drupalRoot . '/core/tests/TestSuites';
     }
+
     protected function addModuleNamespaces(): void
     {
         foreach ($this->moduleData as $module) {

src/Reflection/EntityFieldReflection.php

@@ -2,6 +2,7 @@
 
 namespace PHPStan\Reflection;
 
+use PHPStan\Type\MixedType;
 use PHPStan\Type\ObjectType;
 use PHPStan\Type\Type;
 
@@ -27,12 +28,23 @@ public function __construct(ClassReflection $declaringClass, string $propertyNam
 
     public function getType(): Type
     {
-        if ($this->propertyName == 'original') {
-          // See Drupal\Core\Entity\EntityStorageBase::doPreSave
+        if ($this->propertyName === 'original') {
+            if ($this->declaringClass->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
+                $objectType = 'Drupal\Core\Entity\ContentEntityInterface';
+            } elseif ($this->declaringClass->isSubclassOf('Drupal\Core\Config\Entity\ConfigEntityInterface')) {
+                $objectType = 'Drupal\Core\Config\Entity\ConfigEntityInterface';
+            } else {
+                $objectType = 'Drupal\Core\Entity\EntityInterface';
+            }
+            return new ObjectType($objectType);
+        }
+
+        if ($this->declaringClass->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')) {
+            // Assume the property is a field.
             return new ObjectType('Drupal\Core\Field\FieldItemListInterface');
         }
 
-        return new ObjectType('Drupal\Core\Field\FieldItemListInterface');
+        return new MixedType();
     }
 
     public function getDeclaringClass(): ClassReflection

src/Reflection/EntityFieldsViaMagicReflectionExtension.php

@@ -13,16 +13,21 @@ class EntityFieldsViaMagicReflectionExtension implements PropertiesClassReflecti
     public function hasProperty(ClassReflection $classReflection, string $propertyName): bool
     {
         if ($classReflection->hasNativeProperty($propertyName)) {
-          // Let other parts of PHPStan handle this.
+            // Let other parts of PHPStan handle this.
             return false;
         }
 
         $reflection = $classReflection->getNativeReflection();
-        if ($reflection->implementsInterface('Drupal\Core\Entity\EntityInterface')) {
+        // We need to find a way to parse the entity annotation so that at the minimum the `entity_keys` are
+        // supported. The real fix is Drupal developers _really_ need to start writing @property definitions in the
+        // class doc if they don't get `get` methods.
+        if ($reflection->implementsInterface('Drupal\Core\Entity\ContentEntityInterface')) {
+            // @todo revisit if it's a good idea to be true.
+            // Content entities have magical __get... so it is kind of true.
             return true;
         }
         if ($reflection->implementsInterface('Drupal\Core\Field\FieldItemListInterface')) {
-            return FieldItemListPropertyReflection::canHandleProperty($propertyName);
+            return FieldItemListPropertyReflection::canHandleProperty($classReflection, $propertyName);
         }
 
         return false;

src/Reflection/FieldItemListPropertyReflection.php

@@ -2,9 +2,9 @@
 
 namespace PHPStan\Reflection;
 
-use PHPStan\Type\IntegerType;
-use PHPStan\Type\MixedType;
+use PHPStan\Type\NullType;
 use PHPStan\Type\ObjectType;
+use PHPStan\Type\StringType;
 use PHPStan\Type\Type;
 
 /**
@@ -27,25 +27,28 @@ public function __construct(ClassReflection $declaringClass, string $propertyNam
         $this->propertyName = $propertyName;
     }
 
-    public static function canHandleProperty(string $propertyName): bool
+    public static function canHandleProperty(ClassReflection $classReflection, string $propertyName): bool
     {
+        // @todo use the class reflection and be more specific about handled properties.
+        // Currently \PHPStan\Reflection\EntityFieldReflection::getType always passes FieldItemListInterface.
         $names = ['entity', 'value', 'target_id'];
         return in_array($propertyName, $names, true);
     }
 
     public function getType(): Type
     {
-        if ($this->propertyName == 'entity') {
-          // It was a EntityReferenceFieldItemList
-            return new ObjectType('Drupal\Core\Entity\FieldableEntityInterface');
+        if ($this->propertyName === 'entity') {
+            return new ObjectType('Drupal\Core\Entity\EntityInterface');
         }
-        if ($this->propertyName == 'target_id') {
-          // It was a EntityReferenceFieldItemList
-            return new IntegerType();
+        if ($this->propertyName === 'target_id') {
+            return new StringType();
         }
-        if ($this->propertyName == 'value') {
-            return new MixedType();
+        if ($this->propertyName === 'value') {
+            return new StringType();
         }
+
+        // Fallback.
+        return new NullType();
     }
 
     public function getDeclaringClass(): ClassReflection

tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php renamed to tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldReflection/EntityFieldMagicalGetters.php

@@ -1,11 +1,11 @@
 <?php
 
-namespace Drupal\phpstan_fixtures;
+namespace Drupal\phpstan_fixtures\EntityFieldReflection;
 
 use Drupal\entity_test\Entity\EntityTest;
 
-class EntityFieldFixture {
-    public function testMagicalFanciness() {
+class EntityFieldMagicalGetters {
+    public function testLabel() {
 
         /** @var EntityTest $testEntity */
         $testEntity = EntityTest::create([
@@ -15,9 +15,11 @@ public function testMagicalFanciness() {
 
         // 🤦‍♂
         $label1 = $testEntity->label();
+        // @todo Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.
         $label2 = $testEntity->get('name')->first()->value;
+        // @todo Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.
         $label3 = $testEntity->name->first()->value;
+        // This doesn't fail because of EntityFieldsViaMagicReflectionExtension
         $label4 = $testEntity->name->value;
-
     }
 }

tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldReflection/EntityFieldOriginalProperty.php

@@ -0,0 +1,23 @@
+<?php
+
+namespace Drupal\phpstan_fixtures\EntityFieldReflection;
+
+use Drupal\entity_test\Entity\EntityTest;
+
+class EntityFieldOriginalProperty {
+
+    public function testOriginal() {
+        /** @var EntityTest $testEntity */
+        $testEntity = EntityTest::create([
+            'name' => 'Llama',
+            'type' => 'entity_test',
+        ]);
+        if ($testEntity->getRevisionId() !== $testEntity->original->getRevisionId()) {
+            $testEntity->setSyncing(TRUE);
+        }
+
+        if (empty($testEntity->original) || $testEntity->getRevisionId() !== $testEntity->original->getRevisionId()) {
+            $testEntity->setSyncing(TRUE);
+        }
+    }
+}

tests/src/EntityTestClass.php

@@ -0,0 +1,35 @@
+<?php declare(strict_types=1);
+
+namespace PHPStan\Drupal;
+
+final class EntityTestClass extends AnalyzerTestBase
+{
+    /**
+     * @dataProvider dataEntitySamples
+     */
+    public function testEntityFields(string $path, int $count, array $errorMessages) {
+        $errors = $this->runAnalyze($path);
+        $this->assertCount($count, $errors, print_r($errors, true));
+        foreach ($errors as $key => $error) {
+            $this->assertEquals($errorMessages[$key], $error->getMessage());
+        }
+    }
+
+
+    public function dataEntitySamples(): \Generator
+    {
+        yield [
+            __DIR__ . '/../fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldReflection/EntityFieldMagicalGetters.php',
+            2,
+            [
+                'Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.',
+                'Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.',
+            ]
+        ];
+        yield [
+            __DIR__ . '/../fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldReflection/EntityFieldOriginalProperty.php',
+            0,
+            []
+        ];
+    }
+}