From ba588dabf5968f0535cf078eb9f3d6725a00a992 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Wed, 2 Sep 2026 19:19:28 +0200 Subject: [PATCH] [FEATURE] Report exclude entries which did not take effect An exclude entry that quietly matches nothing is the failure the whole filter exists to prevent: a published archive carrying the very directory the configuration was supposed to keep out. `Resources/Private/Build/` written with a trailing slash, with a leading `./` or with backslashes as separators reads like a valid exclude and packages the directory anyway, without a warning and without an error. Report an entry when the created archive still contains what the entry names, together with a hint about the most likely reason. Comparing against the packaged paths rather than counting pattern matches keeps two cases quiet which are not a problem: an entry for a directory the extension does not have - exclude configurations are shared between extensions and the shipped default covers a lot of them - and an entry already covered by a shorter one next to it. Entries still written with escaped slashes are reported as well, since the escaping is only accepted for compatibility since #107. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_01CFdZJzsCjJ7rmT1u9snkiv Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel --- README.md | 34 ++++- src/Command/AbstractClientRequestCommand.php | 4 + .../CreateExtensionArtefactCommand.php | 6 + .../UploadExtensionVersionCommand.php | 6 + src/Service/VersionService.php | 127 +++++++++++++++++- .../CreateExtensionArtefactCommandTest.php | 18 +++ .../UploadExtensionVersionCommandTest.php | 14 ++ .../config_absent_directory.php | 11 ++ .../config_covered_directory.php | 9 ++ .../config_ineffective_directory.php | 8 ++ .../config_ineffective_file.php | 8 ++ tests/Unit/Service/VersionServiceTest.php | 109 +++++++++++++-- 12 files changed, 339 insertions(+), 15 deletions(-) create mode 100644 tests/Unit/Fixtures/ExcludeFromPackaging/config_absent_directory.php create mode 100644 tests/Unit/Fixtures/ExcludeFromPackaging/config_covered_directory.php create mode 100644 tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_directory.php create mode 100644 tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_file.php diff --git a/README.md b/README.md index 37d574e..b34fa9e 100644 --- a/README.md +++ b/README.md @@ -658,9 +658,37 @@ path to your custom configuration file to the environment variable The entries are plain directory and file names, matched case-insensitively - directories against the beginning of the path, files against the end of the -filename. Nested directories can be written as they appear on disk -(`Resources/Private/Build`); slashes escaped as `Resources\/Private\/Build` -are still accepted and describe the very same directory. +filename. Nested directories are written as they appear on disk, without a +trailing slash and without a leading `./`: + +```php +return [ + 'directories' => [ + 'Resources/Private/Build', + ], + 'files' => [ + 'gulpfile.js', + ], +]; +``` + +Escaping the slashes (`Resources\/Private\/Build`) is still accepted and +describes the very same directory, but is not required any more. + +An entry which did not take effect is reported when the archive is created - +that is, the archive still contains what the entry names. An exclude entry +that quietly does nothing is the reason for this: it used to end up in a +published archive carrying the very directory it was supposed to keep out. + +``` +[WARNING] The exclude entry "Resources/Private/Build/" did not take effect, the + archive contains "Resources/Private/Build/gulpfile.js". Directory + names are matched without a trailing slash, remove it. +``` + +Entries for directories and files the extension does not contain are not +reported. Exclude configurations are usually shared between extensions and +carry entries only some of them need. ## Overview of all available commands diff --git a/src/Command/AbstractClientRequestCommand.php b/src/Command/AbstractClientRequestCommand.php index 8cd5e22..e5400b7 100644 --- a/src/Command/AbstractClientRequestCommand.php +++ b/src/Command/AbstractClientRequestCommand.php @@ -43,6 +43,9 @@ abstract class AbstractClientRequestCommand extends Command /** @var InputInterface */ protected $input; + /** @var SymfonyStyle */ + protected $io; + /** @var HttpClientInterface|null */ private $httpClient; @@ -66,6 +69,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int { $this->input = $input; $io = new SymfonyStyle($input, $output); + $this->io = $io; if ($this->confirmationRequired && !$io->askQuestion(new ConfirmationQuestion($this->getMessages()->getConfirmation())) diff --git a/src/Command/Extension/CreateExtensionArtefactCommand.php b/src/Command/Extension/CreateExtensionArtefactCommand.php index 8d8e327..4310abc 100644 --- a/src/Command/Extension/CreateExtensionArtefactCommand.php +++ b/src/Command/Extension/CreateExtensionArtefactCommand.php @@ -81,6 +81,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int $versionService->createZipArchiveFromPath(getcwd() ?: './'); } + $excludeWarnings = $versionService->getExcludeWarnings(); + + if ($excludeWarnings !== []) { + $io->warning($excludeWarnings); + } + $io->success(sprintf('Extension artefact successfully generated: %s', $versionService->getVersionFilePath())); return 0; diff --git a/src/Command/Extension/UploadExtensionVersionCommand.php b/src/Command/Extension/UploadExtensionVersionCommand.php index 3c06f10..02e0b64 100644 --- a/src/Command/Extension/UploadExtensionVersionCommand.php +++ b/src/Command/Extension/UploadExtensionVersionCommand.php @@ -123,6 +123,12 @@ protected function getFormDataPart(array $options): FormDataPart $versionService->createZipArchiveFromPath(getcwd() ?: './'); } + $excludeWarnings = $versionService->getExcludeWarnings(); + + if ($excludeWarnings !== []) { + $this->io->warning($excludeWarnings); + } + return new FormDataPart([ 'description' => (string)$options['comment'], 'gplCompliant' => '1', diff --git a/src/Service/VersionService.php b/src/Service/VersionService.php index b0c6ce6..b4ea6a3 100644 --- a/src/Service/VersionService.php +++ b/src/Service/VersionService.php @@ -38,6 +38,9 @@ class VersionService /** @var array */ protected $excludeConfiguration = []; + /** @var list The paths the last created archive contains, relative to the extension directory */ + protected $packagedPaths = []; + public function __construct(string $version, string $extension, string $transactionPath) { $this->version = $version; @@ -65,6 +68,7 @@ public function createZipArchiveFromPath(string $path): string $zipArchive->open($this->getVersionFilename(), \ZipArchive::CREATE | \ZipArchive::OVERWRITE); $emConfValidationErrors = [EmConfValidationError::NOT_FOUND]; + $this->packagedPaths = []; $iterator = new \RecursiveDirectoryIterator($fullPath, \FilesystemIterator::SKIP_DOTS); $files = new \RecursiveIteratorIterator( @@ -115,7 +119,9 @@ public function createZipArchiveFromPath(string $path): string } // Add the files including their directories - $zipArchive->addFile($fileRealPath, substr($fileRealPath, strlen($fullPath) + 1)); + $packagedPath = substr($fileRealPath, strlen($fullPath) + 1); + $this->packagedPaths[] = $packagedPath; + $zipArchive->addFile($fileRealPath, $packagedPath); } if ($emConfValidationErrors !== []) { @@ -144,6 +150,125 @@ protected function quoteExcludePattern(string $excludeEntry): string return preg_quote(str_replace('\\/', '/', $excludeEntry), '/'); } + /** + * Warnings about the configured exclude entries. + * + * An entry is reported when the archive still carries what the entry names: a + * `Resources/Private/Build/` written with a trailing slash reads like a valid + * exclude and packages the directory anyway. That silent packaging is what the + * filter exists to prevent, so it is reported instead of being guessed straight. + * + * Entries which name something the extension does not contain are not reported. + * Exclude configurations are usually shared between extensions and carry entries + * for directories and files only some of them have - the shipped default + * configuration most of all. + * + * @return list The warnings, empty if there is nothing to report + */ + public function getExcludeWarnings(): array + { + $warnings = []; + + foreach (['directories', 'files'] as $type) { + foreach ($this->excludeConfiguration[$type] as $excludeEntry) { + $excludeEntry = (string)$excludeEntry; + + if (str_contains($excludeEntry, '\\/')) { + $warnings[] = sprintf( + 'The exclude entry "%s" escapes its slashes. This is no longer required, write it as "%s".', + $excludeEntry, + str_replace('\\/', '/', $excludeEntry) + ); + } + + $packagedPath = $this->getPackagedPathFor($type, $excludeEntry); + + if ($packagedPath === '') { + continue; + } + + $warnings[] = trim(sprintf( + 'The exclude entry "%s" did not take effect, the archive contains "%s". %s', + $excludeEntry, + $packagedPath, + $this->getExcludeEntryHint($type, $excludeEntry) + )); + } + } + + return $warnings; + } + + /** + * Find a packaged path the given exclude entry names but did not keep out of the + * archive. Since the entry is compared against what was packaged, an entry which + * is covered by another one - `Resources/Private` next to `Resources/Private/Build` - + * has nothing left to report. + * + * @param string $type Either `directories` or `files` + * @param string $excludeEntry The configured directory or file name + * + * @return string The packaged path, empty if the entry has nothing to complain about + */ + protected function getPackagedPathFor(string $type, string $excludeEntry): string + { + $entryPath = trim(str_replace(['\\/', '\\'], '/', $excludeEntry), '/'); + + if (str_starts_with($entryPath, './')) { + $entryPath = substr($entryPath, 2); + } + + if ($entryPath === '') { + return ''; + } + + // A file entry is matched against the filename, a path in it can never match + if ($type === 'files' && !str_contains($entryPath, '/')) { + return ''; + } + + foreach ($this->packagedPaths as $packagedPath) { + if ($type === 'files' && strcasecmp($packagedPath, $entryPath) === 0) { + return $packagedPath; + } + + if ($type === 'directories' && stripos($packagedPath, $entryPath . '/') === 0) { + return $packagedPath; + } + } + + return ''; + } + + /** + * Hint about the most likely reason for an exclude entry not to take effect. + * + * @param string $type Either `directories` or `files` + * @param string $excludeEntry The configured directory or file name + * + * @return string The hint, empty if the entry looks the way it is documented + */ + protected function getExcludeEntryHint(string $type, string $excludeEntry): string + { + if ($type === 'files') { + return 'File entries are matched against the filename, they can not contain a path.'; + } + + if (str_ends_with($excludeEntry, '/')) { + return 'Directory names are matched without a trailing slash, remove it.'; + } + + if (str_starts_with($excludeEntry, './')) { + return 'Directory names are matched relative to the extension directory, remove the leading "./".'; + } + + if (str_contains(str_replace('\\/', '/', $excludeEntry), '\\')) { + return 'Use "/" as directory separator.'; + } + + return ''; + } + /** * Extract the given artefact (from either local or remote), * store it in a temporary transaction path and finally call diff --git a/tests/Unit/Command/Extension/CreateExtensionArtefactCommandTest.php b/tests/Unit/Command/Extension/CreateExtensionArtefactCommandTest.php index 1cb35fc..15ab9b0 100644 --- a/tests/Unit/Command/Extension/CreateExtensionArtefactCommandTest.php +++ b/tests/Unit/Command/Extension/CreateExtensionArtefactCommandTest.php @@ -104,6 +104,24 @@ public function excludedFilesAreNotPackaged(): void self::assertNotContains('vendor/autoload.php', $names); } + #[Test] + public function ineffectiveExcludeEntryIsReported(): void + { + $this->writeExtensionFile('Resources/Private/Build/gulpfile.js', '// build only'); + $this->setEnvironment([ + 'TYPO3_EXCLUDE_FROM_PACKAGING' => __DIR__ . '/../../Fixtures/ExcludeFromPackaging/config_ineffective_directory.php', + ]); + + $tester = $this->tester(); + $tester->execute([ + 'version' => '1.2.3', + 'extensionkey' => 'my_ext', + '--path' => $this->extensionDirectory, + ]); + + self::assertDisplayContains('The exclude entry "Resources/Private/Build/" did not take effect', $tester); + } + #[Test] public function versionMismatchInEmConfIsRejected(): void { diff --git a/tests/Unit/Command/Extension/UploadExtensionVersionCommandTest.php b/tests/Unit/Command/Extension/UploadExtensionVersionCommandTest.php index 1e90327..61cd018 100644 --- a/tests/Unit/Command/Extension/UploadExtensionVersionCommandTest.php +++ b/tests/Unit/Command/Extension/UploadExtensionVersionCommandTest.php @@ -110,6 +110,20 @@ public function transactionDirectoryIsRemovedAfterwards(): void self::assertDirectoryDoesNotExist($this->workingDirectory . '/tailor-version-upload'); } + #[Test] + public function ineffectiveExcludeEntryIsReportedBeforeTheUpload(): void + { + $this->writeExtensionFile('Resources/Private/Build/gulpfile.js', '// build only'); + $this->setEnvironment([ + 'TYPO3_EXCLUDE_FROM_PACKAGING' => __DIR__ . '/../../Fixtures/ExcludeFromPackaging/config_ineffective_directory.php', + ]); + + $tester = $this->apiTester($this->command(), self::jsonResponse([], 201)); + + self::assertSame(0, $tester->execute($this->uploadArguments())); + self::assertDisplayContains('The exclude entry "Resources/Private/Build/" did not take effect', $tester); + } + #[Test] public function failingRequestReturnsFailure(): void { diff --git a/tests/Unit/Fixtures/ExcludeFromPackaging/config_absent_directory.php b/tests/Unit/Fixtures/ExcludeFromPackaging/config_absent_directory.php new file mode 100644 index 0000000..94e5c64 --- /dev/null +++ b/tests/Unit/Fixtures/ExcludeFromPackaging/config_absent_directory.php @@ -0,0 +1,11 @@ + [ + 'Tests', + 'Documentation', + ], + 'files' => [ + 'phpstan.neon', + ], +]; diff --git a/tests/Unit/Fixtures/ExcludeFromPackaging/config_covered_directory.php b/tests/Unit/Fixtures/ExcludeFromPackaging/config_covered_directory.php new file mode 100644 index 0000000..5ada83a --- /dev/null +++ b/tests/Unit/Fixtures/ExcludeFromPackaging/config_covered_directory.php @@ -0,0 +1,9 @@ + [ + 'Resources/Private', + 'Resources/Private/Build', + ], + 'files' => [], +]; diff --git a/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_directory.php b/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_directory.php new file mode 100644 index 0000000..12ead7f --- /dev/null +++ b/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_directory.php @@ -0,0 +1,8 @@ + [ + 'Resources/Private/Build/', + ], + 'files' => [], +]; diff --git a/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_file.php b/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_file.php new file mode 100644 index 0000000..21582de --- /dev/null +++ b/tests/Unit/Fixtures/ExcludeFromPackaging/config_ineffective_file.php @@ -0,0 +1,8 @@ + [], + 'files' => [ + 'Resources/Private/Build/gulpfile.js', + ], +]; diff --git a/tests/Unit/Service/VersionServiceTest.php b/tests/Unit/Service/VersionServiceTest.php index 019ff69..da422ed 100644 --- a/tests/Unit/Service/VersionServiceTest.php +++ b/tests/Unit/Service/VersionServiceTest.php @@ -111,7 +111,8 @@ public function getVersionFilenameAsMd5Test(): void #[Test] public function excludedDirectoryContainingASlashIsNotPackaged(): void { - $packagedFiles = $this->packageExtensionWithExcludeConfiguration('config_nested_directory.php'); + $versionService = $this->packageExtensionWithExcludeConfiguration('config_nested_directory.php'); + $packagedFiles = $this->packagedFiles($versionService); self::assertContains('ext_emconf.php', $packagedFiles); self::assertNotContains('Resources/Private/Build/gulpfile.js', $packagedFiles); @@ -120,33 +121,116 @@ public function excludedDirectoryContainingASlashIsNotPackaged(): void #[Test] public function excludedDirectoryContainingAnEscapedSlashIsNotPackaged(): void { - $packagedFiles = $this->packageExtensionWithExcludeConfiguration('config_nested_directory_escaped.php'); + $versionService = $this->packageExtensionWithExcludeConfiguration('config_nested_directory_escaped.php'); + $packagedFiles = $this->packagedFiles($versionService); self::assertContains('ext_emconf.php', $packagedFiles); self::assertNotContains('Resources/Private/Build/gulpfile.js', $packagedFiles); } + #[Test] + public function excludeEntryWhichMatchedIsNotWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_nested_directory.php'); + + self::assertSame([], $versionService->getExcludeWarnings()); + } + + #[Test] + public function packagingWithTheDefaultConfigurationProducesNoWarnings(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration(null); + + self::assertSame([], $versionService->getExcludeWarnings()); + } + + #[Test] + public function directoryExcludeWithATrailingSlashIsWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_ineffective_directory.php'); + $warnings = $versionService->getExcludeWarnings(); + + self::assertCount(1, $warnings); + self::assertStringContainsString('"Resources/Private/Build/" did not take effect', $warnings[0]); + self::assertStringContainsString('the archive contains "Resources/Private/Build/gulpfile.js"', $warnings[0]); + self::assertStringContainsString('remove it', $warnings[0]); + self::assertContains('Resources/Private/Build/gulpfile.js', $this->packagedFiles($versionService)); + } + + #[Test] + public function fileExcludeContainingAPathIsWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_ineffective_file.php'); + $warnings = $versionService->getExcludeWarnings(); + + self::assertCount(1, $warnings); + self::assertStringContainsString('can not contain a path', $warnings[0]); + } + + #[Test] + public function excludeEntryForSomethingTheExtensionDoesNotContainIsNotWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_absent_directory.php'); + + self::assertSame([], $versionService->getExcludeWarnings()); + } + + #[Test] + public function excludeEntryCoveredByAnotherEntryIsNotWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_covered_directory.php'); + + self::assertSame([], $versionService->getExcludeWarnings()); + self::assertNotContains('Resources/Private/Build/gulpfile.js', $this->packagedFiles($versionService)); + } + + #[Test] + public function excludeEntryWithEscapedSlashesIsWarnedAbout(): void + { + $versionService = $this->packageExtensionWithExcludeConfiguration('config_nested_directory_escaped.php'); + $warnings = $versionService->getExcludeWarnings(); + + self::assertCount(1, $warnings); + self::assertStringContainsString('escapes its slashes', $warnings[0]); + self::assertStringContainsString('write it as "Resources/Private/Build"', $warnings[0]); + } + /** - * Package an extension directory with the given exclude configuration - * and return the filenames the created archive contains. + * Package an extension directory with the given exclude configuration. * - * @param string $configurationFilename Filename of the exclude configuration fixture + * @param string|null $configurationFilename Filename of the exclude configuration + * fixture, null for the shipped default * - * @return list The packaged filenames + * @return VersionService The service which created the archive */ - protected function packageExtensionWithExcludeConfiguration(string $configurationFilename): array + protected function packageExtensionWithExcludeConfiguration(?string $configurationFilename): VersionService { unset($_ENV); - putenv('TYPO3_EXCLUDE_FROM_PACKAGING=' . __DIR__ . '/../Fixtures/ExcludeFromPackaging/' . $configurationFilename); + putenv('TYPO3_EXCLUDE_FROM_PACKAGING=' . ( + $configurationFilename !== null ? $this->excludeConfigurationPath($configurationFilename) : '' + )); $extensionPath = $this->createExtensionDirectory(); $transactionPath = $this->createTemporaryDirectory(); - $archivePath = (new VersionService('1.0.0', 'my_ext', $transactionPath)) - ->createZipArchiveFromPath($extensionPath); + $versionService = new VersionService('1.0.0', 'my_ext', $transactionPath); + $versionService->createZipArchiveFromPath($extensionPath); + + return $versionService; + } + + protected function excludeConfigurationPath(string $configurationFilename): string + { + return __DIR__ . '/../Fixtures/ExcludeFromPackaging/' . $configurationFilename; + } + /** + * @return list The filenames the created archive contains + */ + protected function packagedFiles(VersionService $versionService): array + { $archive = new \ZipArchive(); - $archive->open($archivePath); + $archive->open($versionService->getVersionFilePath()); $packagedFiles = []; for ($index = 0; $index < $archive->numFiles; $index++) { @@ -204,6 +288,9 @@ protected function tearDown(): void $this->temporaryDirectories = []; + // Do not leak a fixture configuration into the next test + putenv('TYPO3_EXCLUDE_FROM_PACKAGING='); + parent::tearDown(); }