From 47b6389ae8a0a784f51886f32f19c7f1fc86a4b2 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 31 Dec 2023 18:43:07 +0000 Subject: [PATCH 1/2] [config] Extract RectorConfigValidator and handle config validation there --- packages/Config/RectorConfig.php | 127 ++++-------------- packages/Validation/RectorConfigValidator.php | 105 +++++++++++++++ phpstan.neon | 1 + 3 files changed, 129 insertions(+), 104 deletions(-) create mode 100644 packages/Validation/RectorConfigValidator.php diff --git a/packages/Config/RectorConfig.php b/packages/Config/RectorConfig.php index ceda2743314..47070196605 100644 --- a/packages/Config/RectorConfig.php +++ b/packages/Config/RectorConfig.php @@ -17,6 +17,7 @@ use Rector\Core\ValueObject\PhpVersion; use Rector\Core\ValueObject\PolyfillPackage; use Rector\Skipper\SkipCriteriaResolver\SkippedClassResolver; +use Rector\Validation\RectorConfigValidator; use Webmozart\Assert\Assert; /** @@ -41,6 +42,7 @@ public function paths(array $paths): void { Assert::allString($paths); + // ensure paths exist foreach ($paths as $path) { if (str_contains($path, '*')) { continue; @@ -81,7 +83,7 @@ public function enableCollectors(): void SimpleParameterProvider::setParameter(Option::COLLECTORS, true); } - public function parallel(int $seconds = 120, int $maxNumberOfProcess = 16, int $jobSize = 15): void + public function parallel(int $seconds = 120, int $maxNumberOfProcess = 32, int $jobSize = 20): void { SimpleParameterProvider::setParameter(Option::PARALLEL, true); SimpleParameterProvider::setParameter(Option::PARALLEL_JOB_TIMEOUT_IN_SECONDS, $seconds); @@ -100,51 +102,14 @@ public function memoryLimit(string $memoryLimit): void } /** - * @param array $criteria + * @see https://getrector.com/documentation/ignoring-rules-or-paths + * @param array $skip */ - public function skip(array $criteria): void - { - $notExistsRules = []; - foreach ($criteria as $key => $value) { - /** - * Cover define rule then list of files - * - * $rectorConfig->skip([ - * RenameVariableToMatchMethodCallReturnTypeRector::class => [ - * __DIR__ . '/packages/Config/RectorConfig.php' - * ], - * ]); - */ - if ($this->isRuleNoLongerExists($key)) { - $notExistsRules[] = $key; - } - - if (! is_string($value)) { - continue; - } - - /** - * Cover direct value without array list of files, eg: - * - * $rectorConfig->skip([ - * StringClassNameToClassConstantRector::class, - * ]); - */ - if ($this->isRuleNoLongerExists($value)) { - $notExistsRules[] = $value; - } - } - - if ($notExistsRules !== []) { - throw new ShouldNotHappenException( - 'Following rules on $rectorConfig->skip() do no longer exist or changed to different namespace: ' . implode( - ', ', - $notExistsRules - ) - ); - } + public function skip(array $skip): void + { + RectorConfigValidator::ensureRectorRulesExist($skip); - SimpleParameterProvider::addParameter(Option::SKIP, $criteria); + SimpleParameterProvider::addParameter(Option::SKIP, $skip); } public function removeUnusedImports(bool $removeUnusedImports = true): void @@ -233,6 +198,16 @@ public function rule(string $rectorClass): void SimpleParameterProvider::addParameter(Option::REGISTERED_RECTOR_RULES, $rectorClass); } + /** + * @param array> $collectorClasses + */ + public function collectors(array $collectorClasses): void + { + foreach ($collectorClasses as $collectorClass) { + $this->collector($collectorClass); + } + } + /** * @param class-string $collectorClass */ @@ -266,7 +241,8 @@ public function import(string $filePath): void public function rules(array $rectorClasses): void { Assert::allString($rectorClasses); - $this->ensureNotDuplicatedClasses($rectorClasses); + + RectorConfigValidator::ensureNoDuplicatedClasses($rectorClasses); foreach ($rectorClasses as $rectorClass) { $this->rule($rectorClass); @@ -365,16 +341,9 @@ public function indent(string $character, int $count): void } /** - * @api deprecated, just for BC layer warning + * @internal + * @api used only in tests */ - public function services(): void - { - trigger_error( - 'The services() method is deprecated. Use $rectorConfig->singleton(ServiceType::class) instead', - E_USER_ERROR - ); - } - public function resetRuleConfigurations(): void { $this->ruleConfigurations = []; @@ -429,54 +398,4 @@ public function singleton($abstract, mixed $concrete = null): void $this->tag($abstract, $autotagInterface); } } - - private function isRuleNoLongerExists(mixed $skipRule): bool - { - return // only validate string - is_string($skipRule) - // not regex path - && ! str_contains($skipRule, '*') - // a Rector end - && str_ends_with($skipRule, 'Rector') - // not directory - && ! is_dir($skipRule) - // not file - && ! is_file($skipRule) - // class not exists - && ! class_exists($skipRule); - } - - /** - * @param string[] $values - * @return string[] - */ - private function resolveDuplicatedValues(array $values): array - { - $counted = array_count_values($values); - $duplicates = []; - - foreach ($counted as $value => $count) { - if ($count > 1) { - $duplicates[] = $value; - } - } - - return array_unique($duplicates); - } - - /** - * @param string[] $rectorClasses - */ - private function ensureNotDuplicatedClasses(array $rectorClasses): void - { - $duplicatedRectorClasses = $this->resolveDuplicatedValues($rectorClasses); - if ($duplicatedRectorClasses === []) { - return; - } - - throw new ShouldNotHappenException('Following rules are registered twice: ' . implode( - ', ', - $duplicatedRectorClasses - )); - } } diff --git a/packages/Validation/RectorConfigValidator.php b/packages/Validation/RectorConfigValidator.php new file mode 100644 index 00000000000..1b58a77efb0 --- /dev/null +++ b/packages/Validation/RectorConfigValidator.php @@ -0,0 +1,105 @@ + $value) { + if (self::isRectorClassValue($key) && ! class_exists($key)) { + $nonExistingRules[] = $key; + continue; + } + if (! self::isRectorClassValue($value)) { + continue; + } + if (class_exists($value)) { + continue; + } + $nonExistingRules[] = $value; + } + + if ($nonExistingRules === []) { + return; + } + + $nonExistingRulesString = ''; + foreach ($nonExistingRules as $nonExistingRule) { + $nonExistingRulesString .= ' * ' . $nonExistingRule . PHP_EOL; + } + + throw new ShouldNotHappenException( + 'These rules from "$rectorConfig->skip()" does not exist - remove them or fix their names:' . PHP_EOL . $nonExistingRulesString + ); + } + + private static function isRectorClassValue(mixed $value): bool + { + // only validate string + if (! is_string($value)) { + return false; + } + + // not regex path + if (str_contains($value, '*')) { + return false; + } + + // not if no Rector suffix + if (! str_ends_with($value, 'Rector')) { + return false; + } + + // not directory + if (is_dir($value)) { + return false; + } + + // not file + return ! is_file($value); + } + + /** + * @param string[] $values + * @return string[] + */ + private static function resolveDuplicatedValues(array $values): array + { + $counted = array_count_values($values); + $duplicates = []; + + foreach ($counted as $value => $count) { + if ($count > 1) { + $duplicates[] = $value; + } + } + + return array_unique($duplicates); + } +} diff --git a/phpstan.neon b/phpstan.neon index 05e02cee9d9..0f2fff018d3 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -268,6 +268,7 @@ parameters: - src/Bootstrap/ExtensionConfigResolver.php - src/DependencyInjection/LazyContainerFactory.php - packages/Config/RectorConfig.php + - packages/Validation/RectorConfigValidator.php # use of internal phpstan classes - From 6700e1c81bff34ddcc0d7385d79d78ca800ad59e Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 31 Dec 2023 18:49:18 +0000 Subject: [PATCH 2/2] note, down timeout to 60 seconds to spot bottle necks sooner --- packages/Config/RectorConfig.php | 8 ++++++-- packages/Validation/RectorConfigValidator.php | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/Config/RectorConfig.php b/packages/Config/RectorConfig.php index 47070196605..a15f6564ec4 100644 --- a/packages/Config/RectorConfig.php +++ b/packages/Config/RectorConfig.php @@ -83,10 +83,14 @@ public function enableCollectors(): void SimpleParameterProvider::setParameter(Option::COLLECTORS, true); } - public function parallel(int $seconds = 120, int $maxNumberOfProcess = 32, int $jobSize = 20): void + /** + * Defaults in sync with https://phpstan.org/config-reference#parallel-processing + * as we run PHPStan as well + */ + public function parallel(int $processTimeout = 60, int $maxNumberOfProcess = 32, int $jobSize = 20): void { SimpleParameterProvider::setParameter(Option::PARALLEL, true); - SimpleParameterProvider::setParameter(Option::PARALLEL_JOB_TIMEOUT_IN_SECONDS, $seconds); + SimpleParameterProvider::setParameter(Option::PARALLEL_JOB_TIMEOUT_IN_SECONDS, $processTimeout); SimpleParameterProvider::setParameter(Option::PARALLEL_MAX_NUMBER_OF_PROCESSES, $maxNumberOfProcess); SimpleParameterProvider::setParameter(Option::PARALLEL_JOB_SIZE, $jobSize); } diff --git a/packages/Validation/RectorConfigValidator.php b/packages/Validation/RectorConfigValidator.php index 1b58a77efb0..98018d72ee6 100644 --- a/packages/Validation/RectorConfigValidator.php +++ b/packages/Validation/RectorConfigValidator.php @@ -36,12 +36,15 @@ public static function ensureRectorRulesExist(array $skip): void $nonExistingRules[] = $key; continue; } + if (! self::isRectorClassValue($value)) { continue; } + if (class_exists($value)) { continue; } + $nonExistingRules[] = $value; }