diff --git a/.semver b/.semver index 3b53786..24895fa 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- :major: 3 -:minor: 1 -:patch: 2 +:minor: 2 +:patch: 0 :special: '' diff --git a/README.md b/README.md index f074f7c..ed7e5c3 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,8 @@ Features included: 1. Provide correct return type for `Cake\ORM\Table::saveManyOrFail` based on the first argument passed 1. Provide correct return type for `Cake\ORM\Table::deleteMany` based on the first argument passed 1. Provide correct return type for `Cake\ORM\Table::deleteManyOrFail` based on the first argument passed - +1. Provide correct return type for `Cake\ORM\Locator\LocatorAwareTrait::fetchTable` based on the first argument passed +1. Provide correct return type for `Cake\Mailer\MailerAwareTrait::getMailer` based on the first argument passed
Examples: @@ -130,7 +131,16 @@ This rule check if association options are valid option types based on what each Table::hasMany, Table::belongsToMany, Table::hasOne and AssociationCollection::load. ### AddBehaviorExistsClassRule -This rule check if the target behavior has a valid table class when calling to Table::addBehavior and BehaviorRegistry::load. +This rule check if the target behavior has a valid class when calling to Table::addBehavior and BehaviorRegistry::load. + +### DisallowEntityArrayAccessRule +This rule disallow array access to entity in favor of object notation, is easier to detect a wrong property and to refactor code. + +### GetMailerExistsClassRule +This rule check if the target mailer is a valid class when calling to Cake\Mailer\MailerAwareTrait::getMailer. + +### LoadComponentExistsClassRule +This rule check if the target component has a valid class when calling to Controller::loadComponent and ComponentRegistry::load. ### OrmSelectQueryFindMatchOptionsTypesRule This rule check if the options (args) passed to Table::find and SelectQuery are valid find options types. @@ -138,6 +148,14 @@ This rule check if the options (args) passed to Table::find and SelectQuery are ### TableGetMatchOptionsTypesRule This rule check if the options (args) passed to Table::get are valid find options types. +To enable this rule update your phpstan.neon with: + +``` +parameters: + cakeDC: + disallowEntityArrayAccessRule: true +``` + ### How to disable a rule Each rule has a parameter in cakeDC 'namespace' to enable or disable, it is the same name of the rule with first letter in lowercase. diff --git a/phpstan.neon b/phpstan.neon index bd1cfeb..dbd4715 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -7,3 +7,5 @@ parameters: level: max checkGenericClassInNonGenericObjectType: false treatPhpDocTypesAsCertain: false + cakeDC: + disallowEntityArrayAccessRule: true diff --git a/rules.neon b/rules.neon index 7c0e970..30f5bba 100644 --- a/rules.neon +++ b/rules.neon @@ -5,7 +5,9 @@ parameters: addBehaviorExistsClassRule: true tableGetMatchOptionsTypesRule: true ormSelectQueryFindMatchOptionsTypesRule: true - + disallowEntityArrayAccessRule: false + getMailerExistsClassRule: true + loadComponentExistsClassRule: true parametersSchema: cakeDC: structure([ addAssociationExistsTableClassRule: anyOf(bool(), arrayOf(bool())) @@ -13,17 +15,26 @@ parametersSchema: addBehaviorExistsClassRule: anyOf(bool(), arrayOf(bool())) tableGetMatchOptionsTypesRule: anyOf(bool(), arrayOf(bool())) ormSelectQueryFindMatchOptionsTypesRule: anyOf(bool(), arrayOf(bool())) + disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool())) + getMailerExistsClassRule: anyOf(bool(), arrayOf(bool())) + loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool())) ]) conditionalTags: CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor: phpstan.parser.richParserNodeVisitor: %cakeDC.addAssociationExistsTableClassRule% + CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule: + phpstan.rules.rule: %cakeDC.loadComponentExistsClassRule% CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule: phpstan.rules.rule: %cakeDC.addAssociationExistsTableClassRule% CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule: phpstan.rules.rule: %cakeDC.addAssociationMatchOptionsTypesRule% CakeDC\PHPStan\Rule\Model\AddBehaviorExistsClassRule: phpstan.rules.rule: %cakeDC.addBehaviorExistsClassRule% + CakeDC\PHPStan\Rule\Model\DisallowEntityArrayAccessRule: + phpstan.rules.rule: %cakeDC.disallowEntityArrayAccessRule% + CakeDC\PHPStan\Rule\Mailer\GetMailerExistsClassRule: + phpstan.rules.rule: %cakeDC.getMailerExistsClassRule% CakeDC\PHPStan\Rule\Model\TableGetMatchOptionsTypesRule: phpstan.rules.rule: %cakeDC.tableGetMatchOptionsTypesRule% CakeDC\PHPStan\Rule\Model\OrmSelectQueryFindMatchOptionsTypesRule: @@ -32,12 +43,18 @@ conditionalTags: services: - class: CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor + - + class: CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule - class: CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule - class: CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule - class: CakeDC\PHPStan\Rule\Model\AddBehaviorExistsClassRule + - + class: CakeDC\PHPStan\Rule\Model\DisallowEntityArrayAccessRule + - + class: CakeDC\PHPStan\Rule\Mailer\GetMailerExistsClassRule - class: CakeDC\PHPStan\Rule\Model\TableGetMatchOptionsTypesRule - diff --git a/src/Rule/Controller/LoadComponentExistsClassRule.php b/src/Rule/Controller/LoadComponentExistsClassRule.php new file mode 100644 index 0000000..bfbc1a5 --- /dev/null +++ b/src/Rule/Controller/LoadComponentExistsClassRule.php @@ -0,0 +1,71 @@ + + */ + protected array $sourceMethods = [ + 'loadComponent', + ]; + + /** + * @var array + */ + protected array $componentRegistryMethods = [ + 'load', + ]; + + /** + * @inheritDoc + */ + protected function getTargetClassName(string $name): ?string + { + return CakeNameRegistry::getComponentClassName($name); + } + + /** + * @inheritDoc + */ + protected function getDetails(string $reference, array $args): ?array + { + if (str_ends_with($reference, 'Controller')) { + return [ + 'alias' => $args[0] ?? null, + 'options' => $args[1] ?? null, + 'sourceMethods' => $this->sourceMethods, + ]; + } + if ($reference === ComponentRegistry::class) { + return [ + 'alias' => $args[0] ?? null, + 'options' => $args[1] ?? null, + 'sourceMethods' => $this->componentRegistryMethods, + ]; + } + + return null; + } +} diff --git a/src/Rule/LoadObjectExistsCakeClassRule.php b/src/Rule/LoadObjectExistsCakeClassRule.php index 155c72f..0af86ec 100644 --- a/src/Rule/LoadObjectExistsCakeClassRule.php +++ b/src/Rule/LoadObjectExistsCakeClassRule.php @@ -16,6 +16,7 @@ use CakeDC\PHPStan\Rule\Traits\ParseClassNameFromArgTrait; use PhpParser\Node; use PhpParser\Node\Arg; +use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Scalar\String_; use PHPStan\Analyser\Scope; @@ -73,7 +74,7 @@ public function processNode(Node $node, Scope $scope): array $details['options'] ); } - if ($this->getTargetClassName($inputClassName)) { + if ($inputClassName === null || $this->getTargetClassName($inputClassName)) { return []; } @@ -92,12 +93,11 @@ public function processNode(Node $node, Scope $scope): array /** * @param \PhpParser\Node\Scalar\String_ $nameArg * @param \PhpParser\Node\Arg|null $options - * @return string + * @return string|null */ - protected function getInputClassName(String_ $nameArg, ?Arg $options): string + protected function getInputClassName(String_ $nameArg, ?Arg $options): ?string { $className = $nameArg->value; - if ( $options === null || !$options->value instanceof Node\Expr\Array_ @@ -112,10 +112,11 @@ protected function getInputClassName(String_ $nameArg, ?Arg $options): string ) { continue; } - $name = $this->parseClassNameFromExprTrait($item->value); - if ($name !== null) { - return $name; + if ($item->value instanceof ConstFetch && $item->value->name->toString() === 'null') { + return $className; } + + return $this->parseClassNameFromExprTrait($item->value); } return $className; diff --git a/src/Rule/Mailer/GetMailerExistsClassRule.php b/src/Rule/Mailer/GetMailerExistsClassRule.php new file mode 100644 index 0000000..c8aa957 --- /dev/null +++ b/src/Rule/Mailer/GetMailerExistsClassRule.php @@ -0,0 +1,84 @@ + + */ + public function processNode(Node $node, Scope $scope): array + { + assert($node instanceof MethodCall); + if ( + !$node->name instanceof Node\Identifier + || $node->name->name !== 'getMailer' + ) { + return []; + } + + $args = $node->getArgs(); + if (!isset($args[0])) { + return []; + } + $value = $args[0]->value; + if (!$value instanceof String_) { + return []; + } + $callerType = $scope->getType($node->var); + if (!$callerType instanceof ThisType) { + return []; + } + $reflection = $callerType->getClassReflection(); + + if (CakeNameRegistry::getMailerClassName($value->value)) { + return []; + } + + return [ + RuleErrorBuilder::message(sprintf( + 'Call to %s::%s could not find the class for "%s"', + $reflection->getName(), + $node->name->name, + $value->value, + )) + ->identifier($this->identifier) + ->build(), + ]; + } +} diff --git a/src/Rule/Model/DisallowEntityArrayAccessRule.php b/src/Rule/Model/DisallowEntityArrayAccessRule.php new file mode 100644 index 0000000..781fc89 --- /dev/null +++ b/src/Rule/Model/DisallowEntityArrayAccessRule.php @@ -0,0 +1,48 @@ + + * @throws \PHPStan\ShouldNotHappenException + * @throws \PHPStan\Reflection\MissingMethodFromReflectionException + */ + public function processNode(Node $node, Scope $scope): array + { + assert($node instanceof ArrayDimFetch); + $type = $scope->getType($node->var); + if (!$type instanceof ObjectType || !is_a($type->getClassName(), EntityInterface::class, true)) { + return []; + } + + return [ + RuleErrorBuilder::message(sprintf( + 'Array access to entity to %s is not allowed, access as object instead', + $type->getClassName(), + )) + ->identifier('cake.entity.arrayAccess') + ->build(), + ]; + } +} diff --git a/src/Traits/IsFromTargetTrait.php b/src/Traits/IsFromTargetTrait.php new file mode 100644 index 0000000..d10c308 --- /dev/null +++ b/src/Traits/IsFromTargetTrait.php @@ -0,0 +1,29 @@ +getTraits() as $trait) { + if ($trait->getName() === $targetTrait) { + return true; + } + } + foreach ($reflection->getParents() as $parent) { + if ($this->isFromTargetTrait($parent, $targetTrait)) { + return true; + } + } + + return false; + } +} diff --git a/src/Type/BaseTraitExpressionTypeResolverExtension.php b/src/Type/BaseTraitExpressionTypeResolverExtension.php index f2e6a8c..11ef13a 100644 --- a/src/Type/BaseTraitExpressionTypeResolverExtension.php +++ b/src/Type/BaseTraitExpressionTypeResolverExtension.php @@ -13,6 +13,7 @@ namespace CakeDC\PHPStan\Type; +use CakeDC\PHPStan\Traits\IsFromTargetTrait; use CakeDC\PHPStan\Utility\CakeNameRegistry; use PhpParser\Node\Expr; use PhpParser\Node\Identifier; @@ -27,6 +28,8 @@ class BaseTraitExpressionTypeResolverExtension implements ExpressionTypeResolverExtension { + use IsFromTargetTrait; + /** * TableLocatorDynamicReturnTypeExtension constructor. * @@ -63,7 +66,7 @@ public function getType(Expr $expr, Scope $scope): ?Type return null; } $reflection = $callerType->getClassReflection(); - if ($reflection === null || !$this->isFromTargetTrait($reflection)) { + if ($reflection === null || !$this->isFromTargetTrait($reflection, $this->targetTrait)) { return null; } @@ -80,26 +83,6 @@ public function getType(Expr $expr, Scope $scope): ?Type return null; } - /** - * @param \PHPStan\Reflection\ClassReflection $reflection - * @return bool - */ - protected function isFromTargetTrait(ClassReflection $reflection): bool - { - foreach ($reflection->getTraits() as $trait) { - if ($trait->getName() === $this->targetTrait) { - return true; - } - } - foreach ($reflection->getParents() as $parent) { - if ($this->isFromTargetTrait($parent)) { - return true; - } - } - - return false; - } - /** * @param \PhpParser\Node\Expr|null $value * @param \PHPStan\Reflection\ClassReflection $reflection diff --git a/src/Utility/CakeNameRegistry.php b/src/Utility/CakeNameRegistry.php index cf9e1a6..09b0810 100644 --- a/src/Utility/CakeNameRegistry.php +++ b/src/Utility/CakeNameRegistry.php @@ -44,6 +44,18 @@ public static function getClassName(string $baseName, string|array $namespaceFor return null; } + /** + * @param string $name + * @return string|null + */ + public static function getComponentClassName(string $name): ?string + { + return static::getClassName($name, [ + '%s\\Controller\\Component\\%sComponent', + '%s\\Controller\\Component\\%sComponent', + ]); + } + /** * @param string $name * @return string|null @@ -66,4 +78,15 @@ public static function getTableClassName(string $name): ?string '%s\\Model\\Table\\%sTable', ]); } + + /** + * @param string $name + * @return string|null + */ + public static function getMailerClassName(string $name): ?string + { + return static::getClassName($name, [ + '%s\\Mailer\\%sMailer', + ]); + } } diff --git a/tests/TestCase/Rule/Controller/Fake/FailingLoadComponentController.php b/tests/TestCase/Rule/Controller/Fake/FailingLoadComponentController.php new file mode 100644 index 0000000..3481d5a --- /dev/null +++ b/tests/TestCase/Rule/Controller/Fake/FailingLoadComponentController.php @@ -0,0 +1,32 @@ +loadComponent('Warning')->testWarning(); + $this->loadComponent('FormProtection'); + $this->loadComponent('CrazyWorld'); + $this->loadComponent('HelloWorld', [ + 'className' => null,//when null should ignore and try with name argument. Must cause error + ]); + $this->loadComponent('LegacyFlash', [ + 'className' => 'Flash',//Should use this instead of name argument. Not error + ]); + $this->loadComponent('Faker', [ + 'className' => 'CrayFaker',//Should use this instead of name argument. Must cause error + ]); + $this->loadComponent('AppFaker', [ + 'className' => 'Faker',//Should use this instead of name argument. Not error + ]); + } +} diff --git a/tests/TestCase/Rule/Controller/LoadComponentExistsClassRuleTest.php b/tests/TestCase/Rule/Controller/LoadComponentExistsClassRuleTest.php new file mode 100644 index 0000000..0f627d1 --- /dev/null +++ b/tests/TestCase/Rule/Controller/LoadComponentExistsClassRuleTest.php @@ -0,0 +1,53 @@ +analyse([__DIR__ . '/Fake/FailingLoadComponentController.php'], [ + [ + 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Controller\Fake\FailingLoadComponentController::loadComponent could not find the class for "CrazyWorld"', + 18, + ], + [ + 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Controller\Fake\FailingLoadComponentController::loadComponent could not find the class for "HelloWorld"', + 19, + ], + [ + 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Controller\Fake\FailingLoadComponentController::loadComponent could not find the class for "CrayFaker"', + 25, + ], + ]); + } +} diff --git a/tests/TestCase/Rule/Mailer/Fake/FailingGetMailerUsageLogic.php b/tests/TestCase/Rule/Mailer/Fake/FailingGetMailerUsageLogic.php new file mode 100644 index 0000000..c3b10fc --- /dev/null +++ b/tests/TestCase/Rule/Mailer/Fake/FailingGetMailerUsageLogic.php @@ -0,0 +1,21 @@ +getMailer('MyTestLoad')->testing();//This is okay + $this->getMailer('OldArticles')->testing();//This is NOT okay + $this->getMailer('SomeArticle')->send('published', ['userId' => 10, 'articleId' => 999]); + } +} diff --git a/tests/TestCase/Rule/Mailer/GetMailerExistsClassRuleTest.php b/tests/TestCase/Rule/Mailer/GetMailerExistsClassRuleTest.php new file mode 100644 index 0000000..1476324 --- /dev/null +++ b/tests/TestCase/Rule/Mailer/GetMailerExistsClassRuleTest.php @@ -0,0 +1,50 @@ +analyse([__DIR__ . '/Fake/FailingGetMailerUsageLogic.php'], [ + [ + 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Mailer\Fake\FailingGetMailerUsageLogic::getMailer could not find the class for "OldArticles"', + 18, // asserted error line + ], + [ + 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Mailer\Fake\FailingGetMailerUsageLogic::getMailer could not find the class for "SomeArticle"', + 19, // asserted error line + ], + ]); + } +} diff --git a/tests/TestCase/Rule/Model/AddAssociationExistsTableClassRuleTest.php b/tests/TestCase/Rule/Model/AddAssociationExistsTableClassRuleTest.php index dc02da8..8b24df0 100644 --- a/tests/TestCase/Rule/Model/AddAssociationExistsTableClassRuleTest.php +++ b/tests/TestCase/Rule/Model/AddAssociationExistsTableClassRuleTest.php @@ -11,8 +11,7 @@ * @license MIT License (http://www.opensource.org/licenses/mit-license.php) */ -namespace CakeDC\PHPStan\Test\TestCase\Type; -namespace CakeDC\PHPStan\Test\TestCase\Rule; +namespace CakeDC\PHPStan\Test\TestCase\Rule\Model; use CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule; use PHPStan\Rules\Rule; @@ -42,18 +41,10 @@ public function testRule(): void 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingRuleItemsTable::belongsTo could not find the class for "Fantasies"', 51, // asserted error line ], - [ - 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingRuleItemsTable::hasOne could not find the class for "ParentUsers"', - 120, - ], [ 'Call to Cake\ORM\AssociationCollection::load could not find the class for "CrazyUsers"', 139, ], - [ - 'Call to Cake\ORM\AssociationCollection::load could not find the class for "PalUsers"', - 148, - ], [ 'Call to CakeDC\PHPStan\Test\TestCase\Rule\Model\Fake\FailingRuleItemsTable::hasOne could not find the class for "Articles"', 187, diff --git a/tests/TestCase/Rule/Model/AddBehaviorExistsClassRuleTest.php b/tests/TestCase/Rule/Model/AddBehaviorExistsClassRuleTest.php index cb14310..23c87d8 100644 --- a/tests/TestCase/Rule/Model/AddBehaviorExistsClassRuleTest.php +++ b/tests/TestCase/Rule/Model/AddBehaviorExistsClassRuleTest.php @@ -11,8 +11,7 @@ * @license MIT License (http://www.opensource.org/licenses/mit-license.php) */ -namespace CakeDC\PHPStan\Test\TestCase\Type; -namespace CakeDC\PHPStan\Test\TestCase\Rule; +namespace CakeDC\PHPStan\Test\TestCase\Rule\Model; use CakeDC\PHPStan\Rule\Model\AddBehaviorExistsClassRule; use PHPStan\Rules\Rule; diff --git a/tests/TestCase/Rule/Model/DisallowEntityArrayAccessRuleTest.php b/tests/TestCase/Rule/Model/DisallowEntityArrayAccessRuleTest.php new file mode 100644 index 0000000..442355f --- /dev/null +++ b/tests/TestCase/Rule/Model/DisallowEntityArrayAccessRuleTest.php @@ -0,0 +1,70 @@ +analyse([__DIR__ . '/Fake/FailingEntityUseLogic.php'], [ + [ + 'Array access to entity to App\Model\Entity\Note is not allowed, access as object instead', + 22, // asserted error line + ], + [ + 'Array access to entity to App\Model\Entity\Note is not allowed, access as object instead', + 23, // asserted error line + ], + [ + 'Array access to entity to Cake\Datasource\EntityInterface is not allowed, access as object instead', + 28, + ], + [ + 'Array access to entity to App\Model\Entity\User is not allowed, access as object instead', + 30, // asserted error line + ], + [ + 'Array access to entity to App\Model\Entity\Note is not allowed, access as object instead', + 33, // asserted error line + ], + ]); + } + + /** + * @inheritDoc + */ + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../../../../extension.neon']; + } +} diff --git a/tests/TestCase/Rule/Model/Fake/FailingEntityUseLogic.php b/tests/TestCase/Rule/Model/Fake/FailingEntityUseLogic.php new file mode 100644 index 0000000..097b994 --- /dev/null +++ b/tests/TestCase/Rule/Model/Fake/FailingEntityUseLogic.php @@ -0,0 +1,40 @@ +fetchTable('Notes'); + $entity = $Table->get(1); + $note = $entity['note']; + $notable = 'Notable ' . $entity['note']; + $noted = $entity->note; + + //Unknown entity + $unknown = $this->fetchTable('UnknownRecords')->get(20); + $date = $unknown['create']; + $user = $this->fetchTable('Users')->get(10); + $user['role'] = 'Admin'; + + return [ + 'userId' => $entity['user_id'], + 'note' => $note, + 'noted' => $noted, + 'notable' => $notable, + 'date' => $date, + ]; + } +} diff --git a/tests/test_app/Controller/Component/FakerComponent.php b/tests/test_app/Controller/Component/FakerComponent.php new file mode 100644 index 0000000..e57b705 --- /dev/null +++ b/tests/test_app/Controller/Component/FakerComponent.php @@ -0,0 +1,33 @@ +getController(); + + return $controller->fetchTable('Users')->newEntity(['id' => 10]); + } +}