From 03d8469cfa3bafaec12e0fc96bccdf05bfdf5e29 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 22 Oct 2025 10:04:31 -0500 Subject: [PATCH 1/3] HookFormAlterRule for hook_form_alter fixes #920 --- bleedingEdge.neon | 1 + composer.json | 2 +- extension.neon | 2 + rules.neon | 4 + src/Rules/Drupal/HookFormAlterRule.php | 195 ++++++++++++++++++ tests/src/Rules/HookFormAlterRuleTest.php | 67 ++++++ .../Rules/data/hook-form-alter-invalid.php | 61 ++++++ .../src/Rules/data/hook-form-alter-valid.php | 45 ++++ 8 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 src/Rules/Drupal/HookFormAlterRule.php create mode 100644 tests/src/Rules/HookFormAlterRuleTest.php create mode 100644 tests/src/Rules/data/hook-form-alter-invalid.php create mode 100644 tests/src/Rules/data/hook-form-alter-valid.php 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..106cd18a --- /dev/null +++ b/tests/src/Rules/HookFormAlterRuleTest.php @@ -0,0 +1,67 @@ +analyse([ + __DIR__ . '/data/hook-form-alter-valid.php', + ], [ + // No errors expected for valid implementations + ]); + } + + /** + * Test invalid method-based hook implementations. + */ + public function testInvalidMethodImplementations(): void { + $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, + ], + ]); + } + + +} \ No newline at end of file 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 @@ + Date: Wed, 22 Oct 2025 11:34:41 -0500 Subject: [PATCH 2/3] drop some lint versions --- .github/workflows/php.yml | 2 -- 1 file changed, 2 deletions(-) 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: From c749e0366b073d741933bf7cb3ccc199eeea7725 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 22 Oct 2025 11:43:08 -0500 Subject: [PATCH 3/3] Skip HookFormAlterRule on < 11.1 --- tests/src/Rules/HookFormAlterRuleTest.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/src/Rules/HookFormAlterRuleTest.php b/tests/src/Rules/HookFormAlterRuleTest.php index 106cd18a..5cfcc608 100644 --- a/tests/src/Rules/HookFormAlterRuleTest.php +++ b/tests/src/Rules/HookFormAlterRuleTest.php @@ -2,6 +2,7 @@ namespace mglaman\PHPStanDrupal\Tests\Rules; +use Drupal; use mglaman\PHPStanDrupal\Rules\Drupal\HookFormAlterRule; use mglaman\PHPStanDrupal\Tests\DrupalRuleTestCase; use PHPStan\Rules\Rule; @@ -22,6 +23,9 @@ protected function getRule(): Rule { * Test valid form alter hook implementations. */ public function testValidImplementations(): void { + if (version_compare(Drupal::VERSION, '11.1', '<')) { + self::markTestSkipped(); + } $this->analyse([ __DIR__ . '/data/hook-form-alter-valid.php', ], [ @@ -33,6 +37,9 @@ public function testValidImplementations(): void { * 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', ], [ @@ -64,4 +71,4 @@ public function testInvalidMethodImplementations(): void { } -} \ No newline at end of file +}