Feature/entity property access rule

README.md

@@ -176,6 +176,9 @@ Methods covered:
 ### DisallowEntityArrayAccessRule
 This rule disallow array access to entity in favor of object notation, is easier to detect a wrong property and to refactor code.
 
+### EntityStrictPropertyAccessRule
+This rule check incorrect assigment of native entity properties to avoid issues with dirty tracking.
+
 ### GetMailerExistsClassRule
 This rule check if the target mailer is a valid class when calling to Cake\Mailer\MailerAwareTrait::getMailer.
 

phpstan-test-app.neon

@@ -8,6 +8,3 @@ parameters:
     treatPhpDocTypesAsCertain: false
     cakeDC:
         disallowEntityArrayAccessRule: true
-    ignoreErrors:
-        -
-            identifier: missingType.generics

phpstan.neon

@@ -13,3 +13,8 @@ parameters:
     ignoreErrors:
         -
             identifier: missingType.generics
+        -
+            message: '#^Although PHPStan\\Reflection\\Php\\PhpMethodFromParserNodeReflection is covered by backward compatibility promise, this instanceof assumption might break because it''s not guaranteed to always stay the same\.$#'
+            identifier: phpstanApi.instanceofAssumption
+            count: 1
+            path: src/Rule/Model/EntityStrictPropertyAccessRule.php

rules.neon

@@ -7,6 +7,7 @@ parameters:
 	 	tableGetMatchOptionsTypesRule: true
 	 	ormSelectQueryFindMatchOptionsTypesRule: true
 	 	disallowEntityArrayAccessRule: true
+	 	entityStrictPropertyAccessRule: true
 	 	getMailerExistsClassRule: true
 	 	loadComponentExistsClassRule: true
 	 	controllerMethodMustBeUsedRule: true
@@ -21,6 +22,7 @@ parametersSchema:
 		tableGetMatchOptionsTypesRule: anyOf(bool(), arrayOf(bool()))
 		ormSelectQueryFindMatchOptionsTypesRule: anyOf(bool(), arrayOf(bool()))
 		disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool()))
+		entityStrictPropertyAccessRule: anyOf(bool(), arrayOf(bool()))
 		getMailerExistsClassRule: anyOf(bool(), arrayOf(bool()))
 		loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool()))
 		controllerMethodMustBeUsedRule: anyOf(bool(), arrayOf(bool()))
@@ -43,6 +45,8 @@ conditionalTags:
 		phpstan.rules.rule: %cakeDC.addBehaviorExistsClassRule%
 	CakeDC\PHPStan\Rule\Model\DisallowEntityArrayAccessRule:
 		phpstan.rules.rule: %cakeDC.disallowEntityArrayAccessRule%
+	CakeDC\PHPStan\Rule\Model\EntityStrictPropertyAccessRule:
+		phpstan.rules.rule: %cakeDC.entityStrictPropertyAccessRule%
 	CakeDC\PHPStan\Rule\Mailer\GetMailerExistsClassRule:
 		phpstan.rules.rule: %cakeDC.getMailerExistsClassRule%
 	CakeDC\PHPStan\Rule\Model\TableGetMatchOptionsTypesRule:
@@ -69,6 +73,8 @@ services:
 		class: CakeDC\PHPStan\Rule\Model\AddBehaviorExistsClassRule
 	-
 		class: CakeDC\PHPStan\Rule\Model\DisallowEntityArrayAccessRule
+	-
+		class: CakeDC\PHPStan\Rule\Model\EntityStrictPropertyAccessRule
 	-
 		class: CakeDC\PHPStan\Rule\Mailer\GetMailerExistsClassRule
 	-

src/Rule/Model/EntityStrictPropertyAccessRule.php

