diff --git a/README.md b/README.md index 0c793b2..13c7d67 100644 --- a/README.md +++ b/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. diff --git a/phpstan-test-app.neon b/phpstan-test-app.neon index c250b63..7c6c400 100644 --- a/phpstan-test-app.neon +++ b/phpstan-test-app.neon @@ -8,6 +8,3 @@ parameters: treatPhpDocTypesAsCertain: false cakeDC: disallowEntityArrayAccessRule: true - ignoreErrors: - - - identifier: missingType.generics diff --git a/phpstan.neon b/phpstan.neon index b6df14b..ce4beac 100644 --- a/phpstan.neon +++ b/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 diff --git a/rules.neon b/rules.neon index aaccac6..5170b9e 100644 --- a/rules.neon +++ b/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 - diff --git a/src/Rule/Model/EntityStrictPropertyAccessRule.php b/src/Rule/Model/EntityStrictPropertyAccessRule.php new file mode 100644 index 0000000..33df42c --- /dev/null +++ b/src/Rule/Model/EntityStrictPropertyAccessRule.php @@ -0,0 +1,108 @@ +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(), + ]; + } +} diff --git a/tests/TestCase/Rule/Model/EntityStrictPropertyAccessRuleTest.php b/tests/TestCase/Rule/Model/EntityStrictPropertyAccessRuleTest.php new file mode 100644 index 0000000..84e0274 --- /dev/null +++ b/tests/TestCase/Rule/Model/EntityStrictPropertyAccessRuleTest.php @@ -0,0 +1,82 @@ +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 + ], + ]); + } +} diff --git a/tests/TestCase/Rule/Model/Fake/BaseFailingUser.php b/tests/TestCase/Rule/Model/Fake/BaseFailingUser.php new file mode 100644 index 0000000..389faa1 --- /dev/null +++ b/tests/TestCase/Rule/Model/Fake/BaseFailingUser.php @@ -0,0 +1,11 @@ +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'; + } +} diff --git a/tests/test_app/Model/Entity/User.php b/tests/test_app/Model/Entity/User.php index a67d9d1..a3a7242 100644 --- a/tests/test_app/Model/Entity/User.php +++ b/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 + } + } } diff --git a/tests/test_app/Model/Table/NotesTable.php b/tests/test_app/Model/Table/NotesTable.php index b4065a6..0711b56 100644 --- a/tests/test_app/Model/Table/NotesTable.php +++ b/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 $query * @param string|int $year * @param bool $fun - * @return \Cake\ORM\Query\SelectQuery + * @return \Cake\ORM\Query\SelectQuery */ 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 $query * @param string $myType - * @return \Cake\ORM\Query\SelectQuery + * @return \Cake\ORM\Query\SelectQuery */ 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 $query * @param array $options - * @return \Cake\ORM\Query\SelectQuery + * @return \Cake\ORM\Query\SelectQuery */ 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 $query * @param mixed ...$options - * @return \Cake\ORM\Query\SelectQuery + * @return \Cake\ORM\Query\SelectQuery */ 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 $query * @param mixed ...$args - * @return \Cake\ORM\Query\SelectQuery + * @return \Cake\ORM\Query\SelectQuery */ public function findArgsPacked(SelectQuery $query, mixed ...$args): SelectQuery {