From dfc23c39390b40a53a8443351929a64b06aabfb2 Mon Sep 17 00:00:00 2001 From: Gael Robin Date: Thu, 21 Nov 2024 08:12:41 +0100 Subject: [PATCH 1/4] Rule to check if method of subscribed events exists --- composer.json | 2 + phpstan.neon.dist | 6 + ...allbackMethodsExistsInSubscribedEvents.php | 108 ++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php diff --git a/composer.json b/composer.json index 5c474f3135..e7e645afca 100644 --- a/composer.json +++ b/composer.json @@ -234,6 +234,8 @@ "@test-integration-pressidium" ], "run-stan": "vendor/bin/phpstan analyze --memory-limit=2G --no-progress", + "run-stan-reset-baseline": "vendor/bin/phpstan analyze --memory-limit=2G --no-progress --generate-baseline", + "run-stan-test": "vendor/bin/phpstan analyze inc/Engine/Common/ExtractCSS/Subscriber.php --memory-limit=2G --no-progress --debug", "install-codestandards": "Dealerdirect\\Composer\\Plugin\\Installers\\PHPCodeSniffer\\Plugin::run", "phpcs": "phpcs --basepath=.", "phpcs-changed": "./bin/phpcs-changed.sh", diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 02e47b7399..81e8ec3e95 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -1,8 +1,11 @@ includes: - phar://phpstan.phar/conf/bleedingEdge.neon +# - phpstan-baseline.neon + parameters: level: 4 inferPrivatePropertyTypeFromConstructor: true +# reportUnmatchedIgnoredErrors: false paths: # Test only the new architecture for now. - %currentWorkingDirectory%/inc/Engine/ @@ -10,6 +13,7 @@ parameters: - %currentWorkingDirectory%/inc/ThirdParty/ - %currentWorkingDirectory%/tests/Integration/ - %currentWorkingDirectory%/tests/Unit/ + - %currentWorkingDirectory%/tests/phpstan/Rules/ bootstrapFiles: # Must be first - %currentWorkingDirectory%/inc/functions/options.php @@ -51,3 +55,5 @@ parameters: # These need plugin stubs! - %currentWorkingDirectory%/inc/classes/subscriber/third-party/ - %currentWorkingDirectory%/inc/3rd-party/ +rules: + - WP_Rocket\Tests\phpstan\Rules\EnsureCallbackMethodsExistsInSubscribedEvents diff --git a/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php b/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php new file mode 100644 index 0000000000..45d964b95a --- /dev/null +++ b/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php @@ -0,0 +1,108 @@ +expr) { + + // Get the function/method name from the scope + $functionName = $scope->getFunctionName(); + if ('get_subscribed_events' === $functionName) { + // Check if the return expression is an array + if ($node->expr instanceof Node\Expr\Array_) { + foreach ($node->expr->items as $item) { + if ($item instanceof Node\Expr\ArrayItem) { + // Ensure the key exists and is a valid type (like a string) before accessing the value + $hookName = null; + if ($item->key instanceof Node\Scalar\String_) { + $hookName = $item->key->value; + } + + // Analyze the value of the array item + $methodValue = $item->value; + $errors = array_merge($errors, $this->analyzeMethodValue($methodValue, $scope)); + } + } + } + } + } + + return $errors; // Return all collected errors + } + + /** + * Analyze the method value from the array structure. + */ + private function analyzeMethodValue(Node $methodValue, Scope $scope): array + { + $errors = []; + + if ($methodValue instanceof Node\Scalar\String_) { + // Simple structure: array('hook_name' => 'method_name') + $errors = $this->checkIfMethodExistsInClass($methodValue->value, $scope, $methodValue); + } elseif ($methodValue instanceof Node\Expr\Array_) { + // More complex structures: array or nested array + foreach ($methodValue->items as $subItem) { + if ($subItem instanceof Node\Expr\ArrayItem) { + if ($subItem->value instanceof Node\Scalar\String_) { + $errors = array_merge($errors, $this->checkIfMethodExistsInClass($subItem->value->value, $scope, $subItem->value)); + } + } + } + } + + return $errors; + } + + public function checkIfMethodExistsInClass(string $methodName, Scope $scope, Node $node): array + { + $classReflection = $scope->getClassReflection(); + + // Check if the class reflection is available and the method exists + if ($classReflection && $classReflection->hasMethod($methodName)) { + return []; + } + + // Check if the method exists in the parent class or interfaces + $parentClass = $classReflection ? $classReflection->getParentClass() : null; + if ($parentClass && $parentClass->hasMethod($methodName)) { + return []; + } + + foreach ($classReflection->getInterfaces() as $interface) { + if ($interface->hasMethod($methodName)) { + return []; + } + } + + // If the method doesn't exist, return an error using RuleErrorBuilder + $errorMessage = sprintf( + "The callback function '%s' declared within 'get_subscribed_events' does not exist in the class '%s'.", + $methodName, + $classReflection ? $classReflection->getName() : 'unknown' + ); + + return [ + RuleErrorBuilder::message($errorMessage) + ->line($node->getLine()) // Add the line number + ->build() + ]; + } +} From 6da4cc4091092673d6495dc18e5de587fab8f1ad Mon Sep 17 00:00:00 2001 From: COQUARD Cyrille Date: Fri, 22 Nov 2024 07:44:02 +0100 Subject: [PATCH 2/4] Added a test to validate the logic --- ...ackMethodsExistsInSubscribedEventsTest.php | 45 +++++++++++++++++++ tests/phpstan/tests/bootstrap.php | 3 ++ .../complex-syntax.php | 38 ++++++++++++++++ .../not-existing.php | 42 +++++++++++++++++ .../valid.php | 42 +++++++++++++++++ tests/phpstan/tests/phpunit.xml | 27 +++++++++++ 6 files changed, 197 insertions(+) create mode 100644 tests/phpstan/tests/Rules/EnsureCallbackMethodsExistsInSubscribedEventsTest.php create mode 100644 tests/phpstan/tests/bootstrap.php create mode 100644 tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/complex-syntax.php create mode 100644 tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/not-existing.php create mode 100644 tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/valid.php create mode 100644 tests/phpstan/tests/phpunit.xml diff --git a/tests/phpstan/tests/Rules/EnsureCallbackMethodsExistsInSubscribedEventsTest.php b/tests/phpstan/tests/Rules/EnsureCallbackMethodsExistsInSubscribedEventsTest.php new file mode 100644 index 0000000000..205ea8db8f --- /dev/null +++ b/tests/phpstan/tests/Rules/EnsureCallbackMethodsExistsInSubscribedEventsTest.php @@ -0,0 +1,45 @@ +analyse([__DIR__ . '/../data/EnsureCallbackMethodsExistsInSubscribedEventsTest/valid.php'], [ + ]); + } + + public function testMethodNotExistingShouldHaveErrors() { + $this->analyse([__DIR__ . '/../data/EnsureCallbackMethodsExistsInSubscribedEventsTest/not-existing.php'], [ + [ + "The callback function 'return_falses' declared within 'get_subscribed_events' does not exist in the class 'WP_Rocket\Engine\Admin\ActionSchedulerSubscriber'.", + 19 + ], + [ + "The callback function 'hide_pastdue_status_filterss' declared within 'get_subscribed_events' does not exist in the class 'WP_Rocket\Engine\Admin\ActionSchedulerSubscriber'.", + 21 + ], + [ + "The callback function 'hide_pastdue_status_filterss' declared within 'get_subscribed_events' does not exist in the class 'WP_Rocket\Engine\Admin\ActionSchedulerSubscriber'.", + 22 + ] + ]); + } + + public function testComplexSyntaxNotExistingShouldHaveErrors() { + $this->analyse([__DIR__ . '/../data/EnsureCallbackMethodsExistsInSubscribedEventsTest/complex-syntax.php'], [ + [ + "The callback function 'exclude_inline_from_rucsss' declared within 'get_subscribed_events' does not exist in the class 'WP_Rocket\ThirdParty\Plugins\InlineRelatedPosts'.", + 23 + ] + ]); + } +} diff --git a/tests/phpstan/tests/bootstrap.php b/tests/phpstan/tests/bootstrap.php new file mode 100644 index 0000000000..3106423493 --- /dev/null +++ b/tests/phpstan/tests/bootstrap.php @@ -0,0 +1,3 @@ + 'exclude_inline_from_rucsss' ]; + } + + /** + * Exclude inline style from RUCSS. + * + * @param array $excluded excluded css. + * @return array + */ + public function exclude_inline_from_rucss( $excluded ) { + $excluded[] = '.centered-text-area'; + $excluded[] = '.ctaText'; + + return $excluded; + } +} diff --git a/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/not-existing.php b/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/not-existing.php new file mode 100644 index 0000000000..7e84d20696 --- /dev/null +++ b/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/not-existing.php @@ -0,0 +1,42 @@ + 'return_falses', + 'action_scheduler_extra_action_counts' => [ + ['hide_pastdue_status_filterss'], + ['hide_pastdue_status_filterss', 10, 3], + ], + ]; + } + + /** + * Hide past-due from status filter in Action Scheduler tools page. + * + * @param array $extra_actions Array with format action_count_identifier => action count. + * + * @return array + */ + public function hide_pastdue_status_filter( array $extra_actions ) { + if ( ! isset( $extra_actions['past-due'] ) ) { + return $extra_actions; + } + + unset( $extra_actions['past-due'] ); + return $extra_actions; + } +} diff --git a/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/valid.php b/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/valid.php new file mode 100644 index 0000000000..83cd012da4 --- /dev/null +++ b/tests/phpstan/tests/data/EnsureCallbackMethodsExistsInSubscribedEventsTest/valid.php @@ -0,0 +1,42 @@ + 'return_false', + 'action_scheduler_extra_action_counts' => [ + ['hide_pastdue_status_filter'], + ['hide_pastdue_status_filter', 10, 3], + ], + ]; + } + + /** + * Hide past-due from status filter in Action Scheduler tools page. + * + * @param array $extra_actions Array with format action_count_identifier => action count. + * + * @return array + */ + public function hide_pastdue_status_filter( array $extra_actions ) { + if ( ! isset( $extra_actions['past-due'] ) ) { + return $extra_actions; + } + + unset( $extra_actions['past-due'] ); + return $extra_actions; + } +} diff --git a/tests/phpstan/tests/phpunit.xml b/tests/phpstan/tests/phpunit.xml new file mode 100644 index 0000000000..8028b90e08 --- /dev/null +++ b/tests/phpstan/tests/phpunit.xml @@ -0,0 +1,27 @@ + + + + + Rules + + + + + + Rules + + + From 8e60b551a365dcb6447d7e97b131cc0ec0ecb37a Mon Sep 17 00:00:00 2001 From: Gael Robin Date: Mon, 25 Nov 2024 02:53:04 +0100 Subject: [PATCH 3/4] Modify rule to work properly --- ...allbackMethodsExistsInSubscribedEvents.php | 164 +++++++++++------- 1 file changed, 104 insertions(+), 60 deletions(-) diff --git a/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php b/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php index 45d964b95a..b866e182a2 100644 --- a/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php +++ b/tests/phpstan/Rules/EnsureCallbackMethodsExistsInSubscribedEvents.php @@ -3,67 +3,105 @@ namespace WP_Rocket\Tests\phpstan\Rules; use PhpParser\Node; +use PhpParser\Node\Expr\ArrayItem; use PhpParser\Node\Stmt\Return_; use PHPStan\Analyser\Scope; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -class EnsureCallbackMethodsExistsInSubscribedEvents implements Rule -{ - public function getNodeType(): string - { - return Return_::class; // We want to inspect return statements +class EnsureCallbackMethodsExistsInSubscribedEvents implements Rule { + + /** + * Returns the type of the node that this rule is interested in. + * + * @return string The class name of the node type. + */ + public function getNodeType(): string { + return Return_::class; } - public function processNode(Node $node, Scope $scope): array - { + /** + * Processes a node to ensure that callback methods exist in subscribed events. + * + * @param Node $node The node to process. + * @param Scope $scope The scope in which the node is being processed. + * @return array An array of errors found during the processing of the node. + */ + public function processNode( Node $node, Scope $scope ): array { + // Bail out early if the node is not a return statement with an expression. + if ( ! ( $node instanceof Return_ && $node->expr ) ) { + return []; + } + + $function_name = $scope->getFunctionName(); + // Bail out early if the method is not `get_subscribed_events`. + if ( 'get_subscribed_events' !== $function_name ) { + return []; + } + + // Check if the return expression is an array. + if ( $node->expr instanceof Node\Expr\Array_ ) { + return $this->analyzeArray( $node->expr, $scope ); + } + + return []; + } + + /** + * Analyzes the array structure returned by `get_subscribed_events`. + * + * @param Node\Expr\Array_ $array_expr The array expression node to analyze. + * @param Scope $scope The scope in which the array expression is being analyzed. + * @return array An array of errors found during the analysis of the array structure. + */ + private function analyzeArray( Node\Expr\Array_ $array_expr, Scope $scope ): array { $errors = []; - // Check if the node is a return statement - if ($node instanceof Return_ && $node->expr) { - - // Get the function/method name from the scope - $functionName = $scope->getFunctionName(); - if ('get_subscribed_events' === $functionName) { - // Check if the return expression is an array - if ($node->expr instanceof Node\Expr\Array_) { - foreach ($node->expr->items as $item) { - if ($item instanceof Node\Expr\ArrayItem) { - // Ensure the key exists and is a valid type (like a string) before accessing the value - $hookName = null; - if ($item->key instanceof Node\Scalar\String_) { - $hookName = $item->key->value; - } - - // Analyze the value of the array item - $methodValue = $item->value; - $errors = array_merge($errors, $this->analyzeMethodValue($methodValue, $scope)); - } - } - } + foreach ( $array_expr->items as $item ) { + // Skip invalid array items. + if ( ! $item instanceof ArrayItem ) { // @phpstan-ignore-line PHPStan mess up with the type, and report a false positive error. + continue; } + + $method_value = $item->value; // @phpstan-ignore-line Because of the above issue, we need to ignore the error here as it thinks it's unreachable. + + // Analyze the method value. + $errors = array_merge( $errors, $this->analyzeMethodValue( $method_value, $scope ) ); } - return $errors; // Return all collected errors + return $errors; } /** - * Analyze the method value from the array structure. + * Analyzes the method value from the array structure. + * + * @param Node $method_value The method value node to analyze. + * @param Scope $scope The scope in which the method value is being analyzed. + * @return array An array of errors found during the analysis of the method value. + * @phpstan-ignore-next-line While running phpstan, it can't detect it's being used while in real it is. */ - private function analyzeMethodValue(Node $methodValue, Scope $scope): array - { + private function analyzeMethodValue( Node $method_value, Scope $scope ): array { $errors = []; - if ($methodValue instanceof Node\Scalar\String_) { - // Simple structure: array('hook_name' => 'method_name') - $errors = $this->checkIfMethodExistsInClass($methodValue->value, $scope, $methodValue); - } elseif ($methodValue instanceof Node\Expr\Array_) { - // More complex structures: array or nested array - foreach ($methodValue->items as $subItem) { - if ($subItem instanceof Node\Expr\ArrayItem) { - if ($subItem->value instanceof Node\Scalar\String_) { - $errors = array_merge($errors, $this->checkIfMethodExistsInClass($subItem->value->value, $scope, $subItem->value)); - } + if ( $method_value instanceof Node\Scalar\String_ ) { + // Simple structure: array('hook_name' => 'method_name'). + return $this->checkIfMethodExistsInClass( $method_value->value, $scope, $method_value ); + } + + if ( $method_value instanceof Node\Expr\Array_ ) { + // More complex structures: array or nested array. + foreach ( $method_value->items as $sub_item ) { + if ( ! ( $sub_item instanceof ArrayItem ) ) { // @phpstan-ignore-line + continue; + } + + // @phpstan-ignore-next-line While running phpstan, it can't detect it's being used while in real it is. + if ( $sub_item->value instanceof Node\Scalar\String_ ) { + // Handle string callback in nested array. + $errors = array_merge( $errors, $this->checkIfMethodExistsInClass( $sub_item->value->value, $scope, $sub_item->value ) ); + } elseif ( $sub_item->value instanceof Node\Expr\Array_ ) { + // Recursively analyze nested arrays. + $errors = array_merge( $errors, $this->analyzeMethodValue( $sub_item->value, $scope ) ); } } } @@ -71,38 +109,44 @@ private function analyzeMethodValue(Node $methodValue, Scope $scope): array return $errors; } - public function checkIfMethodExistsInClass(string $methodName, Scope $scope, Node $node): array - { - $classReflection = $scope->getClassReflection(); + /** + * Checks if a method exists in the class, its parent class, or its interfaces. + * + * @param string $method_name The name of the method to check. + * @param Scope $scope The scope in which the method is being checked. + * @param Node $node The node representing the method call. + * @return array An array of errors if the method does not exist, otherwise an empty array. + */ + public function checkIfMethodExistsInClass( string $method_name, Scope $scope, Node $node ): array { + $class_reflection = $scope->getClassReflection(); - // Check if the class reflection is available and the method exists - if ($classReflection && $classReflection->hasMethod($methodName)) { + // Bail out early if the class reflection or method is found. + if ( $class_reflection && $class_reflection->hasMethod( $method_name ) ) { return []; } - // Check if the method exists in the parent class or interfaces - $parentClass = $classReflection ? $classReflection->getParentClass() : null; - if ($parentClass && $parentClass->hasMethod($methodName)) { + $parent_class = $class_reflection ? $class_reflection->getParentClass() : null; + if ( $parent_class && $parent_class->hasMethod( $method_name ) ) { return []; } - foreach ($classReflection->getInterfaces() as $interface) { - if ($interface->hasMethod($methodName)) { + foreach ( $class_reflection->getInterfaces() as $interface ) { + if ( $interface->hasMethod( $method_name ) ) { return []; } } - // If the method doesn't exist, return an error using RuleErrorBuilder - $errorMessage = sprintf( + // If the method doesn't exist, return an error. + $error_message = sprintf( "The callback function '%s' declared within 'get_subscribed_events' does not exist in the class '%s'.", - $methodName, - $classReflection ? $classReflection->getName() : 'unknown' + $method_name, + $class_reflection ? $class_reflection->getName() : 'unknown' ); return [ - RuleErrorBuilder::message($errorMessage) - ->line($node->getLine()) // Add the line number - ->build() + RuleErrorBuilder::message( $error_message ) + ->line( $node->getLine() ) // Add the line number. + ->build(), ]; } } From 73783d66726cf1834b75fe259d619f3cf792e359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Robin?= Date: Tue, 26 Nov 2024 02:44:26 +0100 Subject: [PATCH 4/4] Add usage of baseline --- phpstan.neon.dist | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpstan.neon.dist b/phpstan.neon.dist index c4fb22b888..8d34426995 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -1,10 +1,10 @@ includes: - phar://phpstan.phar/conf/bleedingEdge.neon -# - phpstan-baseline.neon + - phpstan-baseline.neon parameters: level: 4 inferPrivatePropertyTypeFromConstructor: true -# reportUnmatchedIgnoredErrors: false + reportUnmatchedIgnoredErrors: false paths: # Test only the new architecture for now. - %currentWorkingDirectory%/inc/Engine/