feat: add recursive option for validating translation files#37
feat: add recursive option for validating translation files#37konradmichalik merged 3 commits intomainfrom
Conversation
WalkthroughThis update introduces recursive file collection for translation validation. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ValidateTranslationCommand
participant Collector
User->>CLI: Run validate-translations [--recursive] <paths>
CLI->>ValidateTranslationCommand: Parse options and arguments
ValidateTranslationCommand->>Collector: collectFiles(paths, ..., recursive)
Collector->>Collector: findFiles(path, ..., recursive)
Collector-->>ValidateTranslationCommand: Return collected files
ValidateTranslationCommand-->>CLI: Output validation results
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/FileDetector/Collector.php (4)
🔇 Additional comments (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
tests/src/Command/ValidateTranslationCommandTest.php (1)
241-258: This test has the same issue as the previous one.The test should verify recursive behavior with appropriate fixtures and assertions.
🧹 Nitpick comments (4)
tests/src/Fixtures/recursive/level1/auth.en.php (1)
1-6: Optional: adddeclare(strict_types=1);for consistency with modern PHP practice.
Not required for fixtures, but adding it keeps coding standards uniform across the repo.<?php +declare(strict_types=1); return [ 'login' => 'Login', 'logout' => 'Logout', ];tests/src/Fixtures/recursive/level1/auth.de.php (1)
1-6: Same nitpick as English variant – consider strict types declaration.<?php +declare(strict_types=1); return [ 'login' => 'Anmelden', 'logout' => 'Abmelden', ];tests/src/Command/ValidateTranslationCommandTest.php (1)
279-295: Add test cases for error scenarios with recursive option.While this test verifies successful execution with multiple options, consider adding tests for:
- Recursive search in directories with permission issues
- Recursive search hitting depth limits
- Recursive search with circular symbolic links
Would you like me to generate additional test cases for error scenarios?
tests/src/FileDetector/CollectorTest.php (1)
238-258: Verify that nested files are not collected in non-recursive mode.The test should explicitly verify that only root files are collected and nested files are excluded.
$detector = $this->createMock(DetectorInterface::class); - $detector->method('mapTranslationSet')->willReturn(['non_recursive_mapped_data']); + $detector->expects($this->once()) + ->method('mapTranslationSet') + ->with($this->callback(function ($files) { + return count($files) === 1 && + in_array($this->tempDir.'/root.xlf', $files) && + !in_array($this->tempDir.'/level1/nested.xlf', $files); + })) + ->willReturn(['non_recursive_mapped_data']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)src/Command/ValidateTranslationCommand.php(4 hunks)src/FileDetector/Collector.php(3 hunks)tests/src/Command/ValidateTranslationCommandTest.php(1 hunks)tests/src/FileDetector/CollectorTest.php(1 hunks)tests/src/Fixtures/recursive/level1/auth.de.php(1 hunks)tests/src/Fixtures/recursive/level1/auth.en.php(1 hunks)tests/src/Fixtures/recursive/level1/level2/level3/deep.de.xlf(1 hunks)tests/src/Fixtures/recursive/level1/level2/level3/deep.en.xlf(1 hunks)tests/src/Fixtures/recursive/level1/level2/validation.de.yaml(1 hunks)tests/src/Fixtures/recursive/level1/level2/validation.en.yaml(1 hunks)tests/src/Fixtures/recursive/messages.de.json(1 hunks)tests/src/Fixtures/recursive/messages.en.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/src/Command/ValidateTranslationCommandTest.php (1)
src/Command/ValidateTranslationCommand.php (2)
ValidateTranslationCommand(28-323)execute(147-201)
🔇 Additional comments (14)
tests/src/Fixtures/recursive/messages.en.json (1)
1-1: Fixture looks correct and is syntactically valid JSON.
No issues spotted.tests/src/Fixtures/recursive/messages.de.json (1)
1-1: German fixture mirrors the English keys properly.
Everything is well-formed.tests/src/Fixtures/recursive/level1/auth.en.php (1)
3-6: Keys in auth fixtures are synchronized
The key sets intests/src/Fixtures/recursive/level1/auth.en.phpandauth.de.phphave been verified identical—no drift detected.tests/src/Fixtures/recursive/level1/level2/validation.en.yaml (1)
1-2: YAML fixture is clean and minimal.
Keys and quotes are fine; no concerns.tests/src/Fixtures/recursive/level1/level2/validation.de.yaml (1)
1-2: Test fixture content is appropriate and well-structured.The German validation messages are correctly formatted and the file serves its purpose as a test fixture for the recursive file collection feature.
tests/src/Fixtures/recursive/level1/level2/level3/deep.de.xlf (1)
1-11: XLIFF structure and content are correct.The XLIFF file follows proper XML structure and contains valid German translation content. The deep nesting supports testing of the recursive file collection feature.
tests/src/Fixtures/recursive/level1/level2/level3/deep.en.xlf (1)
3-7: XLIFF structure is correct, consider source/target language configuration.The XLIFF file is properly formatted. Note that both source and target languages are set to "en" with identical content, which may be intentional for testing English-only scenarios.
src/Command/ValidateTranslationCommand.php (4)
87-92: Recursive option is properly configured.The option definition is correct with appropriate short flag, description, and VALUE_NONE type for a boolean flag.
103-104: Help text examples effectively demonstrate the new feature.The usage examples clearly show how to use the recursive option both standalone and in combination with other options.
155-155: Input option reading is implemented correctly.The recursive flag is properly read from input and cast to boolean for safe usage.
169-170: Recursive parameter is correctly passed to the Collector.The recursive flag is properly passed as the fourth parameter to the collectFiles method, maintaining the correct parameter order.
README.md (2)
45-45: Command syntax documentation is comprehensive.The updated command syntax correctly shows support for multiple paths and includes the new recursive option.
54-54: Recursive option is clearly documented.The description accurately explains the functionality and follows the same format as other options in the table.
src/FileDetector/Collector.php (1)
34-34: Good use of null-safe operator for logger calls.The consistent use of the null-safe operator (
?->) for logger calls improves code safety when logger is not injected.Also applies to: 41-41, 56-56, 104-104, 126-126
| public function testExecuteWithRecursiveAndJsonFormat(): void | ||
| { | ||
| $application = new Application(); | ||
| $application->add(new ValidateTranslationCommand()); | ||
|
|
||
| $command = $application->find('validate-translations'); | ||
| $commandTester = new CommandTester($command); | ||
|
|
||
| $commandTester->execute([ | ||
| 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'], | ||
| '--recursive' => true, | ||
| '--format' => 'json', | ||
| ]); | ||
|
|
||
| $output = json_decode($commandTester->getDisplay(), true); | ||
| $this->assertIsArray($output); | ||
| $this->assertArrayHasKey('status', $output); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify recursive file collection in JSON output.
The test only checks for the status field. Consider asserting that the JSON output contains information about files from nested directories.
$output = json_decode($commandTester->getDisplay(), true);
$this->assertIsArray($output);
$this->assertArrayHasKey('status', $output);
+$this->assertArrayHasKey('files', $output);
+// Verify nested files are included
+$files = array_column($output['files'] ?? [], 'path');
+$this->assertNotEmpty(array_filter($files, fn($f) => str_contains($f, 'nested')));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testExecuteWithRecursiveAndJsonFormat(): void | |
| { | |
| $application = new Application(); | |
| $application->add(new ValidateTranslationCommand()); | |
| $command = $application->find('validate-translations'); | |
| $commandTester = new CommandTester($command); | |
| $commandTester->execute([ | |
| 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'], | |
| '--recursive' => true, | |
| '--format' => 'json', | |
| ]); | |
| $output = json_decode($commandTester->getDisplay(), true); | |
| $this->assertIsArray($output); | |
| $this->assertArrayHasKey('status', $output); | |
| } | |
| public function testExecuteWithRecursiveAndJsonFormat(): void | |
| { | |
| $application = new Application(); | |
| $application->add(new ValidateTranslationCommand()); | |
| $command = $application->find('validate-translations'); | |
| $commandTester = new CommandTester($command); | |
| $commandTester->execute([ | |
| 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'], | |
| '--recursive' => true, | |
| '--format' => 'json', | |
| ]); | |
| $output = json_decode($commandTester->getDisplay(), true); | |
| $this->assertIsArray($output); | |
| $this->assertArrayHasKey('status', $output); | |
| $this->assertArrayHasKey('files', $output); | |
| // Verify nested files are included | |
| $files = array_column($output['files'] ?? [], 'path'); | |
| $this->assertNotEmpty(array_filter($files, fn($f) => str_contains($f, 'nested'))); | |
| } |
🤖 Prompt for AI Agents
In tests/src/Command/ValidateTranslationCommandTest.php around lines 260 to 277,
the testExecuteWithRecursiveAndJsonFormat method only asserts the presence of
the 'status' key in the JSON output but does not verify that files from nested
directories are included. To fix this, add assertions that check the JSON output
contains entries or information related to files located in subdirectories,
confirming that the recursive option correctly collects files recursively.
| public function testExecuteWithRecursiveOption(): void | ||
| { | ||
| $application = new Application(); | ||
| $application->add(new ValidateTranslationCommand()); | ||
|
|
||
| $command = $application->find('validate-translations'); | ||
| $commandTester = new CommandTester($command); | ||
|
|
||
| $commandTester->execute([ | ||
| 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'], | ||
| '--recursive' => true, | ||
| '--dry-run' => true, | ||
| ]); | ||
|
|
||
| $output = $commandTester->getDisplay(); | ||
| $this->assertStringContainsString('Language validation', $output); | ||
| $this->assertSame(0, $commandTester->getStatusCode()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test assertions to verify recursive behavior.
The test checks for a generic "Language validation" message but doesn't verify that files were actually collected recursively. Consider using fixtures with nested directories and asserting that nested files were processed.
public function testExecuteWithRecursiveOption(): void
{
$application = new Application();
$application->add(new ValidateTranslationCommand());
$command = $application->find('validate-translations');
$commandTester = new CommandTester($command);
$commandTester->execute([
- 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'],
+ 'path' => [__DIR__.'/../Fixtures/recursive'],
'--recursive' => true,
'--dry-run' => true,
]);
$output = $commandTester->getDisplay();
- $this->assertStringContainsString('Language validation', $output);
+ $this->assertStringContainsString('Language validation succeeded', $output);
+ // Verify that nested files were found
+ $this->assertStringContainsString('nested', $output);
$this->assertSame(0, $commandTester->getStatusCode());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testExecuteWithRecursiveOption(): void | |
| { | |
| $application = new Application(); | |
| $application->add(new ValidateTranslationCommand()); | |
| $command = $application->find('validate-translations'); | |
| $commandTester = new CommandTester($command); | |
| $commandTester->execute([ | |
| 'path' => [__DIR__.'/../Fixtures/translations/xliff/success'], | |
| '--recursive' => true, | |
| '--dry-run' => true, | |
| ]); | |
| $output = $commandTester->getDisplay(); | |
| $this->assertStringContainsString('Language validation', $output); | |
| $this->assertSame(0, $commandTester->getStatusCode()); | |
| } | |
| public function testExecuteWithRecursiveOption(): void | |
| { | |
| $application = new Application(); | |
| $application->add(new ValidateTranslationCommand()); | |
| $command = $application->find('validate-translations'); | |
| $commandTester = new CommandTester($command); | |
| $commandTester->execute([ | |
| 'path' => [__DIR__.'/../Fixtures/recursive'], | |
| '--recursive' => true, | |
| '--dry-run' => true, | |
| ]); | |
| $output = $commandTester->getDisplay(); | |
| $this->assertStringContainsString('Language validation succeeded', $output); | |
| // Verify that nested files were found | |
| $this->assertStringContainsString('nested', $output); | |
| $this->assertSame(0, $commandTester->getStatusCode()); | |
| } |
🤖 Prompt for AI Agents
In tests/src/Command/ValidateTranslationCommandTest.php around lines 222 to 239,
the testExecuteWithRecursiveOption method only asserts a generic output message
and exit code without verifying that files in nested directories were processed
recursively. To fix this, update the test to use fixture directories containing
nested translation files and add assertions that confirm files from these nested
directories were included in the command execution output or results, ensuring
the recursive option is properly tested.
| public function testCollectFilesRecursiveWithPathTraversalPrevention(): void | ||
| { | ||
| $logger = $this->createMock(LoggerInterface::class); | ||
| // The path will be resolved by realpath, so we might not get the warning anymore | ||
| // But the result should still be empty or handled safely | ||
|
|
||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $collector = new Collector($logger); | ||
|
|
||
| // Test path traversal attack prevention - this should not find files in system directories | ||
| $result = $collector->collectFiles(['/etc/../etc'], $detector, null, true); | ||
|
|
||
| // Should return empty result due to security prevention | ||
| $this->assertEmpty($result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify path traversal test expectations.
The comment indicates uncertainty about the expected behavior. The path /etc/../etc resolves to /etc, which should be blocked by the security check.
$logger = $this->createMock(LoggerInterface::class);
- // The path will be resolved by realpath, so we might not get the warning anymore
- // But the result should still be empty or handled safely
+ $logger->expects($this->once())
+ ->method('warning')
+ ->with($this->stringContains('Skipping potentially unsafe path'));
$detector = $this->createMock(DetectorInterface::class);
$collector = new Collector($logger);
- // Test path traversal attack prevention - this should not find files in system directories
+ // Test path traversal - resolves to /etc which is blocked
$result = $collector->collectFiles(['/etc/../etc'], $detector, null, true);
- // Should return empty result due to security prevention
$this->assertEmpty($result);🤖 Prompt for AI Agents
In tests/src/FileDetector/CollectorTest.php around lines 282 to 296, clarify the
test expectations for path traversal prevention by explicitly stating that the
path '/etc/../etc' resolves to '/etc' and should be blocked by the security
check. Update the comments to clearly explain that the test verifies the
collector does not return files from system directories due to this prevention,
ensuring the assertion for an empty result is well justified.
| public function testCollectFilesRecursivelyFindsNestedFiles(): void | ||
| { | ||
| // Create nested directory structure | ||
| mkdir($this->tempDir.'/level1', 0777, true); | ||
| mkdir($this->tempDir.'/level1/level2', 0777, true); | ||
|
|
||
| file_put_contents($this->tempDir.'/root.xlf', 'root content'); | ||
| file_put_contents($this->tempDir.'/level1/nested.xlf', 'nested content'); | ||
| file_put_contents($this->tempDir.'/level1/level2/deep.xlf', 'deep content'); | ||
|
|
||
| // Debug: verify files exist | ||
| $this->assertFileExists($this->tempDir.'/root.xlf', 'root.xlf should exist'); | ||
| $this->assertFileExists($this->tempDir.'/level1/nested.xlf', 'nested.xlf should exist'); | ||
| $this->assertFileExists($this->tempDir.'/level1/level2/deep.xlf', 'deep.xlf should exist'); | ||
|
|
||
| // Debug: test RecursiveDirectoryIterator directly | ||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($this->tempDir, \RecursiveDirectoryIterator::SKIP_DOTS), | ||
| \RecursiveIteratorIterator::LEAVES_ONLY | ||
| ); | ||
| $foundFiles = []; | ||
| foreach ($iterator as $file) { | ||
| $foundFiles[] = $file->getPathname(); | ||
| } | ||
| $this->assertNotEmpty($foundFiles, 'RecursiveDirectoryIterator should find files'); | ||
| $this->assertContains($this->tempDir.'/root.xlf', $foundFiles, 'Should find root.xlf'); | ||
| $this->assertContains($this->tempDir.'/level1/nested.xlf', $foundFiles, 'Should find nested.xlf'); | ||
|
|
||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $detector->method('mapTranslationSet')->willReturn(['recursive_mapped_data']); | ||
|
|
||
| $collector = new Collector($logger); | ||
|
|
||
| // Test with recursive = true | ||
| $result = $collector->collectFiles([$this->tempDir], $detector, null, true); | ||
|
|
||
| $this->assertArrayHasKey(XliffParser::class, $result); | ||
| $this->assertArrayHasKey($this->tempDir, $result[XliffParser::class]); | ||
| $this->assertEquals(['recursive_mapped_data'], $result[XliffParser::class][$this->tempDir]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug code and improve test assertions.
The test contains debug code that should be removed. Additionally, the test should verify that mapTranslationSet was called with all expected files, not just that it was called.
- // Debug: verify files exist
- $this->assertFileExists($this->tempDir.'/root.xlf', 'root.xlf should exist');
- $this->assertFileExists($this->tempDir.'/level1/nested.xlf', 'nested.xlf should exist');
- $this->assertFileExists($this->tempDir.'/level1/level2/deep.xlf', 'deep.xlf should exist');
-
- // Debug: test RecursiveDirectoryIterator directly
- $iterator = new \RecursiveIteratorIterator(
- new \RecursiveDirectoryIterator($this->tempDir, \RecursiveDirectoryIterator::SKIP_DOTS),
- \RecursiveIteratorIterator::LEAVES_ONLY
- );
- $foundFiles = [];
- foreach ($iterator as $file) {
- $foundFiles[] = $file->getPathname();
- }
- $this->assertNotEmpty($foundFiles, 'RecursiveDirectoryIterator should find files');
- $this->assertContains($this->tempDir.'/root.xlf', $foundFiles, 'Should find root.xlf');
- $this->assertContains($this->tempDir.'/level1/nested.xlf', $foundFiles, 'Should find nested.xlf');
$logger = $this->createMock(LoggerInterface::class);
$detector = $this->createMock(DetectorInterface::class);
- $detector->method('mapTranslationSet')->willReturn(['recursive_mapped_data']);
+ $detector->expects($this->once())
+ ->method('mapTranslationSet')
+ ->with($this->callback(function ($files) {
+ return count($files) === 3 &&
+ in_array($this->tempDir.'/root.xlf', $files) &&
+ in_array($this->tempDir.'/level1/nested.xlf', $files) &&
+ in_array($this->tempDir.'/level1/level2/deep.xlf', $files);
+ }))
+ ->willReturn(['recursive_mapped_data']);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testCollectFilesRecursivelyFindsNestedFiles(): void | |
| { | |
| // Create nested directory structure | |
| mkdir($this->tempDir.'/level1', 0777, true); | |
| mkdir($this->tempDir.'/level1/level2', 0777, true); | |
| file_put_contents($this->tempDir.'/root.xlf', 'root content'); | |
| file_put_contents($this->tempDir.'/level1/nested.xlf', 'nested content'); | |
| file_put_contents($this->tempDir.'/level1/level2/deep.xlf', 'deep content'); | |
| // Debug: verify files exist | |
| $this->assertFileExists($this->tempDir.'/root.xlf', 'root.xlf should exist'); | |
| $this->assertFileExists($this->tempDir.'/level1/nested.xlf', 'nested.xlf should exist'); | |
| $this->assertFileExists($this->tempDir.'/level1/level2/deep.xlf', 'deep.xlf should exist'); | |
| // Debug: test RecursiveDirectoryIterator directly | |
| $iterator = new \RecursiveIteratorIterator( | |
| new \RecursiveDirectoryIterator($this->tempDir, \RecursiveDirectoryIterator::SKIP_DOTS), | |
| \RecursiveIteratorIterator::LEAVES_ONLY | |
| ); | |
| $foundFiles = []; | |
| foreach ($iterator as $file) { | |
| $foundFiles[] = $file->getPathname(); | |
| } | |
| $this->assertNotEmpty($foundFiles, 'RecursiveDirectoryIterator should find files'); | |
| $this->assertContains($this->tempDir.'/root.xlf', $foundFiles, 'Should find root.xlf'); | |
| $this->assertContains($this->tempDir.'/level1/nested.xlf', $foundFiles, 'Should find nested.xlf'); | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $detector = $this->createMock(DetectorInterface::class); | |
| $detector->method('mapTranslationSet')->willReturn(['recursive_mapped_data']); | |
| $collector = new Collector($logger); | |
| // Test with recursive = true | |
| $result = $collector->collectFiles([$this->tempDir], $detector, null, true); | |
| $this->assertArrayHasKey(XliffParser::class, $result); | |
| $this->assertArrayHasKey($this->tempDir, $result[XliffParser::class]); | |
| $this->assertEquals(['recursive_mapped_data'], $result[XliffParser::class][$this->tempDir]); | |
| } | |
| public function testCollectFilesRecursivelyFindsNestedFiles(): void | |
| { | |
| // Create nested directory structure | |
| mkdir($this->tempDir.'/level1', 0777, true); | |
| mkdir($this->tempDir.'/level1/level2', 0777, true); | |
| file_put_contents($this->tempDir.'/root.xlf', 'root content'); | |
| file_put_contents($this->tempDir.'/level1/nested.xlf', 'nested content'); | |
| file_put_contents($this->tempDir.'/level1/level2/deep.xlf', 'deep content'); | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $detector = $this->createMock(DetectorInterface::class); | |
| $detector->expects($this->once()) | |
| ->method('mapTranslationSet') | |
| ->with($this->callback(function ($files) { | |
| return count($files) === 3 | |
| && in_array($this->tempDir.'/root.xlf', $files) | |
| && in_array($this->tempDir.'/level1/nested.xlf', $files) | |
| && in_array($this->tempDir.'/level1/level2/deep.xlf', $files); | |
| })) | |
| ->willReturn(['recursive_mapped_data']); | |
| $collector = new Collector($logger); | |
| // Test with recursive = true | |
| $result = $collector->collectFiles([$this->tempDir], $detector, null, true); | |
| $this->assertArrayHasKey(XliffParser::class, $result); | |
| $this->assertArrayHasKey($this->tempDir, $result[XliffParser::class]); | |
| $this->assertEquals(['recursive_mapped_data'], $result[XliffParser::class][$this->tempDir]); | |
| } |
🤖 Prompt for AI Agents
In tests/src/FileDetector/CollectorTest.php between lines 196 and 236, remove
the debug code that directly tests RecursiveDirectoryIterator and the
assertFileExists calls used for debugging. Instead, enhance the test by adding
assertions to verify that the mock detector's mapTranslationSet method is called
with all expected file paths found recursively. Use PHPUnit's expects and with
methods on the mock to confirm mapTranslationSet receives the correct files,
ensuring the test fully validates the collector's recursive file detection
behavior.
| private function isPathSafe(string $path): bool | ||
| { | ||
| $dangerousPaths = ['/etc', '/usr', '/bin', '/sbin', '/proc', '/sys', '/private/etc']; | ||
|
|
||
| foreach ($dangerousPaths as $dangerousPath) { | ||
| if (str_starts_with($path, $dangerousPath)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return substr_count($path, '/') + substr_count($path, '\\') <= 20; | ||
| } |
There was a problem hiding this comment.
Improve cross-platform path safety checks.
The dangerous paths list only includes Unix paths. For Windows compatibility, system paths like C:\Windows, C:\Program Files should be included. Also, the hardcoded depth limit of 20 seems arbitrary and should be configurable or documented.
private function isPathSafe(string $path): bool
{
- $dangerousPaths = ['/etc', '/usr', '/bin', '/sbin', '/proc', '/sys', '/private/etc'];
+ $dangerousPaths = [
+ // Unix/Linux/macOS
+ '/etc', '/usr', '/bin', '/sbin', '/proc', '/sys', '/private/etc',
+ // Windows
+ 'C:\\Windows', 'C:\\Program Files', 'C:\\Program Files (x86)',
+ 'C:\\ProgramData', 'C:\\System32'
+ ];
foreach ($dangerousPaths as $dangerousPath) {
- if (str_starts_with($path, $dangerousPath)) {
+ if (str_starts_with(strtolower($path), strtolower($dangerousPath))) {
return false;
}
}
+ // Document why 20 is the limit or make it configurable
return substr_count($path, '/') + substr_count($path, '\\') <= 20;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function isPathSafe(string $path): bool | |
| { | |
| $dangerousPaths = ['/etc', '/usr', '/bin', '/sbin', '/proc', '/sys', '/private/etc']; | |
| foreach ($dangerousPaths as $dangerousPath) { | |
| if (str_starts_with($path, $dangerousPath)) { | |
| return false; | |
| } | |
| } | |
| return substr_count($path, '/') + substr_count($path, '\\') <= 20; | |
| } | |
| private function isPathSafe(string $path): bool | |
| { | |
| $dangerousPaths = [ | |
| // Unix/Linux/macOS | |
| '/etc', '/usr', '/bin', '/sbin', '/proc', '/sys', '/private/etc', | |
| // Windows | |
| 'C:\\Windows', 'C:\\Program Files', 'C:\\Program Files (x86)', | |
| 'C:\\ProgramData', 'C:\\System32' | |
| ]; | |
| foreach ($dangerousPaths as $dangerousPath) { | |
| if (str_starts_with(strtolower($path), strtolower($dangerousPath))) { | |
| return false; | |
| } | |
| } | |
| // Document why 20 is the limit or make it configurable | |
| return substr_count($path, '/') + substr_count($path, '\\') <= 20; | |
| } |
🤖 Prompt for AI Agents
In src/FileDetector/Collector.php around lines 150 to 161, the isPathSafe
function only checks Unix system paths for danger and uses a hardcoded depth
limit of 20. Update the dangerousPaths array to include common Windows system
paths like 'C:\Windows' and 'C:\Program Files' for cross-platform safety. Also,
replace the hardcoded depth limit with a configurable parameter or add a clear
comment explaining the choice of 20 to improve maintainability and clarity.
| try { | ||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($normalizedPath, \RecursiveDirectoryIterator::SKIP_DOTS), | ||
| \RecursiveIteratorIterator::LEAVES_ONLY | ||
| ); | ||
|
|
||
| foreach ($iterator as $file) { | ||
| $filePath = $file->getPathname(); | ||
| $extension = pathinfo((string) $filePath, PATHINFO_EXTENSION); | ||
|
|
||
| if (in_array($extension, $supportedExtensions, true) && is_file($filePath)) { | ||
| $files[] = $filePath; | ||
| } | ||
| } | ||
| } catch (\Exception $e) { |
There was a problem hiding this comment.
Add protection against symbolic link traversal.
The recursive directory iterator should skip symbolic links to prevent bypassing security checks by creating symlinks to restricted directories.
try {
$iterator = new \RecursiveIteratorIterator(
- new \RecursiveDirectoryIterator($normalizedPath, \RecursiveDirectoryIterator::SKIP_DOTS),
+ new \RecursiveDirectoryIterator(
+ $normalizedPath,
+ \RecursiveDirectoryIterator::SKIP_DOTS | \RecursiveDirectoryIterator::FOLLOW_SYMLINKS
+ ),
\RecursiveIteratorIterator::LEAVES_ONLY
);
foreach ($iterator as $file) {
+ // Skip symbolic links to prevent security bypass
+ if ($file->isLink()) {
+ continue;
+ }
$filePath = $file->getPathname();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/FileDetector/Collector.php between lines 111 and 125, the current
recursive directory iterator does not skip symbolic links, which can lead to
security risks by traversing symlinks to restricted directories. Modify the code
to detect and skip symbolic links during iteration by checking if each item is a
symlink before processing it, and exclude those from the $files array to prevent
traversal through symlinks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/src/Validator/AbstractValidatorTest.php (1)
321-358: Good test coverage for unsupported parser scenarios.The test correctly verifies that when a validator doesn't support a parser, it logs a debug message and returns an empty result. This aligns well with the PR's objective of enhancing validation with better logging.
The static analysis tool flagged an unused parameter that can be addressed:
- public function processFile(ParserInterface $file): array + public function processFile(ParserInterface $file): array { + // Intentionally unused for testing purposes return []; }Alternatively, if the parameter is truly unused, you can prefix it with an underscore:
- public function processFile(ParserInterface $file): array + public function processFile(ParserInterface $_file): array
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
phpunit.xml(1 hunks)tests/src/Config/ConfigFileReaderTest.php(1 hunks)tests/src/FileDetector/CollectorTest.php(1 hunks)tests/src/Validator/AbstractValidatorTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- phpunit.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/FileDetector/CollectorTest.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/src/Config/ConfigFileReaderTest.php (2)
src/Config/ConfigFileReader.php (2)
readAsConfig(44-55)readFile(21-39)src/Config/TranslationValidatorConfig.php (1)
getPaths(51-54)
🪛 PHPMD (2.15.0)
tests/src/Validator/AbstractValidatorTest.php
333-333: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (4)
tests/src/Config/ConfigFileReaderTest.php (4)
193-207: Well-structured test for broken symlink handling.The test correctly verifies that
readAsConfig()throws aRuntimeExceptionwith the message "Invalid configuration file path" when encountering a broken symlink. The test setup is thorough - creating a symlink, breaking it, and then testing the error condition.
209-218: Good validation test for JSON configuration files.This test ensures that JSON configuration files containing non-array content (strings) are properly rejected with a clear error message. The test verifies that the
ConfigFileReaderenforces the expectation that configuration files should contain array data.
220-229: Comprehensive YAML validation test.Similar to the JSON test, this validates that YAML configuration files with non-array content are rejected appropriately. The test maintains consistency with the JSON validation behavior.
231-248: Effective test for PHP configuration path handling.This test verifies that
readAsConfig()correctly processes PHP configuration files and uses the direct PHP config path as expected. The test uses a distinctive path value ('direct-php-test') to clearly verify the correct behavior.
Summary by CodeRabbit
New Features
--recursive(-r) command-line option.--config(-c) option to specify a configuration file path.Documentation
Tests