@@ -0,0 +1,108 @@
+<?php
+declare(strict_types=1);
+
+namespace CakeDC\PHPStan\Rule\Model;
+
+use Cake\ORM\Entity;
+use PhpParser\Node;
+use PhpParser\Node\Expr\Assign;
+use PhpParser\Node\Expr\PropertyFetch;
+use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Identifier;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\Printer\ExprPrinter;
+use PHPStan\Reflection\ClassReflection;
+use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection;
+use PHPStan\Reflection\ReflectionProvider;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+
+class EntityStrictPropertyAccessRule implements Rule
+{
+    /**
+     * @var string
+     */
+    protected string $identifier = 'cake.entity.strictPropertyAccess';
+    /**
+     * @var \PHPStan\Reflection\ClassReflection
+     */
+    protected ClassReflection $entityBaseReflection;
+
+    /**
+     * @param \PHPStan\Node\Printer\ExprPrinter $printer
+     */
+    public function __construct(
+        private ExprPrinter $printer,
+        private ReflectionProvider $reflectionProvider,
+    ) {
+        $this->entityBaseReflection = $this->reflectionProvider->getClass(Entity::class);
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function getNodeType(): string
+    {
+        return Assign::class;
+    }
+
+    /**
+     * @param \PhpParser\Node $node
+     * @param \PHPStan\Analyser\Scope $scope
+     * @return list<\PHPStan\Rules\IdentifierRuleError>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        assert($node instanceof Assign);
+
+        if (!$node->var instanceof PropertyFetch) {
+            return [];
+        }
+        if (
+            !$node->var->var instanceof Variable
+            || $node->var->var->name !== 'this'
+        ) {
+            return [];
+        }
+        $classReflection = $scope->getClassReflection();
+        if ($classReflection === null) {
+            return [];
+        }
+        if (
+            !$classReflection->isSubclassOfClass($this->entityBaseReflection)
+            && $classReflection->getName() !== Entity::class
+        ) {
+            return [];
+        }
+        if (!$node->var->name instanceof Identifier) {
+            return [];
+        }
+        $propertyName = $node->var->name->toString();
+        //Ignore properties from \Cake\ORM\Entity
+        if ($this->entityBaseReflection->hasNativeProperty($propertyName)) {
+            return [];
+        }
+        if (!$classReflection->hasNativeProperty($propertyName)) {
+            return [];
+        }
+        $currentFunction = $scope->getFunction();
+        if (
+            $currentFunction instanceof PhpMethodFromParserNodeReflection
+            && $currentFunction->getHookedPropertyName() === $propertyName
+        ) {
+            return [];
+        }
+
+        return [
+            RuleErrorBuilder::message(sprintf(
+                'Modifying property "%s" in entity class "%s" is not allowed. Use $this->set(\'%s\', %s) for proper dirty tracking.',// phpcs:ignore
+                $propertyName,
+                $classReflection->getName(),
+                $propertyName,
+                $this->printer->printExpr($node->expr),
+            ))
+                ->identifier($this->identifier)
+                ->build(),
+        ];
+    }
+}

tests/TestCase/Rule/Model/EntityStrictPropertyAccessRuleTest.php

@@ -0,0 +1,82 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
+ *
+ * Licensed under The MIT License
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
+ * @license   MIT License (http://www.opensource.org/licenses/mit-license.php)
+ */
+
+namespace CakeDC\PHPStan\Test\TestCase\Rule\Model;
+
+use CakeDC\PHPStan\Rule\Model\EntityStrictPropertyAccessRule;
+use PHPStan\Node\Printer\ExprPrinter;
+use PHPStan\Reflection\ReflectionProvider;
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+
+class EntityStrictPropertyAccessRuleTest extends RuleTestCase
+{
+    /**
+     * @return \PHPStan\Rules\Rule
+     */
+    protected function getRule(): Rule
+    {
+        $printer = $this->getContainer()->getByType(ExprPrinter::class);
+        $reflectionProvider = $this->getContainer()->getByType(ReflectionProvider::class);
+        // getRule() method needs to return an instance of the tested rule
+        return new EntityStrictPropertyAccessRule($printer, $reflectionProvider);
+    }
+
+    /**
+     * @return void
+     */
+    public function testRule(): void
+    {
+        // first argument: path to the example file that contains some errors that should be reported by MyRule
+        // second argument: an array of expected errors,
+        // each error consists of the asserted error message, and the asserted error file line
+        $this->analyse([__DIR__ . '/Fake/FailingUser.php'], [
+            [
+                'Modifying property "first_name" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'first_name\', \'John\') for proper dirty tracking.',
+                33, // asserted error line
+            ],
+            [
+                'Modifying property "last_name" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'last_name\', \'Doe\') for proper dirty tracking.',
+                34, // asserted error line
+            ],
+            [
+                'Modifying property "password" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'password\', \'0000000\') for proper dirty tracking.',
+                35, // asserted error line
+            ],
+            [
+                'Modifying property "is_active" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'is_active\', true) for proper dirty tracking.',
+                36, // asserted error line
+            ],
+            [
+                'Modifying property "id" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'id\', 100) for proper dirty tracking.',
+                37, // asserted error line
+            ],
+            [
+                'Modifying property "rate" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'rate\', 9.5) for proper dirty tracking.',
+                38, // asserted error line
+            ],
+            [
+                'Modifying property "first_name" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'first_name\', $name) for proper dirty tracking.',
+                41, // asserted error line
+            ],
+            [
+                'Modifying property "password" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'password\', static::FIELD_ID) for proper dirty tracking.',
+                42, // asserted error line
+            ],
+            [
+                'Modifying property "role" in entity class "CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingUser" is not allowed. Use $this->set(\'role\', \'user\') for proper dirty tracking.',
+                45, // asserted error line
+            ],
+        ]);
+    }
+}

tests/TestCase/Rule/Model/Fake/BaseFailingUser.php

@@ -0,0 +1,11 @@
+<?php
+declare(strict_types=1);
+
+namespace CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake;
+
+use Cake\ORM\Entity;
+
+class BaseFailingUser extends Entity
+{
+    public protected(set) ?string $role;
+}

tests/TestCase/Rule/Model/Fake/FailingUser.php

@@ -0,0 +1,47 @@
+<?php
+declare(strict_types=1);
+
+namespace CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake;
+
+class FailingUser extends BaseFailingUser
+{
+    public const string FIELD_ID = 'id';
+
+    public protected(set) int $id;
+    public protected(set) float $rate;
+    public protected(set) ?string $email;
+    public protected(set) bool $is_active = false;
+    public protected(set) ?string $first_name;
+    public protected(set) ?string $last_name;
+
+    public protected(set) ?string $password {//phpcs:ignore
+        set(?string $value) {//phpcs:ignore
+            $this->password = $value ? password_hash($value, PASSWORD_DEFAULT) : null;//phpcs:ignore
+        }
+    }
+
+    public string $full_name {//phpcs:ignore
+        get => $this->first_name . ' ' . $this->last_name;//phpcs:ignore
+    }
+
+    /**
+     * @return void
+     */
+    public function fail()
+    {
+        //Error, properties should be set using $this>set method
+        $this->first_name = 'John';
+        $this->last_name = 'Doe';
+        $this->password = '0000000';
+        $this->is_active = true;
+        $this->id = 100;
+        $this->rate = 9.5;
+        $this['is_active'] = true;//This is okay
+        $name = 'James';
+        $this->first_name = $name;
+        $this->password = static::FIELD_ID;
+        $this->hidden = ['password'];//Allowed to change from base entity
+        $this->new = false;
+        $this->role = 'user';
+    }
+}

tests/test_app/Model/Entity/User.php

@@ -15,4 +15,12 @@
  */
 class User extends Entity
 {
+    /**
+     * @var string|null
+     */
+    public protected(set) ?string $password {//phpcs:ignore
+        set {
+            $this->password = $value ? password_hash($value, PASSWORD_DEFAULT) : null;//phpcs:ignore
+        }
+    }
 }

tests/test_app/Model/Table/NotesTable.php

@@ -83,10 +83,11 @@ public function warning(): array
     }
 
     /**
-     * @param \Cake\ORM\Query\SelectQuery $query
+     * @template TSubject of \Cake\Datasource\EntityInterface|array
+     * @param \Cake\ORM\Query\SelectQuery<TSubject> $query
      * @param string|int $year
      * @param bool $fun
-     * @return \Cake\ORM\Query\SelectQuery
+     * @return \Cake\ORM\Query\SelectQuery<TSubject>
      */
     public function findFeatured(SelectQuery $query, string|int $year, bool $fun): SelectQuery
     {
@@ -105,9 +106,10 @@ public function findFeatured(SelectQuery $query, string|int $year, bool $fun): S
     }
 
     /**
-     * @param \Cake\ORM\Query\SelectQuery $query
+     * @template TSubject of \Cake\Datasource\EntityInterface|array
+     * @param \Cake\ORM\Query\SelectQuery<TSubject> $query
      * @param string $myType
-     * @return \Cake\ORM\Query\SelectQuery
+     * @return \Cake\ORM\Query\SelectQuery<TSubject>
      */
     public function findTwoArgsButNotLegacy(SelectQuery $query, string $myType): SelectQuery
     {
@@ -121,9 +123,10 @@ public function findTwoArgsButNotLegacy(SelectQuery $query, string $myType): Sel
     }
 
     /**
-     * @param \Cake\ORM\Query\SelectQuery $query
+     * @template TSubject of \Cake\Datasource\EntityInterface|array
+     * @param \Cake\ORM\Query\SelectQuery<TSubject> $query
      * @param array<string, mixed> $options
-     * @return \Cake\ORM\Query\SelectQuery
+     * @return \Cake\ORM\Query\SelectQuery<TSubject>
      */
     public function findLegacy(SelectQuery $query, array $options): SelectQuery
     {
@@ -135,9 +138,10 @@ public function findLegacy(SelectQuery $query, array $options): SelectQuery
     }
 
     /**
-     * @param \Cake\ORM\Query\SelectQuery $query
+     * @template TSubject of \Cake\Datasource\EntityInterface|array
+     * @param \Cake\ORM\Query\SelectQuery<TSubject> $query
      * @param mixed ...$options
-     * @return \Cake\ORM\Query\SelectQuery
+     * @return \Cake\ORM\Query\SelectQuery<TSubject>
      */
     public function findOptionsPacked(SelectQuery $query, mixed ...$options): SelectQuery
     {
@@ -147,9 +151,10 @@ public function findOptionsPacked(SelectQuery $query, mixed ...$options): Select
     }
 
     /**
-     * @param \Cake\ORM\Query\SelectQuery $query
+     * @template TSubject of \Cake\Datasource\EntityInterface|array
+     * @param \Cake\ORM\Query\SelectQuery<TSubject> $query
      * @param mixed ...$args
-     * @return \Cake\ORM\Query\SelectQuery
+     * @return \Cake\ORM\Query\SelectQuery<TSubject>
      */
     public function findArgsPacked(SelectQuery $query, mixed ...$args): SelectQuery
     {