From be827661f14e1f42f2cca06807c269f79c9b607a Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Sat, 9 Mar 2024 14:22:36 +0100 Subject: [PATCH 1/6] Take hirarchy into account --- lib/Filesystem/Domain/FileList.php | 44 ++++++++++++++++--- .../Tests/Unit/Domain/FileListTest.php | 16 +++++++ .../Filesystem/FilesystemFileListProvider.php | 13 ++---- lib/Indexer/Extension/IndexerExtension.php | 4 +- 4 files changed, 58 insertions(+), 19 deletions(-) diff --git a/lib/Filesystem/Domain/FileList.php b/lib/Filesystem/Domain/FileList.php index 41d556becd..9221213c46 100644 --- a/lib/Filesystem/Domain/FileList.php +++ b/lib/Filesystem/Domain/FileList.php @@ -86,19 +86,49 @@ public function byExtensions(array $extensions): self } /** - * @param string[] $globPatterns + * @param string[] $includePatterns + * @param string[] $excludePatterns */ - public function excludePatterns(array $globPatterns): self - { - return $this->filter(function (SplFileInfo $info) use ($globPatterns) { - foreach ($globPatterns as $pattern) { - if (Glob::match($info->getPathname(), $pattern)) { - return false; + public function includeAndExclude(array $includePatterns, array $excludePatterns): self + { + // Building map of include paths that are sub paths of excludes so that we can include them again + $includedExcludes = []; + foreach($excludePatterns as $excludePattern) { + foreach($includePatterns as $includePattern) { + if (Glob::match($includePattern, $excludePattern)) { + $includedExcludes[$excludePattern][] = $includePattern; } } + } + + $this->includePatterns($includePatterns); + + return $this->filter(function (SplFileInfo $info) use ($excludePatterns, $includedExcludes) { + foreach ($excludePatterns as $pattern) { + if (!Glob::match($info->getPathname(), $pattern)) { + continue; + } + + $includePatterns = $includedExcludes[$pattern] ?? []; + foreach($includePatterns as $includePattern) { + if (Glob::match($info->getPathname(), $includePattern)) { + return true; + } + } + return false; + } return true; }); + + } + + /** + * @param string[] $globPatterns + */ + public function excludePatterns(array $globPatterns): self + { + return $this->includeAndExclude([], $globPatterns); } /** diff --git a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php index 14dba9aab6..2aa6cf09ee 100644 --- a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php +++ b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php @@ -140,6 +140,22 @@ public function testIncldesFilesMatchingPatterns(): void ])); } + public function testIncludesExcludePatterns(): void + { + + $list = FileList::fromFilePaths([ + FilePath::fromString('/vendor/cache/important/bartest.php'), + FilePath::fromString('/vendor/cache/important/footest.php'), + FilePath::fromString('/vendor/cache/bar.php'), + FilePath::fromString('/vendor/cache/foo.php'), + ])->includeAndExclude( + includePatterns: [ '/src/**/*', '/vendor/cache/important/**/*'], + excludePatterns: [ '/vendor/**/*' ], + ); + + self::assertCount(2, $list); + } + public function testContainingString(): void { $this->workspace()->put('one', 'one two three'); diff --git a/lib/Indexer/Adapter/Filesystem/FilesystemFileListProvider.php b/lib/Indexer/Adapter/Filesystem/FilesystemFileListProvider.php index 7140e61993..40fe59c094 100644 --- a/lib/Indexer/Adapter/Filesystem/FilesystemFileListProvider.php +++ b/lib/Indexer/Adapter/Filesystem/FilesystemFileListProvider.php @@ -30,15 +30,10 @@ public function provideFileList(Index $index, ?string $subPath = null): FileList return FileList::fromSingleFilePath($subPath); } - $files = $this->filesystem->fileList()->byExtensions($this->supportedExtensions); - - if ($this->includePatterns) { - $files = $files->includePatterns($this->includePatterns); - } - - if ($this->excludePatterns) { - $files = $files->excludePatterns($this->excludePatterns); - } + $files = $this->filesystem->fileList() + ->byExtensions($this->supportedExtensions) + ->includeAndExclude($this->includePatterns, $this->excludePatterns) + ; if ($subPath) { $files = $files->within(FilePath::fromString($subPath)); diff --git a/lib/Indexer/Extension/IndexerExtension.php b/lib/Indexer/Extension/IndexerExtension.php index 703a63237e..18571d4de6 100644 --- a/lib/Indexer/Extension/IndexerExtension.php +++ b/lib/Indexer/Extension/IndexerExtension.php @@ -224,9 +224,7 @@ private function registerModel(ContainerBuilder $container): void ->setIncludePatterns($container->get(self::SERVICE_INDEXER_INCLUDE_PATTERNS)) /** @phpstan-ignore-next-line */ ->setSupportedExtensions($container->parameter(self::PARAM_SUPPORTED_EXTENSIONS)->value()) - ->setFollowSymlinks( - (bool) $container->getParameter(self::PARAM_INDEXER_FOLLOW_SYMLINKS), - ) + ->setFollowSymlinks($container->parameter(self::PARAM_INDEXER_FOLLOW_SYMLINKS)->bool()) ->setStubPaths($container->getParameter(self::PARAM_STUB_PATHS)); }); From a542e26ae78bc552ff4e2ecfa3369f65f0a099e3 Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Sat, 9 Mar 2024 15:55:52 +0100 Subject: [PATCH 2/6] Simplifying the implemenation --- lib/Filesystem/Domain/FileList.php | 65 +++++-------------- .../Tests/Unit/Domain/FileListTest.php | 51 +++++++++++++-- 2 files changed, 61 insertions(+), 55 deletions(-) diff --git a/lib/Filesystem/Domain/FileList.php b/lib/Filesystem/Domain/FileList.php index 9221213c46..5b593403f1 100644 --- a/lib/Filesystem/Domain/FileList.php +++ b/lib/Filesystem/Domain/FileList.php @@ -89,57 +89,23 @@ public function byExtensions(array $extensions): self * @param string[] $includePatterns * @param string[] $excludePatterns */ - public function includeAndExclude(array $includePatterns, array $excludePatterns): self - { - // Building map of include paths that are sub paths of excludes so that we can include them again - $includedExcludes = []; - foreach($excludePatterns as $excludePattern) { - foreach($includePatterns as $includePattern) { - if (Glob::match($includePattern, $excludePattern)) { - $includedExcludes[$excludePattern][] = $includePattern; - } - } + public function includeAndExclude(array $includePatterns = [], array $excludePatterns = []): self + { + $inclusionMap = []; + foreach ($includePatterns as $includePattern) { + $inclusionMap[$includePattern] = true; + } + foreach ($excludePatterns as $excludePattern) { + $inclusionMap[$excludePattern] = false; } - $this->includePatterns($includePatterns); - - return $this->filter(function (SplFileInfo $info) use ($excludePatterns, $includedExcludes) { - foreach ($excludePatterns as $pattern) { - if (!Glob::match($info->getPathname(), $pattern)) { - continue; - } - - $includePatterns = $includedExcludes[$pattern] ?? []; - foreach($includePatterns as $includePattern) { - if (Glob::match($info->getPathname(), $includePattern)) { - return true; - } - } - return false; - } - - return true; - }); - - } - - /** - * @param string[] $globPatterns - */ - public function excludePatterns(array $globPatterns): self - { - return $this->includeAndExclude([], $globPatterns); - } + // Sort map by keys so that more specific paths are getting matched first + krsort($inclusionMap); - /** - * @param string[] $globPatterns - */ - public function includePatterns(array $globPatterns): self - { - return $this->filter(function (SplFileInfo $info) use ($globPatterns) { - foreach ($globPatterns as $pattern) { - if (Glob::match($info->getPathname(), $pattern)) { - return true; + return $this->filter(function (SplFileInfo $info) use ($inclusionMap): bool { + foreach($inclusionMap as $glob => $isIncluded) { + if (Glob::match($info->getPathname(), $glob)) { + return $isIncluded; } } @@ -163,6 +129,9 @@ public function named(string $name): self ))); } + /** + * @param Closure(SplFileInfo): bool $closure + */ public function filter(Closure $closure): self { return new self(new CallbackFilterIterator($this->iterator, $closure)); diff --git a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php index 2aa6cf09ee..17c84f36f1 100644 --- a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php +++ b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php @@ -121,9 +121,16 @@ public function testExcludesFilesMatchingPatterns(): void FilePath::fromString('/vendor/foo/bar/src/foo.php'), ]); - self::assertCount(2, $list->excludePatterns([ - '/vendor/**/tests/*', - ])); + self::assertEquals( + [ + FilePath::fromString('/vendor/foo/bar/src/bar.php'), + FilePath::fromString('/vendor/foo/bar/src/foo.php'), + ], + iterator_to_array($list->includeAndExclude( + includePatterns: ['/**/*'], + excludePatterns: [ '/vendor/**/tests/*'] + )) + ); } public function testIncldesFilesMatchingPatterns(): void @@ -135,9 +142,15 @@ public function testIncldesFilesMatchingPatterns(): void FilePath::fromString('/vendor/foo/bar/src/foo.php'), ]); - self::assertCount(2, $list->excludePatterns([ - '/vendor/**/tests/*', - ])); + self::assertEquals( + [ + FilePath::fromString('/vendor/foo/bar/tests/bartest.php'), + FilePath::fromString('/vendor/foo/bar/tests/footest.php'), + ], + iterator_to_array($list->includeAndExclude( + includePatterns: [ '/vendor/**/tests/*'], + )) + ); } public function testIncludesExcludePatterns(): void @@ -153,7 +166,31 @@ public function testIncludesExcludePatterns(): void excludePatterns: [ '/vendor/**/*' ], ); - self::assertCount(2, $list); + self::assertEquals( + [ + FilePath::fromString('/vendor/cache/important/bartest.php'), + FilePath::fromString('/vendor/cache/important/footest.php'), + ], + iterator_to_array($list) + ); + } + + public function testExcludesWithShortFolderName(): void + { + $list = FileList::fromFilePaths([ + FilePath::fromString('/src/package/test.php'), + FilePath::fromString('/src/a/test.php'), + ])->includeAndExclude( + includePatterns: [ '/src/**/*'], + excludePatterns: [ '/src/a/*' ], + ); + + self::assertEquals( + [ + FilePath::fromString('/src/package/test.php'), + ], + iterator_to_array($list) + ); } public function testContainingString(): void From d0e29022b942ad0fb3e0ed9a7fcb74674de1d0d5 Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Sat, 9 Mar 2024 16:00:17 +0100 Subject: [PATCH 3/6] Adding test for default behaviour --- lib/Filesystem/Domain/FileList.php | 4 ++++ .../Tests/Unit/Domain/FileListTest.php | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/lib/Filesystem/Domain/FileList.php b/lib/Filesystem/Domain/FileList.php index 5b593403f1..b2053473d6 100644 --- a/lib/Filesystem/Domain/FileList.php +++ b/lib/Filesystem/Domain/FileList.php @@ -92,6 +92,10 @@ public function byExtensions(array $extensions): self public function includeAndExclude(array $includePatterns = [], array $excludePatterns = []): self { $inclusionMap = []; + if ($includePatterns === []) { + $inclusionMap['/**/*'] = true; + } + foreach ($includePatterns as $includePattern) { $inclusionMap[$includePattern] = true; } diff --git a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php index 17c84f36f1..22fc6890b6 100644 --- a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php +++ b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php @@ -153,6 +153,28 @@ public function testIncldesFilesMatchingPatterns(): void ); } + public function testIncludesEverythingByDefault(): void + { + $list = FileList::fromFilePaths([ + FilePath::fromString('/vendor/cache/important/bartest.php'), + FilePath::fromString('/vendor/cache/important/footest.php'), + FilePath::fromString('/vendor/cache/bar.php'), + FilePath::fromString('/vendor/cache/foo.php'), + ])->includeAndExclude( + includePatterns: [], + excludePatterns: [], + ); + + self::assertEquals( + [ + FilePath::fromString('/vendor/cache/important/bartest.php'), + FilePath::fromString('/vendor/cache/important/footest.php'), + FilePath::fromString('/vendor/cache/bar.php'), + FilePath::fromString('/vendor/cache/foo.php'), + ], + iterator_to_array($list) + ); + } public function testIncludesExcludePatterns(): void { From abff473b2597ef70ac0bfbb2d1a0c316f5c462cf Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Sat, 9 Mar 2024 16:35:45 +0100 Subject: [PATCH 4/6] Fixing the path sorting --- CHANGELOG.md | 5 +- lib/Filesystem/Domain/FileList.php | 22 +++++++- .../Tests/Unit/Domain/FileListTest.php | 53 +++++++++++++++---- 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa8c4633a..e7f0dbfd0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ Changelog ========= +Bug fixes: + - Fixing include and exclude patterns #2593 @mamazu + ## 2024-03-09 Features: @@ -51,7 +54,7 @@ Bug fixes: Documentation: - - Added Helix LSP instructions #2581 @lens0021 + - Added Helix LSP instructions #2581 @lens0021 - Fix typos in Behat #2534 @vuon9 - Fix broken external links #2500 @einenlum diff --git a/lib/Filesystem/Domain/FileList.php b/lib/Filesystem/Domain/FileList.php index b2053473d6..8d2b7a76f3 100644 --- a/lib/Filesystem/Domain/FileList.php +++ b/lib/Filesystem/Domain/FileList.php @@ -104,7 +104,27 @@ public function includeAndExclude(array $includePatterns = [], array $excludePat } // Sort map by keys so that more specific paths are getting matched first - krsort($inclusionMap); + uksort($inclusionMap, function (string $x, string $y) { + $partsX = explode(DIRECTORY_SEPARATOR, $x); + $partsY = explode(DIRECTORY_SEPARATOR, $y); + // Longer paths should come first + $countDiff = -(count($partsX) <=> count($partsY)); + if ($countDiff !== 0) { + return $countDiff; + } + + foreach ($partsX as $i => $pathPartX) { + if ($pathPartX === '**' || $pathPartX === '*') { + return 1; + } + + $compare = strcmp($pathPartX, $partsY[$i]); + if($compare !== 0) { + return $compare; + } + } + return 0; + }); return $this->filter(function (SplFileInfo $info) use ($inclusionMap): bool { foreach($inclusionMap as $glob => $isIncluded) { diff --git a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php index 22fc6890b6..0f2667a919 100644 --- a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php +++ b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php @@ -2,6 +2,7 @@ namespace Phpactor\Filesystem\Tests\Unit\Domain; +use Generator; use Phpactor\Filesystem\Domain\FileList; use Phpactor\Filesystem\Domain\FilePath; use Phpactor\Filesystem\Tests\IntegrationTestCase; @@ -197,22 +198,52 @@ public function testIncludesExcludePatterns(): void ); } - public function testExcludesWithShortFolderName(): void - { - $list = FileList::fromFilePaths([ - FilePath::fromString('/src/package/test.php'), - FilePath::fromString('/src/a/test.php'), - ])->includeAndExclude( - includePatterns: [ '/src/**/*'], - excludePatterns: [ '/src/a/*' ], + /** +@param array $fileList +@param array $includePatterns +@param array $excludePatterns +@param array $expected +* @dataProvider provideExcludesWithShortFolderName() +*/ + public function testExcludesWithShortFolderName( + array $fileList, + array $includePatterns, + array $excludePatterns, + array $expected, + ): void { + $list = FileList::fromFilePaths($fileList)->includeAndExclude( + includePatterns:$includePatterns, + excludePatterns: $excludePatterns ); - self::assertEquals( + self::assertEquals($expected, array_map(fn (FilePath $x) => (string) $x, iterator_to_array($list))); + } + + public function provideExcludesWithShortFolderName(): Generator + { + yield 'ascii file name' => [ [ FilePath::fromString('/src/package/test.php'), + FilePath::fromString('/src/a/test.php'), ], - iterator_to_array($list) - ); + [ '/src/**/*'], + [ '/src/a/*' ], + [ + '/src/package/test.php', + ], + ]; + + yield 'unicode file name' => [ + [ + FilePath::fromString('/src/package/test.php'), + FilePath::fromString('/src/ü/test.php'), + ], + [ '/src/**/*'], + [ '/src/ü/*' ], + [ + '/src/package/test.php', + ], + ]; } public function testContainingString(): void From 6e1bff638bc858824d6604f6566b083a71e48a6e Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:41:35 +0100 Subject: [PATCH 5/6] CS --- lib/Filesystem/Tests/Unit/Domain/FileListTest.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php index 0f2667a919..1f440050b9 100644 --- a/lib/Filesystem/Tests/Unit/Domain/FileListTest.php +++ b/lib/Filesystem/Tests/Unit/Domain/FileListTest.php @@ -176,9 +176,9 @@ public function testIncludesEverythingByDefault(): void iterator_to_array($list) ); } + public function testIncludesExcludePatterns(): void { - $list = FileList::fromFilePaths([ FilePath::fromString('/vendor/cache/important/bartest.php'), FilePath::fromString('/vendor/cache/important/footest.php'), @@ -199,12 +199,13 @@ public function testIncludesExcludePatterns(): void } /** -@param array $fileList -@param array $includePatterns -@param array $excludePatterns -@param array $expected -* @dataProvider provideExcludesWithShortFolderName() -*/ + * @param array $fileList + * @param array $includePatterns + * @param array $excludePatterns + * @param array $expected + * + * @dataProvider provideExcludesWithShortFolderName + */ public function testExcludesWithShortFolderName( array $fileList, array $includePatterns, From b2c36ffa22ad46b402c3b1b7e3ecfe661ff4de34 Mon Sep 17 00:00:00 2001 From: mamazu <14860264+mamazu@users.noreply.github.com> Date: Thu, 14 Mar 2024 20:24:26 +0000 Subject: [PATCH 6/6] Adding comments to explain the behaviour. --- lib/Filesystem/Domain/FileList.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/Filesystem/Domain/FileList.php b/lib/Filesystem/Domain/FileList.php index 8d2b7a76f3..5b2c792668 100644 --- a/lib/Filesystem/Domain/FileList.php +++ b/lib/Filesystem/Domain/FileList.php @@ -104,25 +104,26 @@ public function includeAndExclude(array $includePatterns = [], array $excludePat } // Sort map by keys so that more specific paths are getting matched first - uksort($inclusionMap, function (string $x, string $y) { - $partsX = explode(DIRECTORY_SEPARATOR, $x); - $partsY = explode(DIRECTORY_SEPARATOR, $y); - // Longer paths should come first - $countDiff = -(count($partsX) <=> count($partsY)); + uksort($inclusionMap, function (string $a, string $b) { + $partsA = explode(DIRECTORY_SEPARATOR, $a); + $partsB = explode(DIRECTORY_SEPARATOR, $b); + $countDiff = count($partsA) <=> count($partsB); if ($countDiff !== 0) { - return $countDiff; + // Longer paths should come first + return -$countDiff; } - foreach ($partsX as $i => $pathPartX) { - if ($pathPartX === '**' || $pathPartX === '*') { + foreach ($partsA as $i => $pathPartA) { + if ($pathPartA === '**' || $pathPartA === '*') { return 1; } - $compare = strcmp($pathPartX, $partsY[$i]); + $compare = strcmp($pathPartA, $partsB[$i]); if($compare !== 0) { return $compare; } } + // If none of the path segments were different, then they must be equal return 0; });