From ad87307fd3b0625c7cba8655766cce8c0c62a0c6 Mon Sep 17 00:00:00 2001 From: Hugo Alliaume Date: Tue, 16 Dec 2025 00:48:46 +0100 Subject: [PATCH] Homogenize codebase --- AGENTS.md | 48 ++++++++++++++++++- .../LiveActionMethodsShouldBePublicRule.php | 2 +- .../LiveListenerMethodsShouldBePublicRule.php | 2 +- ...ClassNameShouldNotEndWithComponentRule.php | 2 +- .../ExposePublicPropsShouldBeFalseRule.php | 3 ++ .../ForbiddenAttributesPropertyRule.php | 2 + .../MethodsShouldBePublicOrPrivateRule.php | 14 +++--- .../PublicPropertiesShouldBeCamelCaseRule.php | 8 +++- ...iveActionMethodsShouldBePublicRuleTest.php | 4 +- ...eListenerMethodsShouldBePublicRuleTest.php | 4 +- ...sNameShouldNotEndWithComponentRuleTest.php | 4 +- ...MethodsShouldBePublicOrPrivateRuleTest.php | 10 ++-- ...licPropertiesShouldBeCamelCaseRuleTest.php | 10 ++-- 13 files changed, 84 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ca77d54..64421f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -210,11 +210,55 @@ This command will: ### Other available commands Check the `composer.json` file to see all available commands. +## Code Style Conventions + +### Variable Naming + +- **Reflection variables**: Use `$reflClass` for `ClassReflection` objects (not `$classReflection`) +- **Method reflection**: Use `$reflMethod` for method reflections +- **Property reflection**: Use descriptive names like `$propertyRefl` +- **Attribute nodes**: Use `$attribute` for single attributes, `$livePropAttribute` for specific ones +- **Other nodes**: Use descriptive names like `$method`, `$property`, `$node` + +### PHPDoc Comments + +- **Public methods**: Always include `@implements Rule` or similar on rule classes +- **Private methods**: Always add a brief description comment explaining the method's purpose +- **Return types**: Document complex return types with `@return` (e.g., `@return array{name: string, custom: bool}|null`) +- **Parameters**: Document array parameters with `@param string[]` or similar when applicable + +### Error Messages + +- **Modal verbs**: Use "must" for requirements (e.g., "Method must be public"), not "should" +- **Consistency**: Be consistent with message structure across similar rules +- **Tips**: Always provide actionable tips with `->tip()` to guide users toward solutions +- **Formatting**: Use double quotes for strings in sprintf, backticks in markdown for code + +### Code Structure + +- **Early returns**: Check for attribute presence first, return empty array if not found +- **Null checks**: Check for null/undefined values before proceeding +- **Error collection**: Use `$errors = []` array and collect all errors before returning +- **Method order**: + 1. `getNodeType()` + 2. `processNode()` + 3. Private helper methods (alphabetically sorted if multiple) + +### Validation Order in `processNode()` + +1. Check if node has required attribute (return `[]` if not) +2. Check if `namespacedName` exists (when needed) +3. Initialize error array: `$errors = []` +4. Get reflection class if needed: `$reflClass = $this->reflectionProvider->getClass(...)` +5. Iterate over relevant elements (methods, properties, etc.) +6. Perform validations and collect errors +7. Return `$errors` + ## Best Practices 1. **Naming**: Rules should have a descriptive name and end with `Rule` -2. **Identifiers**: Use the format `symfonyUX.twigComponent.descriptiveName` for error identifiers -3. **Clear messages**: Error messages should be explicit and include a `tip()` with a suggestion +2. **Identifiers**: Use the format `symfonyUX.twigComponent.descriptiveName` or `symfonyUX.liveComponent.descriptiveName` for error identifiers +3. **Clear messages**: Error messages must be explicit and include a `tip()` with a suggestion 4. **Complete tests**: Always test valid cases, invalid cases, and non-components 5. **Documentation**: Document each rule in the README with concrete examples 6. **Validation**: Always run `symfony composer qa-fix` before committing diff --git a/src/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule.php b/src/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule.php index be2e5c4..c5332d3 100644 --- a/src/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule.php +++ b/src/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule.php @@ -40,7 +40,7 @@ public function processNode(Node $node, Scope $scope): array $methodName = $method->name->toString(); $errors[] = RuleErrorBuilder::message( - sprintf('LiveAction method "%s()" should be public.', $methodName) + sprintf('LiveAction method "%s()" must be public.', $methodName) ) ->identifier('symfonyUX.liveComponent.liveActionMethodsShouldBePublic') ->line($method->getLine()) diff --git a/src/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule.php b/src/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule.php index d5cdffd..39c8866 100644 --- a/src/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule.php +++ b/src/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule.php @@ -38,7 +38,7 @@ public function processNode(Node $node, Scope $scope): array if (! $method->isPublic()) { $errors[] = RuleErrorBuilder::message(sprintf( - 'LiveListener method "%s()" should be public.', + 'LiveListener method "%s()" must be public.', $method->name->toString() )) ->identifier('symfonyUX.liveComponent.liveListenerMethodShouldBePublic') diff --git a/src/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule.php b/src/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule.php index a496b10..6c2a8a5 100644 --- a/src/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule.php +++ b/src/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule.php @@ -37,7 +37,7 @@ public function processNode(Node $node, Scope $scope): array $classNameString = $className->toString(); if (str_ends_with($classNameString, 'Component')) { return [ - RuleErrorBuilder::message(sprintf('Twig component class "%s" should not end with "Component".', $classNameString)) + RuleErrorBuilder::message(sprintf('Twig component class "%s" must not end with "Component".', $classNameString)) ->identifier('symfonyUX.twigComponent.classNameShouldNotEndWithComponent') ->line($className->getLine()) ->tip('Remove the "Component" suffix from the class name.') diff --git a/src/Rules/TwigComponent/ExposePublicPropsShouldBeFalseRule.php b/src/Rules/TwigComponent/ExposePublicPropsShouldBeFalseRule.php index 61f668f..2b8dedc 100644 --- a/src/Rules/TwigComponent/ExposePublicPropsShouldBeFalseRule.php +++ b/src/Rules/TwigComponent/ExposePublicPropsShouldBeFalseRule.php @@ -50,6 +50,9 @@ public function processNode(Node $node, Scope $scope): array return []; } + /** + * Extract the exposePublicProps value from the component attribute. + */ private function getExposePublicPropsValue(Node\Attribute $attribute): ?bool { foreach ($attribute->args as $arg) { diff --git a/src/Rules/TwigComponent/ForbiddenAttributesPropertyRule.php b/src/Rules/TwigComponent/ForbiddenAttributesPropertyRule.php index 1590a27..2b24710 100644 --- a/src/Rules/TwigComponent/ForbiddenAttributesPropertyRule.php +++ b/src/Rules/TwigComponent/ForbiddenAttributesPropertyRule.php @@ -68,6 +68,8 @@ public function processNode(Node $node, Scope $scope): array } /** + * Extract the attributes variable name from the component attribute. + * * @return array{name: string, custom: bool}|null */ private function getAttributesVarName(Node\Attribute $attribute): ?array diff --git a/src/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule.php b/src/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule.php index 9d6811f..04b9c00 100644 --- a/src/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule.php +++ b/src/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule.php @@ -46,8 +46,8 @@ public function processNode(Node $node, Scope $scope): array } $errors = []; - $classReflection = $this->reflectionProvider->getClass($className); - $abstractTraitMethods = $this->getAbstractTraitMethods($classReflection); + $reflClass = $this->reflectionProvider->getClass($className); + $abstractTraitMethods = $this->getAbstractTraitMethods($reflClass); foreach ($node->getMethods() as $method) { if (! $method->isProtected()) { @@ -62,7 +62,7 @@ public function processNode(Node $node, Scope $scope): array } $errors[] = RuleErrorBuilder::message( - sprintf('Method "%s()" in a Twig component should not be protected.', $methodName) + sprintf('Method "%s()" in a Twig component must not be protected.', $methodName) ) ->identifier('symfonyUX.twigComponent.methodsShouldBePublicOrPrivate') ->line($method->getLine()) @@ -77,7 +77,7 @@ public function processNode(Node $node, Scope $scope): array } $reportedTraitMethods = []; - foreach ($classReflection->getTraits() as $traitReflection) { + foreach ($reflClass->getTraits() as $traitReflection) { // Use native reflection to get trait methods foreach ($traitReflection->getNativeReflection()->getMethods() as $traitMethod) { if (! $traitMethod->isProtected()) { @@ -111,7 +111,7 @@ public function processNode(Node $node, Scope $scope): array } $errors[] = RuleErrorBuilder::message( - sprintf('Method "%s()" in a Twig component should not be protected.', $methodName) + sprintf('Method "%s()" in a Twig component must not be protected.', $methodName) ) ->identifier('symfonyUX.twigComponent.methodsShouldBePublicOrPrivate') ->line($lineNumber) @@ -128,11 +128,11 @@ public function processNode(Node $node, Scope $scope): array * * @return array */ - private function getAbstractTraitMethods(ClassReflection $classReflection): array + private function getAbstractTraitMethods(ClassReflection $reflClass): array { $abstractTraitMethods = []; - foreach ($classReflection->getTraits() as $traitReflection) { + foreach ($reflClass->getTraits() as $traitReflection) { foreach ($traitReflection->getNativeReflection()->getMethods() as $method) { if ($method->isAbstract()) { $abstractTraitMethods[$method->getName()] = true; diff --git a/src/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule.php b/src/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule.php index 105b196..1b1e2b5 100644 --- a/src/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule.php +++ b/src/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule.php @@ -41,7 +41,7 @@ public function processNode(Node $node, Scope $scope): array if (! $this->isCamelCase($propertyName)) { $errors[] = RuleErrorBuilder::message( - sprintf('Public property "%s" in a Twig component should be in camelCase.', $propertyName) + sprintf('Public property "%s" in a Twig component must be in camelCase.', $propertyName) ) ->identifier('symfonyUX.twigComponent.publicPropertiesShouldBeCamelCase') ->line($property->getLine()) @@ -54,12 +54,18 @@ public function processNode(Node $node, Scope $scope): array return $errors; } + /** + * Check if a property name is in camelCase format. + */ private function isCamelCase(string $name): bool { // Property should start with a lowercase letter and contain no underscores return preg_match('/^[a-z][a-zA-Z0-9]*$/', $name) === 1; } + /** + * Convert a string to camelCase format. + */ private function toCamelCase(string $name): string { // Convert snake_case or PascalCase to camelCase diff --git a/tests/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule/LiveActionMethodsShouldBePublicRuleTest.php b/tests/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule/LiveActionMethodsShouldBePublicRuleTest.php index 9fcbdf0..7ec307d 100644 --- a/tests/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule/LiveActionMethodsShouldBePublicRuleTest.php +++ b/tests/Rules/LiveComponent/LiveActionMethodsShouldBePublicRule/LiveActionMethodsShouldBePublicRuleTest.php @@ -19,7 +19,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/LiveComponentWithPrivateLiveAction.php'], [ [ - 'LiveAction method "save()" should be public.', + 'LiveAction method "save()" must be public.', 15, 'Methods annotated with #[LiveAction] must be public to be accessible as component actions.', ], @@ -30,7 +30,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/LiveComponentWithProtectedLiveAction.php'], [ [ - 'LiveAction method "delete()" should be public.', + 'LiveAction method "delete()" must be public.', 15, 'Methods annotated with #[LiveAction] must be public to be accessible as component actions.', ], diff --git a/tests/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule/LiveListenerMethodsShouldBePublicRuleTest.php b/tests/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule/LiveListenerMethodsShouldBePublicRuleTest.php index 8f368af..fdcc0b5 100644 --- a/tests/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule/LiveListenerMethodsShouldBePublicRuleTest.php +++ b/tests/Rules/LiveComponent/LiveListenerMethodsShouldBePublicRule/LiveListenerMethodsShouldBePublicRuleTest.php @@ -19,12 +19,12 @@ public function testViolations(): void [__DIR__ . '/Fixture/PrivateLiveListener.php'], [ [ - 'LiveListener method "onAnotherEvent()" should be public.', + 'LiveListener method "onAnotherEvent()" must be public.', 13, 'Change the method visibility to public.', ], [ - 'LiveListener method "onSomeEvent()" should be public.', + 'LiveListener method "onSomeEvent()" must be public.', 18, 'Change the method visibility to public.', ], diff --git a/tests/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule/ClassNameShouldNotEndWithComponentRuleTest.php b/tests/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule/ClassNameShouldNotEndWithComponentRuleTest.php index 249fee2..95c0b32 100644 --- a/tests/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule/ClassNameShouldNotEndWithComponentRuleTest.php +++ b/tests/Rules/TwigComponent/ClassNameShouldNotEndWithComponentRule/ClassNameShouldNotEndWithComponentRuleTest.php @@ -19,7 +19,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/AlertComponent.php'], [ [ - 'Twig component class "AlertComponent" should not end with "Component".', + 'Twig component class "AlertComponent" must not end with "Component".', 10, 'Remove the "Component" suffix from the class name.', ], @@ -30,7 +30,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/CounterComponent.php'], [ [ - 'Twig component class "CounterComponent" should not end with "Component".', + 'Twig component class "CounterComponent" must not end with "Component".', 10, 'Remove the "Component" suffix from the class name.', ], diff --git a/tests/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule/MethodsShouldBePublicOrPrivateRuleTest.php b/tests/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule/MethodsShouldBePublicOrPrivateRuleTest.php index 6b508d8..bb9aa63 100644 --- a/tests/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule/MethodsShouldBePublicOrPrivateRuleTest.php +++ b/tests/Rules/TwigComponent/MethodsShouldBePublicOrPrivateRule/MethodsShouldBePublicOrPrivateRuleTest.php @@ -19,7 +19,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/ComponentWithProtectedMethod.php'], [ [ - 'Method "protectedMethod()" in a Twig component should not be protected.', + 'Method "protectedMethod()" in a Twig component must not be protected.', 14, 'Twig component methods should be either public or private, not protected.', ], @@ -30,7 +30,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/LiveComponentWithProtectedMethod.php'], [ [ - 'Method "protectedMethod()" in a Twig component should not be protected.', + 'Method "protectedMethod()" in a Twig component must not be protected.', 14, 'Twig component methods should be either public or private, not protected.', ], @@ -42,7 +42,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/ComponentWithProtectedConcreteMethodFromTrait.php'], [ [ - 'Method "protectedConcreteTraitMethod()" in a Twig component should not be protected.', + 'Method "protectedConcreteTraitMethod()" in a Twig component must not be protected.', 12, 'Twig component methods should be either public or private, not protected.', ], @@ -53,7 +53,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/LiveComponentWithProtectedConcreteMethodFromTrait.php'], [ [ - 'Method "protectedConcreteTraitMethod()" in a Twig component should not be protected.', + 'Method "protectedConcreteTraitMethod()" in a Twig component must not be protected.', 12, 'Twig component methods should be either public or private, not protected.', ], @@ -65,7 +65,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/ComponentWithDuplicateTraitMethods.php'], [ [ - 'Method "duplicateConcreteMethod()" in a Twig component should not be protected.', + 'Method "duplicateConcreteMethod()" in a Twig component must not be protected.', 12, 'Twig component methods should be either public or private, not protected.', ], diff --git a/tests/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule/PublicPropertiesShouldBeCamelCaseRuleTest.php b/tests/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule/PublicPropertiesShouldBeCamelCaseRuleTest.php index 6d49be0..c76aaaf 100644 --- a/tests/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule/PublicPropertiesShouldBeCamelCaseRuleTest.php +++ b/tests/Rules/TwigComponent/PublicPropertiesShouldBeCamelCaseRule/PublicPropertiesShouldBeCamelCaseRuleTest.php @@ -19,12 +19,12 @@ public function testViolations(): void [__DIR__ . '/Fixture/ComponentWithSnakeCaseProperty.php'], [ [ - 'Public property "user_name" in a Twig component should be in camelCase.', + 'Public property "user_name" in a Twig component must be in camelCase.', 12, 'Consider renaming "user_name" to "userName".', ], [ - 'Public property "is_active" in a Twig component should be in camelCase.', + 'Public property "is_active" in a Twig component must be in camelCase.', 14, 'Consider renaming "is_active" to "isActive".', ], @@ -35,7 +35,7 @@ public function testViolations(): void [__DIR__ . '/Fixture/ComponentWithPascalCaseProperty.php'], [ [ - 'Public property "UserName" in a Twig component should be in camelCase.', + 'Public property "UserName" in a Twig component must be in camelCase.', 12, 'Consider renaming "UserName" to "userName".', ], @@ -46,12 +46,12 @@ public function testViolations(): void [__DIR__ . '/Fixture/LiveComponentWithSnakeCaseProperty.php'], [ [ - 'Public property "user_name" in a Twig component should be in camelCase.', + 'Public property "user_name" in a Twig component must be in camelCase.', 12, 'Consider renaming "user_name" to "userName".', ], [ - 'Public property "is_active" in a Twig component should be in camelCase.', + 'Public property "is_active" in a Twig component must be in camelCase.', 14, 'Consider renaming "is_active" to "isActive".', ],