diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index b845f4a6..7c229f30 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -14,8 +14,6 @@ jobs: strategy: matrix: php-version: - - "8.1" - - "8.2" - "8.3" - "8.4" steps: diff --git a/bleedingEdge.neon b/bleedingEdge.neon index 9d0c6cfb..93448188 100644 --- a/bleedingEdge.neon +++ b/bleedingEdge.neon @@ -9,3 +9,4 @@ parameters: dependencySerializationTraitPropertyRule: true accessResultConditionRule: true cacheableDependencyRule: true + hookRules: true diff --git a/composer.json b/composer.json index 387ad2b7..324e41e4 100644 --- a/composer.json +++ b/composer.json @@ -20,7 +20,7 @@ "require-dev": { "behat/mink": "^1.10", "composer/installers": "^1.9 || ^2", - "drupal/core-recommended": "^10", + "drupal/core-recommended": "^11", "drush/drush": "^11 || ^12 || ^13", "phpstan/extension-installer": "^1.4.3", "phpstan/phpstan-strict-rules": "^2.0", diff --git a/extension.neon b/extension.neon index 6a2e15c4..098118de 100644 --- a/extension.neon +++ b/extension.neon @@ -34,6 +34,7 @@ parameters: classExtendsInternalClassRule: true pluginManagerInspectionRule: false cacheableDependencyRule: false + hookRules: false entityMapping: aggregator_feed: class: Drupal\aggregator\Entity\Feed @@ -269,6 +270,7 @@ parametersSchema: classExtendsInternalClassRule: boolean() pluginManagerInspectionRule: boolean() cacheableDependencyRule: boolean() + hookRules: boolean() ]) entityMapping: arrayOf(anyOf( structure([ diff --git a/rules.neon b/rules.neon index d435567a..695a7fbf 100644 --- a/rules.neon +++ b/rules.neon @@ -14,6 +14,8 @@ rules: - mglaman\PHPStanDrupal\Rules\Drupal\TestClassesProtectedPropertyModulesRule conditionalTags: + mglaman\PHPStanDrupal\Rules\Drupal\HookFormAlterRule: + phpstan.rules.rule: %drupal.rules.hookRules% mglaman\PHPStanDrupal\Rules\Drupal\Tests\TestClassSuffixNameRule: phpstan.rules.rule: %drupal.rules.testClassSuffixNameRule% mglaman\PHPStanDrupal\Rules\Drupal\DependencySerializationTraitPropertyRule: @@ -28,6 +30,8 @@ conditionalTags: phpstan.rules.rule: %drupal.rules.cacheableDependencyRule% services: + - + class: mglaman\PHPStanDrupal\Rules\Drupal\HookFormAlterRule - class: mglaman\PHPStanDrupal\Rules\Drupal\Tests\TestClassSuffixNameRule - diff --git a/src/Rules/Drupal/HookFormAlterRule.php b/src/Rules/Drupal/HookFormAlterRule.php new file mode 100644 index 00000000..3bdd50f9 --- /dev/null +++ b/src/Rules/Drupal/HookFormAlterRule.php @@ -0,0 +1,195 @@ + + */ +class HookFormAlterRule implements Rule +{ + + /** + * Pattern matching form alter hook names. + */ + private const FORM_ALTER_PATTERNS = [ + '/^form_alter$/', + '/^form_[a-zA-Z0-9_]+_alter$/', + ]; + + + public function getNodeType(): string + { + return ClassMethod::class; + } + + public function processNode(Node $node, Scope $scope): array + { + // Skip if Hook attribute class doesn't exist (older Drupal versions) + if (!class_exists(Hook::class)) { + return []; + } + + return $this->processMethod($node, $scope); + } + + /** + * Process a method with Hook attribute. + * + * @return list<\PHPStan\Rules\IdentifierRuleError> + */ + private function processMethod(ClassMethod $method, Scope $scope): array + { + if (!$scope->isInClass()) { + return []; + } + + $classReflection = $scope->getClassReflection(); + $nativeClass = $classReflection->getNativeReflection(); + + if (!$nativeClass->hasMethod($method->name->toString())) { + return []; + } + + $nativeMethod = $nativeClass->getMethod($method->name->toString()); + $hookAttributes = $nativeMethod->getAttributes(Hook::class); + + if (count($hookAttributes) === 0) { + return []; + } + + $errors = []; + foreach ($hookAttributes as $hookAttribute) { + /** @var Hook $hookInstance */ + $hookInstance = $hookAttribute->newInstance(); + if ($this->isFormAlterHook($hookInstance->hook)) { + $methodErrors = $this->validateMethodSignature($method, $scope, $hookInstance->hook); + $errors[] = $methodErrors; + } + } + + return array_merge(...$errors); + } + + + /** + * Check if a hook name matches form alter patterns. + */ + private function isFormAlterHook(string $hookName): bool + { + foreach (self::FORM_ALTER_PATTERNS as $pattern) { + if (preg_match($pattern, $hookName) === 1) { + return true; + } + } + return false; + } + + /** + * Validate method signature for method-targeted hooks. + * + * @return list<\PHPStan\Rules\IdentifierRuleError> + */ + private function validateMethodSignature(ClassMethod $method, Scope $scope, string $hookName): array + { + $expectedSignature = 'Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])'; + $paramCount = count($method->params); + + if ($paramCount < 2 || $paramCount > 3) { + return [ + RuleErrorBuilder::message( + sprintf('Form alter hook "%s" implementation must have 2 or 3 parameters. %s', $hookName, $expectedSignature) + ) + ->line($method->getStartLine()) + ->identifier('hookFormAlter.invalidParameterCount') + ->build() + ]; + } + + $errors = []; + + // Validate first parameter (form array by reference) + $formParam = $method->params[0]; + if (!$formParam->byRef) { + $errors[] = RuleErrorBuilder::message( + sprintf('Form alter hook "%s" first parameter must be passed by reference (&$form). %s', $hookName, $expectedSignature) + ) + ->line($method->getStartLine()) + ->identifier('hookFormAlter.formParameterNotByRef') + ->build(); + } + + // Validate parameter types if type hints are present + if ($formParam->type !== null) { + $formParamType = $scope->getFunctionType($formParam->type, false, false); + if (!$formParamType->isArray()->yes()) { + $errors[] = RuleErrorBuilder::message( + sprintf('Form alter hook "%s" first parameter should be an array. %s', $hookName, $expectedSignature) + ) + ->line($method->getStartLine()) + ->identifier('hookFormAlter.invalidFormParameterType') + ->build(); + } + } + + // Validate second parameter (FormStateInterface) + $formStateParam = $method->params[1]; + if ($formStateParam->type !== null) { + $formStateType = $scope->getFunctionType($formStateParam->type, false, false); + $expectedFormStateType = new ObjectType(FormStateInterface::class); + if (!$expectedFormStateType->isSuperTypeOf($formStateType)->yes()) { + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Form alter hook "%s" second parameter should be \Drupal\Core\Form\FormStateInterface, %s given. %s', + $hookName, + $formStateType->describe(VerbosityLevel::typeOnly()), + $expectedSignature + ) + ) + ->line($method->getStartLine()) + ->identifier('hookFormAlter.invalidFormStateParameterType') + ->build(); + } + } + + // Validate third parameter (string form_id) - only if present + if ($paramCount === 3) { + $formIdParam = $method->params[2]; + if ($formIdParam->type !== null) { + $formIdType = $scope->getFunctionType($formIdParam->type, false, false); + if (!$formIdType->isString()->yes()) { + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Form alter hook "%s" third parameter should be string, %s given. %s', + $hookName, + $formIdType->describe(VerbosityLevel::typeOnly()), + $expectedSignature + ) + ) + ->line($method->getStartLine()) + ->identifier('hookFormAlter.invalidFormIdParameterType') + ->build(); + } + } + } + + return $errors; + } +} diff --git a/tests/src/Rules/HookFormAlterRuleTest.php b/tests/src/Rules/HookFormAlterRuleTest.php new file mode 100644 index 00000000..5cfcc608 --- /dev/null +++ b/tests/src/Rules/HookFormAlterRuleTest.php @@ -0,0 +1,74 @@ +analyse([ + __DIR__ . '/data/hook-form-alter-valid.php', + ], [ + // No errors expected for valid implementations + ]); + } + + /** + * Test invalid method-based hook implementations. + */ + public function testInvalidMethodImplementations(): void { + if (version_compare(Drupal::VERSION, '11.1', '<')) { + self::markTestSkipped(); + } + $this->analyse([ + __DIR__ . '/data/hook-form-alter-invalid.php', + ], [ + [ + 'Form alter hook "form_alter" implementation must have 2 or 3 parameters. Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 16, + ], + [ + 'Form alter hook "form_alter" first parameter must be passed by reference (&$form). Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 24, + ], + [ + 'Form alter hook "form_alter" second parameter should be \Drupal\Core\Form\FormStateInterface, string given. Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 32, + ], + [ + 'Form alter hook "form_node_form_alter" third parameter should be string, int given. Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 40, + ], + [ + 'Form alter hook "form_alter" implementation must have 2 or 3 parameters. Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 48, + ], + [ + 'Form alter hook "form_alter" first parameter should be an array. Expected signature: method(&$form, \Drupal\Core\Form\FormStateInterface $form_state[, $form_id])', + 56, + ], + ]); + } + + +} diff --git a/tests/src/Rules/data/hook-form-alter-invalid.php b/tests/src/Rules/data/hook-form-alter-invalid.php new file mode 100644 index 00000000..d71f6561 --- /dev/null +++ b/tests/src/Rules/data/hook-form-alter-invalid.php @@ -0,0 +1,61 @@ +