From 2c68ddd19fd571143de5302e6b397a0d2254dbbb Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Thu, 30 Jul 2026 18:14:42 +0200 Subject: [PATCH] refactor: collapse single-use Skipper collaborators ClassSkipVoter was the only implementation of a voter pattern and the only consumer of its own indirection: both methods were pass-throughs to SkipSkipper and SkippedClassResolver. Inline it into Skipper. Fnmatcher, RealpathMatcher and FnMatchPathNormalizer were one-method services injected into FileInfoMatcher and used nowhere else. They are steps of one matching algorithm, so fold them in as private methods; FileInfoMatcher now has no constructor dependencies. PathSkipper looped over skipped paths and passed each one to the matcher as a single-element array, while matchPattern() already loops and returns the matching pattern. Call it once. That leaves doesFileInfoMatchPatterns() without callers, so drop it. FnMatchPathNormalizerTest cases move to FileInfoMatcherTest, asserted through the public matchPattern() instead of the folded-in method. --- .../FileSystem/FnMatchPathNormalizer.php | 29 ------- src/Skipper/Fnmatcher.php | 18 ----- src/Skipper/Matcher/FileInfoMatcher.php | 79 +++++++++++++------ src/Skipper/RealpathMatcher.php | 35 -------- src/Skipper/SkipVoter/ClassSkipVoter.php | 35 -------- src/Skipper/Skipper/PathSkipper.php | 11 ++- src/Skipper/Skipper/Skipper.php | 11 ++- .../FileSystem/FnMatchPathNormalizerTest.php | 43 ---------- tests/Skipper/Matcher/FileInfoMatcherTest.php | 32 +++++++- .../Fixture/in/it/KeepThisFile.txt | 0 .../Fixture/path/in/it/KeepThisFile.txt | 0 .../Fixture/path/with/KeepThisFile.txt | 0 12 files changed, 98 insertions(+), 195 deletions(-) delete mode 100644 src/Skipper/FileSystem/FnMatchPathNormalizer.php delete mode 100644 src/Skipper/Fnmatcher.php delete mode 100644 src/Skipper/RealpathMatcher.php delete mode 100644 src/Skipper/SkipVoter/ClassSkipVoter.php delete mode 100644 tests/Skipper/FileSystem/FnMatchPathNormalizerTest.php rename tests/Skipper/{FileSystem => Matcher}/Fixture/in/it/KeepThisFile.txt (100%) rename tests/Skipper/{FileSystem => Matcher}/Fixture/path/in/it/KeepThisFile.txt (100%) rename tests/Skipper/{FileSystem => Matcher}/Fixture/path/with/KeepThisFile.txt (100%) diff --git a/src/Skipper/FileSystem/FnMatchPathNormalizer.php b/src/Skipper/FileSystem/FnMatchPathNormalizer.php deleted file mode 100644 index dc1872b2646..00000000000 --- a/src/Skipper/FileSystem/FnMatchPathNormalizer.php +++ /dev/null @@ -1,29 +0,0 @@ -matchPattern($filePath, $filePatterns) !== null; - } - /** * Returns the original (un-normalized) pattern that matched, so callers can report the exact * configured path. Returns null when no pattern matches. @@ -58,7 +40,7 @@ private function doesFileMatchPattern(string $filePath, string $ignoredPath): bo return true; } - $ignoredPath = $this->fnMatchPathNormalizer->normalizeForFnmatch($ignoredPath); + $ignoredPath = $this->normalizeForFnmatch($ignoredPath); if ($ignoredPath === '') { return false; } @@ -71,10 +53,63 @@ private function doesFileMatchPattern(string $filePath, string $ignoredPath): bo return true; } - if ($this->fnmatcher->match($ignoredPath, $filePath)) { + if ($this->matchFnmatch($ignoredPath, $filePath)) { + return true; + } + + return $this->matchRealpath($ignoredPath, $filePath); + } + + private function normalizeForFnmatch(string $path): string + { + if (str_ends_with($path, '*') || str_starts_with($path, '*')) { + return '*' . trim($path, '*') . '*'; + } + + if (str_contains($path, '..')) { + $realPath = realpath($path); + if ($realPath === false) { + return ''; + } + + return PathNormalizer::normalize($realPath); + } + + return $path; + } + + private function matchFnmatch(string $matchingPath, string $filePath): bool + { + if (fnmatch($matchingPath, $filePath)) { + return true; + } + + // in case of relative compare + return fnmatch('*/' . $matchingPath, $filePath); + } + + private function matchRealpath(string $matchingPath, string $filePath): bool + { + $realPathMatchingPath = realpath($matchingPath); + if ($realPathMatchingPath === false) { + return false; + } + + $realpathFilePath = realpath($filePath); + if ($realpathFilePath === false) { + return false; + } + + $normalizedMatchingPath = PathNormalizer::normalize($realPathMatchingPath); + $normalizedFilePath = PathNormalizer::normalize($realpathFilePath); + + // skip define direct path exactly equal + if ($normalizedMatchingPath === $normalizedFilePath) { return true; } - return $this->realpathMatcher->match($ignoredPath, $filePath); + // ensure add / suffix to ensure no same prefix directory + $suffixedMatchingPath = rtrim($normalizedMatchingPath, '/') . '/'; + return str_starts_with($normalizedFilePath, $suffixedMatchingPath); } } diff --git a/src/Skipper/RealpathMatcher.php b/src/Skipper/RealpathMatcher.php deleted file mode 100644 index 6a947fd26b1..00000000000 --- a/src/Skipper/RealpathMatcher.php +++ /dev/null @@ -1,35 +0,0 @@ -reflectionProvider->hasClass($element); - } - - public function matchSkip(string|object $element, string $filePath): ?SkipMatch - { - $skippedClasses = $this->skippedClassResolver->resolve(); - return $this->skipSkipper->match($element, $filePath, $skippedClasses); - } -} diff --git a/src/Skipper/Skipper/PathSkipper.php b/src/Skipper/Skipper/PathSkipper.php index 7d1d68d2b71..5616ad782e3 100644 --- a/src/Skipper/Skipper/PathSkipper.php +++ b/src/Skipper/Skipper/PathSkipper.php @@ -18,13 +18,12 @@ public function __construct( public function shouldSkip(string $filePath): bool { - foreach ($this->skippedPathsResolver->resolve() as $skippedPath) { - if ($this->fileInfoMatcher->doesFileInfoMatchPatterns($filePath, [$skippedPath])) { - $this->usedSkipCollector->markUsed($skippedPath); - return true; - } + $matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $this->skippedPathsResolver->resolve()); + if ($matchedPath === null) { + return false; } - return false; + $this->usedSkipCollector->markUsed($matchedPath); + return true; } } diff --git a/src/Skipper/Skipper/Skipper.php b/src/Skipper/Skipper/Skipper.php index fee095cd785..9664e5595c7 100644 --- a/src/Skipper/Skipper/Skipper.php +++ b/src/Skipper/Skipper/Skipper.php @@ -5,9 +5,10 @@ namespace Rector\Skipper\Skipper; use PhpParser\Node; +use PHPStan\Reflection\ReflectionProvider; use Rector\Contract\Rector\RectorInterface; use Rector\ProcessAnalyzer\RectifiedAnalyzer; -use Rector\Skipper\SkipVoter\ClassSkipVoter; +use Rector\Skipper\SkipCriteriaResolver\SkippedClassResolver; use Rector\Skipper\ValueObject\SkipMatch; /** @@ -19,7 +20,9 @@ public function __construct( private RectifiedAnalyzer $rectifiedAnalyzer, private PathSkipper $pathSkipper, - private ClassSkipVoter $classSkipVoter, + private SkipSkipper $skipSkipper, + private SkippedClassResolver $skippedClassResolver, + private ReflectionProvider $reflectionProvider, private UsedSkipCollector $usedSkipCollector, ) { } @@ -51,11 +54,11 @@ public function shouldSkipElementAndFilePath(string|object $element, string $fil */ public function matchSkip(string|object $element, string $filePath): ?SkipMatch { - if (! $this->classSkipVoter->match($element)) { + if (! is_object($element) && ! $this->reflectionProvider->hasClass($element)) { return null; } - return $this->classSkipVoter->matchSkip($element, $filePath); + return $this->skipSkipper->match($element, $filePath, $this->skippedClassResolver->resolve()); } public function markSkipUsed(SkipMatch $skipMatch): void diff --git a/tests/Skipper/FileSystem/FnMatchPathNormalizerTest.php b/tests/Skipper/FileSystem/FnMatchPathNormalizerTest.php deleted file mode 100644 index 1e176a58ae2..00000000000 --- a/tests/Skipper/FileSystem/FnMatchPathNormalizerTest.php +++ /dev/null @@ -1,43 +0,0 @@ -fnMatchPathNormalizer = $this->make(FnMatchPathNormalizer::class); - } - - #[DataProvider('providePaths')] - public function testPaths(string $path, string $expectedNormalizedPath): void - { - $normalizedPath = $this->fnMatchPathNormalizer->normalizeForFnmatch($path); - $this->assertSame($expectedNormalizedPath, $normalizedPath); - } - - /** - * @return Iterator> - */ - public static function providePaths(): Iterator - { - yield ['path/with/no/asterisk', 'path/with/no/asterisk']; - yield ['*path/with/asterisk/begin', '*path/with/asterisk/begin*']; - yield ['path/with/asterisk/end*', '*path/with/asterisk/end*']; - yield ['*path/with/asterisk/begin/and/end*', '*path/with/asterisk/begin/and/end*']; - yield [__DIR__ . '/Fixture/path/with/../in/it', PathNormalizer::normalize(__DIR__ . '/Fixture/path/in/it')]; - yield [__DIR__ . '/Fixture/path/with/../../in/it', PathNormalizer::normalize(__DIR__ . '/Fixture/in/it')]; - } -} diff --git a/tests/Skipper/Matcher/FileInfoMatcherTest.php b/tests/Skipper/Matcher/FileInfoMatcherTest.php index 862608b55c6..72f92ee4dc7 100644 --- a/tests/Skipper/Matcher/FileInfoMatcherTest.php +++ b/tests/Skipper/Matcher/FileInfoMatcherTest.php @@ -4,6 +4,8 @@ namespace Rector\Tests\Skipper\Matcher; +use Iterator; +use PHPUnit\Framework\Attributes\DataProvider; use Rector\Skipper\Matcher\FileInfoMatcher; use Rector\Testing\PHPUnit\AbstractLazyTestCase; @@ -33,9 +35,33 @@ public function testMatchPatternReturnsNullWhenNoPatternMatches(): void $this->assertNull($matchedPattern); } - public function testDoesFileInfoMatchPatternsStillReportsBoolean(): void + #[DataProvider('providePatterns')] + public function testPatternNormalization(string $filePath, string $filePattern, bool $shouldMatch): void { - $this->assertTrue($this->fileInfoMatcher->doesFileInfoMatchPatterns('/project/src/Foo.php', ['*/src/*'])); - $this->assertFalse($this->fileInfoMatcher->doesFileInfoMatchPatterns('/project/src/Foo.php', ['*/tests/*'])); + $matchedPattern = $this->fileInfoMatcher->matchPattern($filePath, [$filePattern]); + + $this->assertSame($shouldMatch ? $filePattern : null, $matchedPattern); + } + + /** + * @return Iterator + */ + public static function providePatterns(): Iterator + { + // a pattern without asterisk is used as is, and matches the path suffix + yield ['/project/path/with/no/asterisk', 'path/with/no/asterisk', true]; + yield ['/project/path/with/no/asterisk', 'path/with/another/asterisk', false]; + + // an asterisk on either end is padded to both ends + yield ['/project/path/with/asterisk/begin/Foo.php', '*path/with/asterisk/begin', true]; + yield ['/project/path/with/asterisk/end/Foo.php', 'path/with/asterisk/end*', true]; + + // ".." in a pattern is resolved against the real path + yield [__DIR__ . '/Fixture/path/in/it/KeepThisFile.txt', __DIR__ . '/Fixture/path/with/../in/it', true]; + yield [__DIR__ . '/Fixture/in/it/KeepThisFile.txt', __DIR__ . '/Fixture/path/with/../../in/it', true]; + yield [__DIR__ . '/Fixture/in/it/KeepThisFile.txt', __DIR__ . '/Fixture/path/with/../in/it', false]; + + // a ".." pattern that resolves to nothing never matches + yield [__DIR__ . '/Fixture/in/it/KeepThisFile.txt', __DIR__ . '/Fixture/missing/../nope', false]; } } diff --git a/tests/Skipper/FileSystem/Fixture/in/it/KeepThisFile.txt b/tests/Skipper/Matcher/Fixture/in/it/KeepThisFile.txt similarity index 100% rename from tests/Skipper/FileSystem/Fixture/in/it/KeepThisFile.txt rename to tests/Skipper/Matcher/Fixture/in/it/KeepThisFile.txt diff --git a/tests/Skipper/FileSystem/Fixture/path/in/it/KeepThisFile.txt b/tests/Skipper/Matcher/Fixture/path/in/it/KeepThisFile.txt similarity index 100% rename from tests/Skipper/FileSystem/Fixture/path/in/it/KeepThisFile.txt rename to tests/Skipper/Matcher/Fixture/path/in/it/KeepThisFile.txt diff --git a/tests/Skipper/FileSystem/Fixture/path/with/KeepThisFile.txt b/tests/Skipper/Matcher/Fixture/path/with/KeepThisFile.txt similarity index 100% rename from tests/Skipper/FileSystem/Fixture/path/with/KeepThisFile.txt rename to tests/Skipper/Matcher/Fixture/path/with/KeepThisFile.txt