diff --git a/README.md b/README.md index c6bedbc..afde10f 100644 --- a/README.md +++ b/README.md @@ -93,6 +93,7 @@ parameters: | Rule | Description | Target | |------|-------------|---------| +| **[BooleanArgumentFlag](docs/BooleanArgumentFlag.md)** | Detects boolean parameters in functions and methods that may indicate multiple responsibilities | Methods, Functions, Closures | | **[LongVariable](docs/LongVariable.md)** | Limits variable name length | Variables | | **[MissingClosureParameterTypehint](docs/MissingClosureParameterTypehint.md)** | Requires type hints on closure parameters | Closures | | **[ShortMethodName](docs/ShortMethodName.md)** | Enforces minimum method name length | Methods | diff --git a/config/extension.neon b/config/extension.neon index c5ade24..9033734 100644 --- a/config/extension.neon +++ b/config/extension.neon @@ -60,6 +60,10 @@ parametersSchema: number_of_children: structure([ maximum: int(), ]), + boolean_argument_flag: structure([ + ignored_in_classes: arrayOf(string()), + ignore_pattern: string(), + ]), ]) # default parameters @@ -112,6 +116,9 @@ parameters: elseif_allowed: true number_of_children: maximum: 15 + boolean_argument_flag: + ignored_in_classes: [] + ignore_pattern: '' services: - @@ -190,4 +197,9 @@ services: - factory: Orrison\MeliorStan\Rules\NumberOfChildren\Config arguments: - - %meliorstan.number_of_children.maximum% \ No newline at end of file + - %meliorstan.number_of_children.maximum% + - + factory: Orrison\MeliorStan\Rules\BooleanArgumentFlag\Config + arguments: + - %meliorstan.boolean_argument_flag.ignored_in_classes% + - %meliorstan.boolean_argument_flag.ignore_pattern% \ No newline at end of file diff --git a/docs/BooleanArgumentFlag.md b/docs/BooleanArgumentFlag.md new file mode 100644 index 0000000..9533dcf --- /dev/null +++ b/docs/BooleanArgumentFlag.md @@ -0,0 +1,283 @@ +# BooleanArgumentFlag + +Boolean parameters in functions and methods are often indicators that a function is doing more than one thing, violating the Single Responsibility Principle. + +This rule detects boolean parameters in class methods, static methods, named functions, and closures. When a function or method accepts a boolean flag to control its behavior, it typically means the function has at least two different execution paths, suggesting it should be split into separate functions. + +## Configuration + +This rule supports the following configuration options: + +### `ignored_in_classes` +- **Type**: `array` +- **Default**: `[]` +- **Description**: A list of fully-qualified class names where boolean parameters in methods are allowed. Only class methods are affected by this configuration; functions and closures are not ignored even when defined within ignored classes. + +```php +// Example: 'App\\Service\\ConfigurationService' +``` + +### `ignore_pattern` +- **Type**: `string` +- **Default**: `''` +- **Description**: A regular expression pattern to match function or method names that should be ignored. This applies to class methods and named functions. Closures are not affected by this pattern. Common patterns include setter methods or boolean query methods. + +```php +// Example: '/^set/' to ignore all setter methods +// Example: '/^(is|has|should)/' to ignore boolean query methods +``` + +## Usage + +Add the rule to your PHPStan configuration: + +```neon +includes: + - vendor/orrison/meliorstan/config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: [] + ignore_pattern: '' +``` + +## Examples + +### Default Configuration + +```php +activateUser($user); + } else { + $this->deactivateUser($user); + } + } + + // ✗ Error: Method "UserService::save()" has boolean parameter "$validate" which may indicate the method has multiple responsibilities. + public function save(User $user, bool $validate) + { + } + + // ✗ Error: Method "UserService::handleNullable()" has boolean parameter "$option" which may indicate the method has multiple responsibilities. + public function handleNullable(?bool $option) + { + } + + // ✗ Error: Method "UserService::unionType()" has boolean parameter "$flag" which may indicate the method has multiple responsibilities. + public function unionType(bool|null $flag) + { + } + + // ✓ Valid: No boolean parameters + public function getUser(int $id): User + { + } +} + +// ✗ Error: Function "processData()" has boolean parameter "$flag" which may indicate the function has multiple responsibilities. +function processData(array $data, bool $flag) +{ +} + +// ✗ Error: Closure has boolean parameter "$enabled" which may indicate the closure has multiple responsibilities. +$handler = function (string $name, bool $enabled) { +}; +``` + +### Configuration Examples + +#### Ignored Classes + +Useful for specific classes where boolean flags are acceptable (e.g., configuration builders, option classes). + +```neon +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: + - 'App\Service\ConfigurationBuilder' + - 'App\Options\UserOptions' + ignore_pattern: '' +``` + +```php + + */ +class BooleanArgumentFlagRule implements Rule +{ + public const ERROR_MESSAGE_TEMPLATE_METHOD = 'Method "%s::%s()" has boolean parameter "$%s" which may indicate the method has multiple responsibilities.'; + + public const ERROR_MESSAGE_TEMPLATE_FUNCTION = 'Function "%s()" has boolean parameter "$%s" which may indicate the function has multiple responsibilities.'; + + public const ERROR_MESSAGE_TEMPLATE_CLOSURE = 'Closure has boolean parameter "$%s" which may indicate the closure has multiple responsibilities.'; + + public function __construct( + protected Config $config, + ) {} + + public function getNodeType(): string + { + return FunctionLike::class; + } + + public function processNode(Node $node, Scope $scope): array + { + // Check if this is a class method and if the class should be ignored + if ($node instanceof ClassMethod && $this->shouldIgnoreByClass($scope)) { + return []; + } + + // Get the function/method name for pattern matching + $functionName = $this->getFunctionName($node); + + if ($functionName !== null && $this->shouldIgnoreByPattern($functionName)) { + return []; + } + + $errors = []; + + // Check each parameter for boolean types + foreach ($node->getParams() as $param) { + if ($this->isBooleanType($param->type)) { + $paramName = $param->var instanceof Node\Expr\Variable && is_string($param->var->name) + ? $param->var->name + : 'unknown'; + + $errors[] = RuleErrorBuilder::message($this->buildErrorMessage($node, $scope, $paramName)) + ->identifier('MeliorStan.booleanArgumentFlag') + ->build(); + } + } + + return $errors; + } + + protected function shouldIgnoreByClass(Scope $scope): bool + { + $classReflection = $scope->getClassReflection(); + + if ($classReflection === null) { + return false; + } + + $className = $classReflection->getName(); + $ignoredClasses = $this->config->getIgnoredInClasses(); + + return in_array($className, $ignoredClasses, true); + } + + protected function shouldIgnoreByPattern(string $functionName): bool + { + $pattern = $this->config->getIgnorePattern(); + + if ($pattern === '') { + return false; + } + + $result = @preg_match($pattern, $functionName); + + // Check for both false return value and error codes + if ($result === false || preg_last_error() !== PREG_NO_ERROR) { + $error = preg_last_error_msg(); + + throw new InvalidArgumentException( + sprintf('Invalid regex pattern in ignore_pattern configuration: "%s". Error: %s', $pattern, $error) + ); + } + + return $result === 1; + } + + protected function getFunctionName(FunctionLike $node): ?string + { + if ($node instanceof ClassMethod || $node instanceof Function_) { + return $node->name->toString(); + } + + // Closures don't have names + return null; + } + + protected function isBooleanType(?Node $type): bool + { + if ($type === null) { + return false; + } + + // Direct bool identifier + if ($type instanceof Identifier) { + return $type->toString() === 'bool'; + } + + // Nullable type: ?bool + if ($type instanceof NullableType) { + return $this->isBooleanType($type->type); + } + + // Union types: bool|null, int|bool, etc. + if ($type instanceof UnionType) { + foreach ($type->types as $unionType) { + if ($this->isBooleanType($unionType)) { + return true; + } + } + } + + return false; + } + + protected function buildErrorMessage(FunctionLike $node, Scope $scope, string $paramName): string + { + if ($node instanceof ClassMethod) { + $classReflection = $scope->getClassReflection(); + $className = $classReflection !== null ? $classReflection->getName() : 'Unknown'; + $methodName = $node->name->toString(); + + return sprintf(self::ERROR_MESSAGE_TEMPLATE_METHOD, $className, $methodName, $paramName); + } + + if ($node instanceof Function_) { + $functionName = $node->name->toString(); + + return sprintf(self::ERROR_MESSAGE_TEMPLATE_FUNCTION, $functionName, $paramName); + } + + // Closure + return sprintf(self::ERROR_MESSAGE_TEMPLATE_CLOSURE, $paramName); + } +} diff --git a/src/Rules/BooleanArgumentFlag/Config.php b/src/Rules/BooleanArgumentFlag/Config.php new file mode 100644 index 0000000..fbb35e9 --- /dev/null +++ b/src/Rules/BooleanArgumentFlag/Config.php @@ -0,0 +1,28 @@ + $ignoredInClasses List of fully-qualified class names in which boolean argument flag checks should be ignored. + * @param string $ignorePattern Regular expression pattern for method names to ignore. + */ + public function __construct( + protected array $ignoredInClasses, + protected string $ignorePattern, + ) {} + + /** + * @return array + */ + public function getIgnoredInClasses(): array + { + return $this->ignoredInClasses; + } + + public function getIgnorePattern(): string + { + return $this->ignorePattern; + } +} diff --git a/tests/Rules/BooleanArgumentFlag/AllOptionsTrueTest.php b/tests/Rules/BooleanArgumentFlag/AllOptionsTrueTest.php new file mode 100644 index 0000000..17eb49b --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/AllOptionsTrueTest.php @@ -0,0 +1,34 @@ + + */ +class AllOptionsTrueTest extends RuleTestCase +{ + public function testAllOptions(): void + { + $this->analyse([ + __DIR__ . '/Fixture/AllOptions.php', + ], [ + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\AllOptionsNotIgnored', 'handle', 'flag'), 18], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\AllOptionsNotIgnored', 'processData', 'flag'), 20], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_FUNCTION, 'processConfig', 'value'), 25], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/config/all-options.neon']; + } + + protected function getRule(): Rule + { + return self::getContainer()->getByType(BooleanArgumentFlagRule::class); + } +} diff --git a/tests/Rules/BooleanArgumentFlag/DefaultOptionsTest.php b/tests/Rules/BooleanArgumentFlag/DefaultOptionsTest.php new file mode 100644 index 0000000..f90f7bb --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/DefaultOptionsTest.php @@ -0,0 +1,49 @@ + + */ +class DefaultOptionsTest extends RuleTestCase +{ + public function testDefaultExample(): void + { + $this->analyse([ + __DIR__ . '/Fixture/DefaultExample.php', + ], [ + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', '__construct', 'debug'), 7], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'processWithFlag', 'flag'), 9], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'handleNullable', 'option'), 11], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'unionType', 'value'), 13], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'multiUnion', 'mixed'), 15], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'staticMethod', 'enabled'), 19], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', '__set', 'value'), 21], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'multipleParams', 'enabled'), 23], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'multipleBools', 'first'), 25], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'multipleBools', 'second'), 25], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'protectedMethod', 'flag'), 27], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\DefaultExample', 'privateMethod', 'flag'), 29], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_FUNCTION, 'namedFunction', 'flag'), 32], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'flag'), 36], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'flag'), 40], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'enabled'), 44], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'outer'), 46], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'inner'), 47], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/config/default.neon']; + } + + protected function getRule(): Rule + { + return self::getContainer()->getByType(BooleanArgumentFlagRule::class); + } +} diff --git a/tests/Rules/BooleanArgumentFlag/Fixture/AllOptions.php b/tests/Rules/BooleanArgumentFlag/Fixture/AllOptions.php new file mode 100644 index 0000000..5bf68e9 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/Fixture/AllOptions.php @@ -0,0 +1,25 @@ + $flag; + +$validArrow = fn (string $name) => $name; + +$closureMultipleParams = function (string $name, bool $enabled) {}; + +$nestedClosure = function (bool $outer) { + return function (bool $inner) { + return $inner; + }; +}; diff --git a/tests/Rules/BooleanArgumentFlag/Fixture/IgnorePattern.php b/tests/Rules/BooleanArgumentFlag/Fixture/IgnorePattern.php new file mode 100644 index 0000000..57e6571 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/Fixture/IgnorePattern.php @@ -0,0 +1,28 @@ + $value; + +$processorClosure = fn (bool $value) => $value; diff --git a/tests/Rules/BooleanArgumentFlag/Fixture/IgnoredClasses.php b/tests/Rules/BooleanArgumentFlag/Fixture/IgnoredClasses.php new file mode 100644 index 0000000..fdb7dfc --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/Fixture/IgnoredClasses.php @@ -0,0 +1,23 @@ + + */ +class IgnorePatternTest extends RuleTestCase +{ + public function testIgnorePattern(): void + { + $this->analyse([ + __DIR__ . '/Fixture/IgnorePattern.php', + ], [ + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\IgnorePatternExample', 'processWithFlag', 'flag'), 13], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\IgnorePatternExample', 'handleBool', 'value'), 15], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_FUNCTION, 'processGlobal', 'value'), 24], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'value'), 26], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'value'), 28], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/config/ignore-pattern.neon']; + } + + protected function getRule(): Rule + { + return self::getContainer()->getByType(BooleanArgumentFlagRule::class); + } +} diff --git a/tests/Rules/BooleanArgumentFlag/IgnoredInClassesTest.php b/tests/Rules/BooleanArgumentFlag/IgnoredInClassesTest.php new file mode 100644 index 0000000..704d28d --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/IgnoredInClassesTest.php @@ -0,0 +1,34 @@ + + */ +class IgnoredInClassesTest extends RuleTestCase +{ + public function testIgnoredClasses(): void + { + $this->analyse([ + __DIR__ . '/Fixture/IgnoredClasses.php', + ], [ + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_METHOD, 'Fixtures\BooleanArgumentFlag\NotIgnoredClass', 'methodWithBool', 'flag'), 16], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_FUNCTION, 'functionWithBool', 'flag'), 21], + [sprintf(BooleanArgumentFlagRule::ERROR_MESSAGE_TEMPLATE_CLOSURE, 'flag'), 23], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/config/ignored-classes.neon']; + } + + protected function getRule(): Rule + { + return self::getContainer()->getByType(BooleanArgumentFlagRule::class); + } +} diff --git a/tests/Rules/BooleanArgumentFlag/InvalidPatternTest.php b/tests/Rules/BooleanArgumentFlag/InvalidPatternTest.php new file mode 100644 index 0000000..8d873c9 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/InvalidPatternTest.php @@ -0,0 +1,34 @@ + + */ +class InvalidPatternTest extends RuleTestCase +{ + public function testInvalidPattern(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid regex pattern in ignore_pattern configuration'); + + $this->analyse([ + __DIR__ . '/Fixture/IgnorePattern.php', + ], []); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/config/invalid-pattern.neon']; + } + + protected function getRule(): Rule + { + return self::getContainer()->getByType(BooleanArgumentFlagRule::class); + } +} diff --git a/tests/Rules/BooleanArgumentFlag/config/all-options.neon b/tests/Rules/BooleanArgumentFlag/config/all-options.neon new file mode 100644 index 0000000..697ea56 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/config/all-options.neon @@ -0,0 +1,12 @@ +includes: + - ../../../../config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: + - 'Fixtures\BooleanArgumentFlag\AllOptionsIgnoredClass' + ignore_pattern: '/^set/' diff --git a/tests/Rules/BooleanArgumentFlag/config/default.neon b/tests/Rules/BooleanArgumentFlag/config/default.neon new file mode 100644 index 0000000..8c9ee36 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/config/default.neon @@ -0,0 +1,11 @@ +includes: + - ../../../../config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: [] + ignore_pattern: '' diff --git a/tests/Rules/BooleanArgumentFlag/config/ignore-pattern.neon b/tests/Rules/BooleanArgumentFlag/config/ignore-pattern.neon new file mode 100644 index 0000000..81d4016 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/config/ignore-pattern.neon @@ -0,0 +1,11 @@ +includes: + - ../../../../config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: [] + ignore_pattern: '/^set/' diff --git a/tests/Rules/BooleanArgumentFlag/config/ignored-classes.neon b/tests/Rules/BooleanArgumentFlag/config/ignored-classes.neon new file mode 100644 index 0000000..c465573 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/config/ignored-classes.neon @@ -0,0 +1,12 @@ +includes: + - ../../../../config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: + - 'Fixtures\BooleanArgumentFlag\IgnoredClass' + ignore_pattern: '' diff --git a/tests/Rules/BooleanArgumentFlag/config/invalid-pattern.neon b/tests/Rules/BooleanArgumentFlag/config/invalid-pattern.neon new file mode 100644 index 0000000..04d9a48 --- /dev/null +++ b/tests/Rules/BooleanArgumentFlag/config/invalid-pattern.neon @@ -0,0 +1,11 @@ +includes: + - ../../../../config/extension.neon + +rules: + - Orrison\MeliorStan\Rules\BooleanArgumentFlag\BooleanArgumentFlagRule + +parameters: + meliorstan: + boolean_argument_flag: + ignored_in_classes: [] + ignore_pattern: '/invalid[regex' # Invalid regex - missing closing bracket