Conversation
… modes for validation results
…ribution and formatting methods
… introduce renderer interface
WalkthroughThe changes refactor the CLI rendering logic for validation results to support both compact and verbose output modes, introducing new methods for formatting, grouping, and displaying issues. Validator classes and their interface are extended with methods for issue message formatting, file-based grouping, and detailed output, and corresponding tests are updated or added. Legacy renderer classes and interfaces were removed and replaced by a new renderer interface and factory. Tests were refactored to align with the new output formats and interface methods. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidationResultCliRenderer as CLI Renderer
participant Validator
participant Output
User->>CLI Renderer: render(validationResult)
CLI Renderer->>CLI Renderer: Check verbosity level
alt Verbose mode
CLI Renderer->>Validator: distributeIssuesForDisplay(fileSet)
CLI Renderer->>CLI Renderer: sortValidatorGroupsBySeverity()
loop For each file
CLI Renderer->>Output: print file path
loop For each validator group
CLI Renderer->>Output: print validator name (bold)
CLI Renderer->>Output: print issues (formatted without prefix)
alt Validator supports detailed output
CLI Renderer->>Validator: renderDetailedOutput(output, issues)
Validator->>Output: print detailed table
end
end
end
else Compact mode
CLI Renderer->>Validator: distributeIssuesForDisplay(fileSet)
CLI Renderer->>CLI Renderer: sortIssuesBySeverity()
loop For each file
CLI Renderer->>Output: print file path
CLI Renderer->>Output: print issues (with validator prefix)
end
CLI Renderer->>Output: print verbose mode hint if failures exist
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (11)
✨ 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: 2
🧹 Nitpick comments (2)
src/Validator/DuplicateValuesValidator.php (1)
127-144: Consider removing unused parameter or adding verbose behavior.The method implementation is solid, but the
$isVerboseparameter is unused. Consider either:
- Remove the parameter if verbose behavior isn't needed
- Add different formatting for verbose mode (e.g., more detailed output)
For consistency, if verbose mode isn't needed, apply this diff:
- public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string + public function formatIssueMessage(Issue $issue, string $prefix = ''): stringsrc/Validator/DuplicateKeysValidator.php (1)
89-106: Consider removing unused parameter or adding verbose behavior.The method implementation correctly formats duplicate key issues with appropriate color coding. However, the
$isVerboseparameter is unused. For consistency with the interface, consider either:
- Remove the parameter if verbose behavior isn't needed
- Add different formatting for verbose mode
If verbose behavior isn't needed, apply this diff:
- public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string + public function formatIssueMessage(Issue $issue, string $prefix = ''): string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Result/ValidationResultCliRenderer.php(2 hunks)src/Validator/AbstractValidator.php(2 hunks)src/Validator/DuplicateKeysValidator.php(2 hunks)src/Validator/DuplicateValuesValidator.php(1 hunks)src/Validator/MismatchValidator.php(2 hunks)src/Validator/ResultType.php(1 hunks)src/Validator/SchemaValidator.php(3 hunks)src/Validator/ValidatorInterface.php(2 hunks)tests/src/Command/ValidateTranslationCommandTest.php(1 hunks)tests/src/Result/ValidationResultCliRendererTest.php(5 hunks)tests/src/Result/ValidationResultTest.php(4 hunks)tests/src/Result/ValidationRunTest.php(2 hunks)tests/src/Validator/SchemaValidatorTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Validator/AbstractValidator.php (6)
src/FileDetector/FileSet.php (2)
FileSet(7-42)getPath(25-28)src/Result/Issue.php (3)
Issue(7-55)getDetails(28-31)getFile(20-23)src/Result/ValidationResultCliRenderer.php (1)
formatIssueMessage(266-271)src/Validator/MismatchValidator.php (5)
formatIssueMessage(142-175)resultTypeOnValidationFailure(137-140)distributeIssuesForDisplay(177-211)shouldShowDetailedOutput(213-216)renderDetailedOutput(218-290)src/Validator/ValidatorInterface.php (5)
formatIssueMessage(54-54)resultTypeOnValidationFailure(40-40)distributeIssuesForDisplay(61-61)shouldShowDetailedOutput(66-66)renderDetailedOutput(72-72)src/Validator/ResultType.php (2)
toString(38-45)toColorString(47-54)
🪛 PHPMD (2.15.0)
src/Validator/DuplicateKeysValidator.php
89-89: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/DuplicateValuesValidator.php
127-127: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/SchemaValidator.php
122-122: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/AbstractValidator.php
125-125: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultCliRendererTest.php
266-266: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/MismatchValidator.php
142-142: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultTest.php
287-287: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
311-311: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
311-311: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
466-466: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
481-481: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
481-481: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationRunTest.php
240-240: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
240-240: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
245-245: Avoid unused parameters such as '$fileSet'. (Unused Code Rules)
(UnusedFormalParameter)
255-255: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
255-255: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
317-317: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
317-317: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
341-341: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
341-341: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 GitHub Actions: CGL
src/Validator/AbstractValidator.php
[error] 147-147: EditorConfig: Trailing whitespaces found on line 147.
🪛 PHPStan (2.1.15)
src/Result/ValidationResultCliRenderer.php
257-257: Dead catch - ReflectionException is never thrown in the try block.
(catch.neverThrown)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (PHP 8.4, Symfony 7.x, Composer 2.4)
🔇 Additional comments (19)
src/Validator/ResultType.php (1)
38-54: LGTM! Clean enum methods implementation.The
toString()andtoColorString()methods are well-implemented using match expressions. The color mapping (green/yellow/red) provides intuitive visual feedback for different result types.tests/src/Validator/SchemaValidatorTest.php (1)
170-172: LGTM! Test updated to match new formatting.The test expectations correctly reflect the new capitalized result type format ("Error"/"Warning" instead of "ERROR"/"WARNING") introduced by the
ResultType::toString()method.tests/src/Command/ValidateTranslationCommandTest.php (1)
174-175: LGTM! Test assertion updated for new verbose output format.The assertion correctly checks for validator names ("MismatchValidator") in verbose mode, which aligns with the new output style that groups validators by file.
src/Validator/SchemaValidator.php (2)
96-96: Good improvement to error level formatting.Changing from uppercase to capitalized case ("Warning"/"Error") provides a more consistent and professional appearance in the output.
122-146: Well-implemented issue formatting method.The implementation properly handles schema validation errors with color-coded output and appropriate fallbacks. The method correctly iterates through error details and formats them consistently.
src/Validator/ValidatorInterface.php (1)
51-72: Excellent interface design for enhanced output capabilities.The new methods provide a well-structured API for issue formatting and detailed output rendering. The documentation is clear and the method signatures are appropriately designed.
src/Validator/AbstractValidator.php (1)
125-169: Good base implementations for the new interface methods.The abstract class provides sensible defaults:
formatIssueMessagecreates a simple colored message using the validator's result typedistributeIssuesForDisplaycorrectly builds full file paths and groups issues- The stub methods allow subclasses to opt-in to detailed output rendering
tests/src/Result/ValidationResultTest.php (1)
214-240: Test validators correctly implement the new interface methods.The anonymous validator classes have been properly updated with stub implementations of the new interface methods. This ensures the tests remain compatible with the extended interface.
Also applies to: 287-313, 393-419, 466-483
src/Validator/MismatchValidator.php (4)
137-140: Appropriate use of WARNING level for mismatches.Setting the result type to WARNING is a good choice for translation key mismatches, as these are typically not critical errors but important issues to address.
142-175: Excellent contextual message formatting.The method intelligently determines whether a key is missing from or present in other files and constructs a clear, informative message. The logic properly handles the current file context.
177-211: Smart issue distribution across affected files.The implementation correctly creates file-specific issues for each affected file, enabling proper grouping in the verbose output mode.
218-290: Comprehensive detailed output implementation.The table rendering provides a clear visual comparison of translation keys across files, with the current file positioned first for easy reference. The implementation handles all edge cases well.
tests/src/Result/ValidationRunTest.php (2)
239-258: Mock implementations look good for testing purposes.The new interface methods are correctly implemented in the mock validators. The unused parameters flagged by static analysis are acceptable for test mocks that need to satisfy the interface contract.
322-334: Correct implementation of issue distribution logic.The
distributeIssuesForDisplaymethod properly constructs full file paths and groups issues accordingly, which aligns with the new file-based grouping in the CLI renderer.tests/src/Result/ValidationResultCliRendererTest.php (2)
63-65: Test assertions correctly updated for new output format.The assertions now properly verify:
- File names are displayed in the output
- ERROR labels are shown
- Validator names are grouped in verbose mode
- The old "Validator:" prefix is removed in the new format
Also applies to: 138-141, 197-201
204-258: Excellent test coverage for the new output modes.The new tests effectively verify:
- Compact mode includes validator name in parentheses
- Verbose mode groups by validator name without parentheses in messages
- The verbose mode hint is displayed in compact mode
These tests ensure the refactored rendering logic works correctly for both output modes.
src/Result/ValidationResultCliRenderer.php (3)
28-37: Clean separation of rendering modes.The dispatch logic based on output verbosity is well-structured and maintains backward compatibility while adding the new verbose mode functionality.
47-98: Well-structured grouping and sorting implementation.The rendering methods effectively:
- Group issues by file path using the validator's
distributeIssuesForDisplay()method- Sort by severity to prioritize errors over warnings
- In verbose mode, further group by validator class and show detailed output
The implementation provides clear, organized output in both modes.
Also applies to: 109-174
282-284: Good UX improvement with verbose mode hint.Adding the hint about the
-voption when validation fails in compact mode helps users discover the more detailed output format.
| private function getValidatorSeverity(string $validatorClass): int | ||
| { | ||
| // For SchemaValidator, maintain current behavior (always ERROR) | ||
| if (str_contains($validatorClass, 'SchemaValidator')) { | ||
| return 1; // Error | ||
| } | ||
|
|
||
| // For other validators, use their ResultType to determine severity | ||
| try { | ||
| $reflection = new \ReflectionClass($validatorClass); | ||
| if ($reflection->isInstantiable()) { | ||
| $validator = $reflection->newInstance(); | ||
| if ($validator instanceof ValidatorInterface) { | ||
| $resultType = $validator->resultTypeOnValidationFailure(); | ||
| return $resultType === ResultType::ERROR ? 1 : 2; | ||
| } | ||
| } | ||
| } catch (\ReflectionException | \Throwable $e) { | ||
| // Fallback to error if we can't instantiate the validator | ||
| } | ||
|
|
||
| return $legacy; | ||
| // Fallback to error | ||
| return 1; // Error | ||
| } |
There was a problem hiding this comment.
Potential issue with validator instantiation.
The method attempts to instantiate validators without constructor arguments at line 251. This could fail if validators require dependencies (e.g., logger).
Consider passing null or checking constructor requirements:
- $validator = $reflection->newInstance();
+ $constructor = $reflection->getConstructor();
+ if ($constructor && $constructor->getNumberOfRequiredParameters() > 0) {
+ // Skip validators that require constructor arguments
+ return 1; // Default to error
+ }
+ $validator = $reflection->newInstance();Note: The PHPStan warning about ReflectionException being never thrown is a false positive - the ReflectionClass constructor can throw this exception.
📝 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 getValidatorSeverity(string $validatorClass): int | |
| { | |
| // For SchemaValidator, maintain current behavior (always ERROR) | |
| if (str_contains($validatorClass, 'SchemaValidator')) { | |
| return 1; // Error | |
| } | |
| // For other validators, use their ResultType to determine severity | |
| try { | |
| $reflection = new \ReflectionClass($validatorClass); | |
| if ($reflection->isInstantiable()) { | |
| $validator = $reflection->newInstance(); | |
| if ($validator instanceof ValidatorInterface) { | |
| $resultType = $validator->resultTypeOnValidationFailure(); | |
| return $resultType === ResultType::ERROR ? 1 : 2; | |
| } | |
| } | |
| } catch (\ReflectionException | \Throwable $e) { | |
| // Fallback to error if we can't instantiate the validator | |
| } | |
| return $legacy; | |
| // Fallback to error | |
| return 1; // Error | |
| } | |
| private function getValidatorSeverity(string $validatorClass): int | |
| { | |
| // For SchemaValidator, maintain current behavior (always ERROR) | |
| if (str_contains($validatorClass, 'SchemaValidator')) { | |
| return 1; // Error | |
| } | |
| // For other validators, use their ResultType to determine severity | |
| try { | |
| $reflection = new \ReflectionClass($validatorClass); | |
| if ($reflection->isInstantiable()) { | |
| - $validator = $reflection->newInstance(); | |
| + $constructor = $reflection->getConstructor(); | |
| + if ($constructor && $constructor->getNumberOfRequiredParameters() > 0) { | |
| + // Skip validators that require constructor arguments | |
| + return 1; // Default to error | |
| + } | |
| + $validator = $reflection->newInstance(); | |
| if ($validator instanceof ValidatorInterface) { | |
| $resultType = $validator->resultTypeOnValidationFailure(); | |
| return $resultType === ResultType::ERROR ? 1 : 2; | |
| } | |
| } | |
| } catch (\ReflectionException | \Throwable $e) { | |
| // Fallback to error if we can't instantiate the validator | |
| } | |
| // Fallback to error | |
| return 1; // Error | |
| } |
🧰 Tools
🪛 PHPStan (2.1.15)
257-257: Dead catch - ReflectionException is never thrown in the try block.
(catch.neverThrown)
🤖 Prompt for AI Agents
In src/Result/ValidationResultCliRenderer.php around lines 240 to 263, the
getValidatorSeverity method instantiates validators without constructor
arguments, which may fail if validators require dependencies. To fix this, check
the constructor of the validator class for required parameters and either pass
null or suitable default values when instantiating, or avoid instantiation if
dependencies cannot be satisfied. This ensures safe creation of validator
instances without causing errors.
- Upgrading friendsofphp/php-cs-fixer (v3.75.0 => v3.82.0) - Upgrading myclabs/deep-copy (1.13.1 => 1.13.3) - Upgrading phpunit/phpunit (12.2.5 => 12.2.6) - Upgrading roave/security-advisories (dev-latest b79895b => dev-latest a76f62e) - Upgrading symfony/console (v7.3.0 => v7.3.1) - Upgrading symfony/translation (v7.3.0 => v7.3.1) - Upgrading symfony/yaml (v7.3.0 => v7.3.1)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/Result/ValidationResult.php (1)
11-11: Fix PHP version compatibility issue with readonly classes.Same issue as in
ValidationResultJsonRenderer.php- readonly classes require PHP 8.2+ but the pipeline indicates an older PHP version is being used.src/Validator/AbstractValidator.php (1)
147-147: Remove trailing whitespace.- +src/Result/ValidationResultCliRenderer.php (1)
215-228: Address validator instantiation issue flagged in previous review.The existing review comment about potential validator instantiation failures is still valid. The reflection-based instantiation could fail if validators require constructor dependencies.
The method attempts to instantiate validators without constructor arguments at line 218. This could fail if validators require dependencies.
Consider checking constructor requirements before instantiation:
try { $reflection = new \ReflectionClass($validatorClass); if ($reflection->isInstantiable()) { + $constructor = $reflection->getConstructor(); + if ($constructor && $constructor->getNumberOfRequiredParameters() > 0) { + return 1; // Default to highest severity for safety + } $validator = $reflection->newInstance();
🧹 Nitpick comments (5)
src/Result/ValidationResultJsonRenderer.php (1)
12-14: Inconsistent readonly usage in constructor properties.The class is declared as
readonlybut the constructor properties are marked asprivateinstead ofprivate readonly. In a readonly class, all properties should be readonly by default, but for consistency and clarity, consider keeping the explicitreadonlymodifier.- private OutputInterface $output, - private bool $dryRun = false, - private bool $strict = false, + private readonly OutputInterface $output, + private readonly bool $dryRun = false, + private readonly bool $strict = false,src/Result/ValidationResult.php (1)
18-20: Inconsistent readonly usage in constructor properties.Similar to the other file, the class is declared as
readonlybut the constructor properties are marked asprivateinstead ofprivate readonly. For consistency, consider keeping the explicitreadonlymodifier.- private array $validatorInstances, - private ResultType $overallResult, - private array $validatorFileSetPairs = [], + private readonly array $validatorInstances, + private readonly ResultType $overallResult, + private readonly array $validatorFileSetPairs = [],src/Validator/MismatchValidator.php (1)
75-107: Remove unused parameter to clean up the method signature.The
$isVerboseparameter is not used in the method implementation.-public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string +public function formatIssueMessage(Issue $issue, string $prefix = ''): stringtests/src/Result/ValidationResultTest.php (1)
265-307: Consider extracting mock validator creation to reduce duplication.The anonymous validator classes have significant code duplication. Consider creating a helper method or base mock class to reduce repetition.
private function createMockValidator(bool $hasIssues = false, array $issues = []): ValidatorInterface { return new class($hasIssues, $issues) implements ValidatorInterface { public function __construct(private bool $hasIssues, private array $issues) {} public function hasIssues(): bool { return $this->hasIssues; } public function getIssues(): array { return $this->issues; } // ... other required methods with minimal implementations }; }src/Result/ValidationResultCliRenderer.php (1)
19-22: Make properties readonly or private.Since the class was intended to be readonly, consider making the properties readonly for immutability:
public function __construct( - private OutputInterface $output, - private InputInterface $input, - private bool $dryRun = false, - private bool $strict = false, + private readonly OutputInterface $output, + private readonly InputInterface $input, + private readonly bool $dryRun = false, + private readonly bool $strict = false, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/console.pngis excluded by!**/*.png
📒 Files selected for processing (31)
src/Result/CliRenderer.php(0 hunks)src/Result/JsonRenderer.php(0 hunks)src/Result/Output.php(1 hunks)src/Result/RendererFactory.php(0 hunks)src/Result/RendererInterface.php(0 hunks)src/Result/ValidationResult.php(2 hunks)src/Result/ValidationResultCliRenderer.php(2 hunks)src/Result/ValidationResultJsonRenderer.php(1 hunks)src/Result/ValidationResultRendererFactory.php(1 hunks)src/Result/ValidationResultRendererInterface.php(1 hunks)src/Result/ValidationRun.php(1 hunks)src/Utility/PathUtility.php(1 hunks)src/Validator/AbstractValidator.php(2 hunks)src/Validator/DuplicateKeysValidator.php(2 hunks)src/Validator/DuplicateValuesValidator.php(1 hunks)src/Validator/MismatchValidator.php(3 hunks)src/Validator/SchemaValidator.php(2 hunks)src/Validator/ValidatorInterface.php(2 hunks)tests/src/Result/CliRendererTest.php(0 hunks)tests/src/Result/JsonRendererTest.php(0 hunks)tests/src/Result/ValidationResultCliRendererTest.php(5 hunks)tests/src/Result/ValidationResultRendererFactoryTest.php(1 hunks)tests/src/Result/ValidationResultTest.php(4 hunks)tests/src/Result/ValidationRunTest.php(2 hunks)tests/src/Utility/PathUtilityTest.php(2 hunks)tests/src/Validator/AbstractValidatorTest.php(1 hunks)tests/src/Validator/DuplicateKeysValidatorTest.php(1 hunks)tests/src/Validator/DuplicateValuesValidatorTest.php(1 hunks)tests/src/Validator/MismatchValidatorTest.php(1 hunks)tests/src/Validator/SchemaValidatorTest.php(1 hunks)tests/src/Validator/ValidatorInterfaceTest.php(1 hunks)
💤 Files with no reviewable changes (6)
- src/Result/RendererInterface.php
- src/Result/RendererFactory.php
- tests/src/Result/JsonRendererTest.php
- src/Result/CliRenderer.php
- src/Result/JsonRenderer.php
- tests/src/Result/CliRendererTest.php
✅ Files skipped from review due to trivial changes (1)
- src/Result/ValidationResultRendererInterface.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Validator/ValidatorInterface.php
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/src/Utility/PathUtilityTest.php (1)
src/Utility/PathUtility.php (2)
PathUtility(7-29)normalizeFolderPath(9-28)
tests/src/Validator/ValidatorInterfaceTest.php (4)
src/FileDetector/FileSet.php (1)
FileSet(7-42)src/Result/Issue.php (1)
Issue(7-55)src/Validator/AbstractValidator.php (1)
AbstractValidator(14-177)src/Validator/ValidatorInterface.php (8)
formatIssueMessage(46-46)addIssue(41-41)distributeIssuesForDisplay(53-53)shouldShowDetailedOutput(58-58)getShortName(70-70)processFile(17-17)supportsParser(30-30)resultTypeOnValidationFailure(32-32)
tests/src/Validator/AbstractValidatorTest.php (4)
src/Validator/ValidatorInterface.php (1)
getShortName(70-70)tests/src/Result/ValidationRunTest.php (2)
getShortName(250-253)getShortName(333-336)tests/src/Result/ValidationResultTest.php (4)
getShortName(234-237)getShortName(304-307)getShortName(407-410)getShortName(467-470)src/Validator/AbstractValidator.php (1)
getShortName(171-176)
src/Result/ValidationResultJsonRenderer.php (2)
src/Result/ValidationResult.php (1)
__construct(17-22)src/Result/ValidationResultCliRenderer.php (1)
__construct(18-25)
🪛 GitHub Actions: Tests
src/Result/ValidationRun.php
[error] 12-12: ParseError: syntax error, unexpected token "readonly", expecting end of file
src/Result/ValidationResult.php
[error] 11-11: ParseError: syntax error, unexpected token "readonly", expecting end of file
tests/src/Result/ValidationResultRendererFactoryTest.php
[error] 27-107: ParseError: syntax error, unexpected token "readonly", expecting end of file in ValidationResultCliRenderer.php and ValidationResultJsonRenderer.php
src/Result/ValidationResultJsonRenderer.php
[error] 9-9: ParseError: syntax error, unexpected token "readonly", expecting end of file
src/Result/ValidationResultCliRenderer.php
[error] 14-14: ParseError: syntax error, unexpected token "readonly", expecting end of file
tests/src/Result/ValidationResultCliRendererTest.php
[error] 29-29: ParseError: syntax error, unexpected token "readonly", expecting end of file in ValidationResultCliRenderer.php
tests/src/Result/ValidationResultTest.php
[error] 21-480: ParseError: syntax error, unexpected token "readonly", expecting end of file in ValidationResult.php
tests/src/Result/ValidationRunTest.php
[error] 25-172: ParseError: syntax error, unexpected token "readonly", expecting end of file in ValidationRun.php
🪛 PHPMD (2.15.0)
src/Validator/SchemaValidator.php
49-49: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/DuplicateKeysValidator.php
34-34: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Validator/ValidatorInterfaceTest.php
117-117: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
135-135: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/MismatchValidator.php
75-75: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultCliRendererTest.php
366-366: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/AbstractValidator.php
125-125: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
src/Validator/DuplicateValuesValidator.php
61-61: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultTest.php
275-275: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
300-300: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
300-300: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
448-448: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
463-463: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
463-463: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationRunTest.php
231-231: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
231-231: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
236-236: Avoid unused parameters such as '$fileSet'. (Unused Code Rules)
(UnusedFormalParameter)
246-246: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
246-246: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
304-304: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
304-304: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
329-329: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
329-329: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (38)
src/Utility/PathUtility.php (1)
13-27: LGTM! Path normalization improvements are well-implemented.The changes consistently remove trailing directory separators from normalized paths in both success and failure cases of
realpath(). The logic correctly handles:
- Removal of trailing separators from input paths
- Temporary addition of separator only for comparison (line 23) without affecting the stored normalized path
- Consistent relative path calculation without trailing separators
This standardization will improve consistency across the codebase.
tests/src/Utility/PathUtilityTest.php (1)
43-84: LGTM! Test assertions correctly updated for normalized path behavior.The test updates consistently align with the
PathUtility::normalizeFolderPath()changes to remove trailing directory separators. All test cases now expect normalized paths without trailing slashes, which properly validates the updated implementation.tests/src/Validator/AbstractValidatorTest.php (1)
62-65: LGTM! Validator interface compliance correctly implemented.The
getShortName()method implementation aligns with the updatedValidatorInterfacerequirements and follows the same pattern as other test validator classes in the codebase (returning the fully qualified class name).src/Result/Output.php (1)
33-41: Verified: ValidationResultRendererFactory & create() method exist with correct signature — Approving changesThe factory class
ValidationResultRendererFactoryand itscreatemethod have been confirmed to match the expected parameters and return type. Using the factory pattern for renderer instantiation is appropriate and improves architectural clarity. No further action required.src/Result/ValidationResultJsonRenderer.php (1)
18-20: LGTM: PHPDoc annotation for JsonException is appropriate.The addition of the
@throws \JsonExceptionannotation properly documents the exception that can be thrown by thejson_encodefunction with theJSON_THROW_ON_ERRORflag.tests/src/Result/ValidationResultRendererFactoryTest.php (1)
15-112: LGTM: Comprehensive test coverage for the factory method.The test class provides good coverage of the
ValidationResultRendererFactory::createmethod, testing both CLI and JSON renderer creation with various flag combinations. The use of mocked dependencies is appropriate for unit testing.src/Result/ValidationResultRendererFactory.php (1)
10-24: LGTM: Clean factory implementation with appropriate use of match expression.The factory class follows the factory pattern well, using a match expression to create the appropriate renderer based on the format type. The method signature is clear and the implementation is straightforward.
src/Validator/DuplicateKeysValidator.php (1)
42-49: LGTM: Good message formatting with proper color coding.The message formatting logic correctly uses the result type for color coding and provides clear, descriptive messages for duplicate keys. The loop structure and string formatting are appropriate.
src/Result/ValidationResult.php (1)
7-7: LGTM: Import optimization for FileSet class.The addition of the
FileSetimport improves code readability by avoiding fully qualified class names in the type annotations.tests/src/Validator/ValidatorInterfaceTest.php (3)
16-26: Well-structured test for default message formatting.The test correctly verifies that the formatted message contains the expected error level, message content, and color formatting tags.
51-71: Good test coverage for issue distribution functionality.The test properly verifies that issues are correctly distributed by file path, including proper path construction and issue association.
73-86: Important edge case handling for empty filenames.This test ensures that issues with empty filenames are properly skipped during distribution, which is crucial for avoiding invalid file paths.
src/Validator/SchemaValidator.php (1)
49-73: Well-implemented message formatting with comprehensive error handling.The
formatIssueMessagemethod properly handles:
- Multiple error types with appropriate color coding
- Optional line numbers and error codes
- Fallback for empty error details
- Proper string formatting with color tags
The unused
$isVerboseparameter is part of the interface contract and may be used by other validators.src/Validator/DuplicateValuesValidator.php (1)
61-78: Clear and informative duplicate value message formatting.The implementation correctly formats duplicate value issues by:
- Listing all affected keys with proper punctuation
- Using appropriate color coding based on result type (WARNING)
- Validating data structure before processing
- Providing clear, actionable error messages
The unused
$isVerboseparameter is part of the interface contract.src/Validator/AbstractValidator.php (3)
125-136: Solid default implementation for issue message formatting.The default
formatIssueMessageimplementation provides:
- Proper color coding based on result type
- Fallback message for missing details
- Consistent formatting with level and prefix support
The unused
$isVerboseparameter is part of the interface contract and may be used by subclasses.
138-159: Robust issue distribution with proper path handling.The
distributeIssuesForDisplaymethod correctly:
- Skips issues with empty filenames
- Constructs full paths from FileSet base path
- Groups issues by file path
- Handles path normalization with
rtrim
171-176: Correct implementation of short name extraction.The
getShortNamemethod properly extracts the class name from the fully qualified class name usingstrrchrand handles edge cases where no namespace is present.tests/src/Validator/SchemaValidatorTest.php (4)
118-124: Clear test for parser support verification.The test correctly verifies that the SchemaValidator supports the expected XliffParser class.
126-152: Comprehensive test for error message formatting.The test properly verifies all aspects of error message formatting:
- Error level display
- Message content inclusion
- Line and code information
- Color formatting tags
181-214: Excellent test coverage for multiple error scenarios.This test ensures that multiple errors within a single issue are properly formatted with:
- All error messages included
- Correct line information for each error
- Proper newline separation between errors
216-233: Important edge case test for empty details.The test verifies that the formatter handles empty issue details gracefully with appropriate fallback messaging and error-level formatting.
tests/src/Validator/DuplicateKeysValidatorTest.php (1)
61-106: LGTM! Well-structured tests for the new interface methods.The new test methods properly cover the validator's interface requirements:
testSupportsParser()verifies parser supporttestGetShortName()tests the validator identifiertestResultTypeOnValidationFailure()confirms error severitytestFormatIssueMessage()validates output formatting with color codes and contentThe test structure is clean and focused on individual behaviors.
tests/src/Validator/DuplicateValuesValidatorTest.php (1)
206-247: LGTM! Comprehensive tests for the updated validator interface.The new tests effectively validate:
- Multi-parser support (Xliff and Yaml)
- Warning-level result type
- Message formatting with proper color coding and content structure
The test implementation follows the established pattern and provides good coverage.
tests/src/Validator/MismatchValidatorTest.php (1)
184-305: LGTM! Comprehensive test coverage for the enhanced validator interface.The test suite properly validates all new interface methods:
- Parser support verification
- Issue distribution by file path with proper path construction
- Detailed output rendering with table formatting
- Message formatting with appropriate warning styling
The tests demonstrate good understanding of the validator's behavior and provide thorough coverage of the new functionality.
src/Validator/MismatchValidator.php (2)
109-140: LGTM! Well-implemented issue distribution logic.The method correctly distributes issues by file path, creating file-specific Issue instances for proper display grouping. The path construction with
rtrimand proper concatenation is handled well.
142-210: LGTM! Comprehensive detailed output implementation.The table rendering logic is well-structured:
- Proper data collection and organization
- Current file prioritization in column order
- Clean table formatting with appropriate styling
- Handles missing values gracefully
The implementation provides valuable detailed view of translation key mismatches.
src/Result/ValidationResultCliRenderer.php (5)
40-96: LGTM - Well-structured compact output implementation.The compact output method properly groups issues by file, sorts by severity, and formats with validator name prefixes. The implementation correctly handles file path normalization and issue distribution.
98-168: LGTM - Comprehensive verbose output implementation.The verbose output method provides excellent organization by grouping issues first by file, then by validator. The detailed output support and proper indentation enhance readability.
175-202: LGTM - Proper severity-based sorting implementation.The sorting methods correctly prioritize issues by severity level, with appropriate handling for both individual issues and validator groups.
231-236: LGTM - Clean message formatting abstraction.The formatIssueMessage method properly handles verbose vs compact mode formatting by delegating to the validator's own formatting method.
246-248: LGTM - Helpful verbose mode hint.The addition of the verbose mode hint in compact output provides good user guidance for accessing more detailed information.
tests/src/Result/ValidationResultCliRendererTest.php (5)
202-234: LGTM - Comprehensive test coverage for output modes.The test properly validates the difference between compact and verbose output modes, checking for validator name prefixes and grouping behavior.
236-256: LGTM - Good verification of verbose mode hint.The test correctly verifies that the verbose mode hint is displayed in compact mode when failures occur.
280-327: LGTM - Thorough testing of sorting functionality.The tests for sorting methods use reflection to test private methods, which is appropriate for testing internal logic that affects output order.
344-360: LGTM - Good test coverage for message formatting.The test properly verifies that message formatting behaves differently in verbose vs compact modes.
365-387: LGTM - Well-implemented mock validator.The mock validator properly implements all new interface methods with realistic behavior for testing purposes. The unused parameters flagged by static analysis are expected in mock methods.
tests/src/Result/ValidationRunTest.php (2)
231-253: LGTM - Proper implementation of new interface methods.The MockValidatorWithoutIssues class correctly implements the new interface methods with appropriate default behavior for a validator with no issues.
304-336: LGTM - Realistic mock implementation with issue distribution.The MockValidatorWithIssues class properly implements issue distribution by file path, which aligns with the new renderer's expectations for grouping issues.
src/Result/ValidationRun.php
Outdated
| use Psr\Log\LoggerInterface; | ||
|
|
||
| class ValidationRun | ||
| readonly class ValidationRun |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix PHP version compatibility issue with readonly class.
The pipeline failure indicates a syntax error with the readonly keyword, suggesting the CI environment doesn't support PHP 8.2+ readonly classes.
🏁 Script executed:
#!/bin/bash
# Check the PHP version requirements and compatibility
echo "Checking composer.json for PHP version requirements:"
cat composer.json | jq '.require.php // "not specified"'
echo -e "\nChecking for other readonly class usage:"
rg -n "readonly class" --type php
echo -e "\nChecking CI configuration for PHP version:"
fd -e yml -e yaml . | xargs grep -l "php" | head -5 | xargs catLength of output: 875
Require PHP 8.2+ for readonly class usage
The readonly class feature was introduced in PHP 8.2, but your composer.json still requires "php": "^8.1", and the CI environment appears to be running PHP 8.1 (hence the syntax error). To fix:
• Update composer.json:
"require": {
- "php": "^8.1",
+ "php": "^8.2",
// …
}• Adjust your CI configuration (e.g. .github/workflows/*.yml) to install/run PHP 8.2 or later.
• No further changes are needed to the readonly properties inside these classes—removing the explicit readonly modifier on individual properties is correct, as the class‐level readonly makes them implicitly readonly.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 12-12: ParseError: syntax error, unexpected token "readonly", expecting end of file
🤖 Prompt for AI Agents
In src/Result/ValidationRun.php at line 12, the use of the `readonly class`
feature requires PHP 8.2 or higher, but your composer.json currently specifies
PHP 8.1 and the CI runs PHP 8.1, causing a syntax error. To fix this, update the
PHP version requirement in composer.json to "^8.2" or higher, and modify your CI
configuration files (such as .github/workflows/*.yml) to use PHP 8.2 or later.
No changes are needed to the readonly properties inside the class since the
class-level readonly already enforces immutability.
| public function supportsParser(): array | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| public function explain(): string | ||
| public function resultTypeOnValidationFailure(): ResultType | ||
| { | ||
| return ''; | ||
| return ResultType::ERROR; | ||
| } | ||
|
|
||
| public function supportsParser(): array | ||
| public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string | ||
| { | ||
| return []; | ||
| return "- <fg=red>ERROR</> {$prefix}Mock validation error"; | ||
| } | ||
|
|
||
| public function resultTypeOnValidationFailure(): ResultType | ||
| public function distributeIssuesForDisplay(FileSet $fileSet): array | ||
| { | ||
| return ResultType::ERROR; | ||
| $distribution = []; | ||
| foreach ($this->getIssues() as $issue) { | ||
| $fileName = $issue->getFile(); | ||
| if (!empty($fileName)) { | ||
| $basePath = rtrim($fileSet->getPath(), '/'); | ||
| $filePath = $basePath.'/'.$fileName; | ||
| $distribution[$filePath][] = $issue; | ||
| } | ||
| } | ||
|
|
||
| return $distribution; | ||
| } | ||
|
|
||
| public function shouldShowDetailedOutput(): bool | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| public function renderDetailedOutput(\Symfony\Component\Console\Output\OutputInterface $output, array $issues): void | ||
| { | ||
| } | ||
|
|
||
| public function getShortName(): string | ||
| { | ||
| return static::class; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address unused parameters and investigate pipeline failure.
The mock validator implementations have several unused parameters that should be addressed for cleaner code.
-public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string
+public function formatIssueMessage(Issue $issue, string $prefix = ''): string
-public function renderDetailedOutput(\Symfony\Component\Console\Output\OutputInterface $output, array $issues): void
+public function renderDetailedOutput(\Symfony\Component\Console\Output\OutputInterface $output, array $issues): void
{
// Empty implementation for test mock
}Additionally, there's a pipeline failure indicating a syntax error with "readonly". Please verify there are no syntax issues in the ValidationResult.php file.
🏁 Script executed:
#!/bin/bash
# Check for syntax errors in ValidationResult.php
find . -name "ValidationResult.php" -exec php -l {} \;
# Look for usage of readonly keyword that might be causing issues
rg "readonly" --type phpLength of output: 1554
Refactor test mocks & adjust CI PHP version
- Prefix unused parameters with an underscore to satisfy the interface and silence static-analysis warnings:
- In
tests/src/Result/ValidationResultTest.php:- public function formatIssueMessage(Issue $issue, string $prefix = '', bool $isVerbose = false): string + public function formatIssueMessage(Issue $issue, string $prefix = '', bool $_isVerbose = false): string
- public function renderDetailedOutput(\Symfony\Component\Console\Output\OutputInterface $output, array $issues): void + public function renderDetailedOutput(\Symfony\Component\Console\Output\OutputInterface $_output, array $_issues): void { // no-op }
- In
- The pipeline syntax errors aren’t in
ValidationResult.php(PHP –l reports no errors); they’re due to newreadonlyusage (a PHP 8.2 feature). You have two options:- Upgrade CI to PHP 8.2+ so
readonly classandprivate readonlyproperties are supported. - Remove/adjust
readonlydeclarations in:tests/src/Validator/AbstractValidatorTest.php(private readonly string $filePath)src/Result/ValidationResultCliRenderer.phpsrc/Result/ValidationResultJsonRenderer.phpsrc/Result/ValidationResult.phpsrc/Result/ValidationRun.php
- Upgrade CI to PHP 8.2+ so
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/src/Result/ValidationResultTest.php around lines 195 to 237, prefix all
unused function parameters with an underscore to comply with the interface and
suppress static analysis warnings. Additionally, to resolve pipeline syntax
errors caused by PHP 8.2 readonly features, either upgrade the CI environment to
PHP 8.2 or higher, or remove/modify readonly declarations in the specified files
including tests/src/Validator/AbstractValidatorTest.php and the various
src/Result/*.php files.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/src/Result/ValidationResultCliRendererTest.php (2)
365-382: Consider simplifying the complex mock setup.The mock validator setup is quite complex and may be brittle. Consider extracting common mock creation logic or using a factory method for different mock types.
private function createMockValidator(): ValidatorInterface|MockObject { $validator = $this->createMock(ValidatorInterface::class); $validator->method('resultTypeOnValidationFailure')->willReturn(ResultType::ERROR); - $validator->method('formatIssueMessage')->willReturnCallback(fn (Issue $issue, string $prefix = '', bool $isVerbose = false): string => $isVerbose ? "- <fg=red>ERROR</> Validation error" : "- <fg=red>ERROR</> {$prefix}Validation error"); - $validator->method('distributeIssuesForDisplay')->willReturnCallback(function (FileSet $fileSet) use ($validator): array { - $distribution = []; - foreach ($validator->getIssues() as $issue) { - $fileName = $issue->getFile(); - if (!empty($fileName)) { - $basePath = rtrim($fileSet->getPath(), '/'); - $filePath = $basePath.'/'.$fileName; - $distribution[$filePath][] = $issue; - } - } - - return $distribution; - }); + $this->setupMockValidatorMethods($validator); $validator->method('shouldShowDetailedOutput')->willReturn(false); $validator->method('renderDetailedOutput'); $validator->method('getShortName')->willReturn('MockObject_ValidatorInterface'); return $validator; } + +private function setupMockValidatorMethods(ValidatorInterface|MockObject $validator): void +{ + $validator->method('formatIssueMessage')->willReturnCallback( + fn (Issue $issue, string $prefix = '', bool $isVerbose = false): string => + $isVerbose ? "- <fg=red>ERROR</> Validation error" : "- <fg=red>ERROR</> {$prefix}Validation error" + ); + + $validator->method('distributeIssuesForDisplay')->willReturnCallback( + function (FileSet $fileSet) use ($validator): array { + $distribution = []; + foreach ($validator->getIssues() as $issue) { + $fileName = $issue->getFile(); + if (!empty($fileName)) { + $basePath = rtrim($fileSet->getPath(), '/'); + $filePath = $basePath.'/'.$fileName; + $distribution[$filePath][] = $issue; + } + } + return $distribution; + } + ); +}
280-342: Consider reducing reliance on reflection-based testing.While reflection-based testing of private methods provides thorough coverage, it can make refactoring more difficult. Consider whether these behaviors can be tested through public interfaces instead.
Testing private methods through reflection couples the tests tightly to the implementation. Consider whether you can test the sorting behavior through the public
rendermethod by verifying the order of items in the output instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
composer.json(0 hunks)src/Result/ValidationResultCliRenderer.php(2 hunks)src/Utility/PathUtility.php(2 hunks)tests/src/Result/ValidationResultCliRendererTest.php(5 hunks)
💤 Files with no reviewable changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Utility/PathUtility.php
- src/Result/ValidationResultCliRenderer.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/src/Result/ValidationResultCliRendererTest.php
366-366: Avoid unused parameters such as '$isVerbose'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (3)
tests/src/Result/ValidationResultCliRendererTest.php (3)
63-64: LGTM: Improved test assertions for specific content.The updated assertions now check for specific file names and error types, which provides better validation of the output format changes.
123-125: LGTM: Enhanced test data with actual Issue instances.Adding real Issue instances instead of empty arrays makes the tests more realistic and better reflects actual usage scenarios.
Also applies to: 176-179
202-234: Well-structured test for compact vs verbose mode comparison.This test effectively validates the different output formats between compact and verbose modes by creating separate outputs and comparing their content.
| public function testSortIssuesBySeverity(): void | ||
| { | ||
| $validator1 = $this->createMockValidator(); | ||
| $validator1->method('resultTypeOnValidationFailure')->willReturn(ResultType::WARNING); | ||
|
|
||
| $validator2 = $this->createMockValidator(); | ||
| $validator2->method('resultTypeOnValidationFailure')->willReturn(ResultType::ERROR); | ||
|
|
||
| $issue1 = new Issue('test1.xlf', ['warning'], 'TestParser', 'TestValidator'); | ||
| $issue2 = new Issue('test2.xlf', ['error'], 'TestParser', 'TestValidator'); | ||
|
|
||
| $fileIssues = [ | ||
| ['validator' => $validator1, 'issue' => $issue1], | ||
| ['validator' => $validator2, 'issue' => $issue2], | ||
| ]; | ||
|
|
||
| $reflection = new \ReflectionClass($this->renderer); | ||
| $method = $reflection->getMethod('sortIssuesBySeverity'); | ||
| $method->setAccessible(true); | ||
|
|
||
| $sorted = $method->invoke($this->renderer, $fileIssues); | ||
|
|
||
| // Test that sorting actually occurred - array should have 2 elements | ||
| $this->assertCount(2, $sorted); | ||
| $this->assertArrayHasKey('validator', $sorted[0]); | ||
| $this->assertArrayHasKey('issue', $sorted[0]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve sorting tests to verify actual sorting behavior.
The current sorting tests only verify that the structure is maintained but don't actually test that sorting occurs correctly. Consider testing with different severities to verify the sorting order.
public function testSortIssuesBySeverity(): void
{
$validator1 = $this->createMockValidator();
$validator1->method('resultTypeOnValidationFailure')->willReturn(ResultType::WARNING);
$validator2 = $this->createMockValidator();
$validator2->method('resultTypeOnValidationFailure')->willReturn(ResultType::ERROR);
$issue1 = new Issue('test1.xlf', ['warning'], 'TestParser', 'TestValidator');
$issue2 = new Issue('test2.xlf', ['error'], 'TestParser', 'TestValidator');
$fileIssues = [
['validator' => $validator1, 'issue' => $issue1],
['validator' => $validator2, 'issue' => $issue2],
];
$reflection = new \ReflectionClass($this->renderer);
$method = $reflection->getMethod('sortIssuesBySeverity');
$method->setAccessible(true);
$sorted = $method->invoke($this->renderer, $fileIssues);
- // Test that sorting actually occurred - array should have 2 elements
+ // Test that sorting actually occurred - errors should come before warnings
$this->assertCount(2, $sorted);
- $this->assertArrayHasKey('validator', $sorted[0]);
- $this->assertArrayHasKey('issue', $sorted[0]);
+ $this->assertSame(ResultType::ERROR, $sorted[0]['validator']->resultTypeOnValidationFailure());
+ $this->assertSame(ResultType::WARNING, $sorted[1]['validator']->resultTypeOnValidationFailure());
}Also applies to: 308-327
🤖 Prompt for AI Agents
In tests/src/Result/ValidationResultCliRendererTest.php around lines 280 to 306
and 308 to 327, the testSortIssuesBySeverity method currently only checks the
structure of the sorted array but does not verify that the issues are actually
sorted by severity. To fix this, enhance the test by asserting the order of the
issues based on their severity levels, ensuring that issues with higher severity
(e.g., ERROR) appear before lower severity (e.g., WARNING). Add assertions that
compare the severity of the first and second elements in the sorted array to
confirm correct sorting behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Result/ValidationResultCliRenderer.php (1)
215-228: Validator instantiation issue persists from previous reviews.The method still attempts to instantiate validators without constructor arguments, which could fail if validators require dependencies.
The previous review comment about this issue remains valid. Run the following script to verify if any validators require constructor arguments:
#!/bin/bash # Description: Check if any validators require constructor arguments echo "Checking validator classes for required constructor parameters..." # Find all validator classes ast-grep --pattern $'class $_ implements ValidatorInterface { $$$ }' # Check each validator's constructor rg -A 10 "class.*Validator.*implements.*ValidatorInterface" --type php | rg -A 10 "public function __construct"
🧹 Nitpick comments (1)
src/Result/ValidationResultCliRenderer.php (1)
209-229: Consider caching reflection results for performance.The
getValidatorSeveritymethod uses reflection for each validator instance, which could impact performance with many validators. Consider caching the severity results.Add a static cache to improve performance:
+ private static array $severityCache = []; + private function getValidatorSeverity(string $validatorClass): int { + if (isset(self::$severityCache[$validatorClass])) { + return self::$severityCache[$validatorClass]; + } + if (str_contains($validatorClass, 'SchemaValidator')) { - return 1; + return self::$severityCache[$validatorClass] = 1; } try { $reflection = new \ReflectionClass($validatorClass); if ($reflection->isInstantiable()) { $validator = $reflection->newInstance(); if ($validator instanceof ValidatorInterface) { $resultType = $validator->resultTypeOnValidationFailure(); - return ResultType::ERROR === $resultType ? 1 : 2; + return self::$severityCache[$validatorClass] = ResultType::ERROR === $resultType ? 1 : 2; } } } catch (\Throwable) { } - return 1; + return self::$severityCache[$validatorClass] = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Result/ValidationResult.php(2 hunks)src/Result/ValidationResultCliRenderer.php(2 hunks)src/Result/ValidationResultJsonRenderer.php(1 hunks)src/Result/ValidationRun.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Result/ValidationRun.php
- src/Result/ValidationResultJsonRenderer.php
- src/Result/ValidationResult.php
🔇 Additional comments (10)
src/Result/ValidationResultCliRenderer.php (10)
14-14: Class declaration correctly updated.The
readonlykeyword has been properly removed to fix the PHP version compatibility issue identified in previous reviews.
16-16: Property declaration correctly updated.The
readonlykeyword has been properly removed from the$ioproperty declaration.
19-22: Constructor parameters correctly updated.The
readonlykeyword has been properly removed from all constructor parameters.
29-33: Clean delegation based on verbosity level.The conditional rendering logic properly delegates to appropriate output methods based on the verbosity setting.
40-96: Well-structured compact output implementation.The compact output method effectively groups issues by file path and provides clean, sorted output. The implementation properly handles empty issues and uses appropriate formatting.
98-168: Comprehensive verbose output implementation.The verbose output method provides detailed, well-organized display by grouping issues first by file, then by validator. The integration with detailed output rendering enhances the user experience.
175-185: Effective issue sorting implementation.The sorting method provides consistent ordering based on validator severity, improving output readability.
192-202: Proper validator group sorting implementation.The sorting method ensures consistent ordering of validator groups by severity.
231-236: Clean message formatting abstraction.The method properly delegates formatting to the validator while handling prefix logic for different verbosity levels.
242-250: Enhanced summary with helpful user guidance.The addition of the verbose mode hint in the summary message improves user experience by guiding users to more detailed output when needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Result/ValidationResultCliRenderer.php (1)
215-229: Potential validator instantiation issue remains.The method still instantiates validators without constructor arguments at line 218, which could fail if validators require dependencies like a logger. This was flagged in a previous review but appears unresolved.
Consider adding constructor parameter checking:
- $validator = $reflection->newInstance(); + $constructor = $reflection->getConstructor(); + if ($constructor && $constructor->getNumberOfRequiredParameters() > 0) { + // Skip validators that require constructor arguments + return 1; // Default to error + } + $validator = $reflection->newInstance();tests/src/Result/ValidationResultCliRendererTest.php (2)
308-327: ****This test doesn't verify actual sorting behavior - it only checks that the structure is maintained. The past review comment already addressed this issue for the
testSortIssuesBySeveritymethod, and the same concern applies here.
366-366: ****The static analysis tool correctly identifies that the
$isVerboseparameter is unused in the callback. This issue has already been flagged in past review comments.
🧹 Nitpick comments (3)
tests/src/Result/ValidationResultCliRendererTest.php (3)
329-342: Expand test coverage for validator severity mapping.The test only covers two cases. Consider adding more test cases to verify the complete severity mapping logic, including different validator types and edge cases.
public function testGetValidatorSeverity(): void { $reflection = new \ReflectionClass($this->renderer); $method = $reflection->getMethod('getValidatorSeverity'); $method->setAccessible(true); // SchemaValidator should have priority 1 $result = $method->invoke($this->renderer, 'SchemaValidator'); $this->assertSame(1, $result); - // Other error validators should have priority 1 - $result = $method->invoke($this->renderer, 'SomeErrorValidator'); - $this->assertSame(1, $result); + // Test various validator types + $result = $method->invoke($this->renderer, 'DuplicateKeysValidator'); + $this->assertSame(2, $result); + + $result = $method->invoke($this->renderer, 'MismatchValidator'); + $this->assertSame(3, $result); + + $result = $method->invoke($this->renderer, 'UnknownValidator'); + $this->assertSame(4, $result); // Default priority }
354-359: Improve assertions for message formatting test.The current assertions only check for presence/absence of the validator name prefix. Consider adding more specific assertions to verify the complete message format.
// Test with validator name prefix (non-verbose) $result = $method->invoke($this->renderer, $validator, $issue, 'TestValidator', false); $this->assertStringContainsString('(TestValidator)', $result); +$this->assertStringContainsString('- <fg=red>ERROR</>', $result); +$this->assertStringContainsString('Validation error', $result); // Test verbose mode (no prefix) $result = $method->invoke($this->renderer, $validator, $issue, 'TestValidator', true); $this->assertStringNotContainsString('(TestValidator)', $result); +$this->assertStringContainsString('- <fg=red>ERROR</>', $result); +$this->assertStringContainsString('Validation error', $result);
367-379: Simplify mock callback implementation.The
distributeIssuesForDisplaycallback is overly complex for test purposes. Consider simplifying it to focus on the essential behavior being tested.- $validator->method('distributeIssuesForDisplay')->willReturnCallback(function (FileSet $fileSet) use ($validator): array { - $distribution = []; - foreach ($validator->getIssues() as $issue) { - $fileName = $issue->getFile(); - if (!empty($fileName)) { - $basePath = rtrim($fileSet->getPath(), '/'); - $filePath = $basePath.'/'.$fileName; - $distribution[$filePath][] = $issue; - } - } - - return $distribution; - }); + $validator->method('distributeIssuesForDisplay')->willReturnCallback(function (FileSet $fileSet) use ($validator): array { + $distribution = []; + foreach ($validator->getIssues() as $issue) { + $filePath = $fileSet->getPath() . '/' . $issue->getFile(); + $distribution[$filePath][] = $issue; + } + return $distribution; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Result/ValidationResultCliRenderer.php(3 hunks)src/Validator/AbstractValidator.php(2 hunks)src/Validator/DuplicateKeysValidator.php(2 hunks)src/Validator/DuplicateValuesValidator.php(1 hunks)src/Validator/MismatchValidator.php(3 hunks)src/Validator/SchemaValidator.php(2 hunks)src/Validator/ValidatorInterface.php(2 hunks)tests/src/Result/ValidationResultCliRendererTest.php(5 hunks)tests/src/Validator/ValidatorInterfaceTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Validator/DuplicateValuesValidator.php
- src/Validator/DuplicateKeysValidator.php
- src/Validator/ValidatorInterface.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Validator/AbstractValidator.php
166-166: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$issues'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Validator/ValidatorInterfaceTest.php
108-108: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
126-126: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (7)
tests/src/Validator/ValidatorInterfaceTest.php (1)
1-141: Comprehensive test coverage for validator interface methods.The test file provides excellent coverage of the new validator interface methods including
formatIssueMessage,distributeIssuesForDisplay,shouldShowDetailedOutput, andgetShortName. The test implementations are well-structured with clear assertions and proper edge case handling (e.g., skipping empty filenames intestDistributeIssuesForDisplaySkipsEmptyFilenames).src/Validator/SchemaValidator.php (1)
49-73: Well-implemented formatIssueMessage method.The method correctly handles schema validation errors by:
- Extracting message, line, code, and level information from error arrays
- Applying appropriate color coding (red for errors, yellow for warnings)
- Providing fallback handling for empty messages
- Formatting multiple errors with proper line breaks
src/Validator/AbstractValidator.php (1)
125-176: Solid implementation of new validator interface methods.The added methods provide excellent foundational support for the refactored validation result rendering:
formatIssueMessage: Correctly formats issues with appropriate color coding based on result typedistributeIssuesForDisplay: Properly builds full file paths and groups issues by fileshouldShowDetailedOutput/renderDetailedOutput: Provide sensible defaults for detailed output controlgetShortName: Correctly extracts class name without namespaceThe unused parameters in
renderDetailedOutputare acceptable as this is a default implementation that can be overridden by subclasses.src/Validator/MismatchValidator.php (1)
75-234: Comprehensive implementation of mismatch validation methods.The refactored methods provide excellent support for handling translation key mismatches:
formatIssueMessage: Correctly determines whether a key is missing from or present in other files and generates clear, descriptive messagesdistributeIssuesForDisplay: Properly creates file-specific issues for each affected filerenderDetailedOutput: Provides a comprehensive table view showing all keys and their values across filesresultTypeOnValidationFailure: Appropriately returns WARNING for mismatch issuesshouldShowDetailedOutput: Returns true to enable the detailed table viewThe logic for determining file relationships and generating appropriate messages is well-implemented.
src/Result/ValidationResultCliRenderer.php (3)
40-96: Well-implemented compact output rendering.The compact output mode effectively groups issues by file path and sorts them by severity. The use of
distributeIssuesForDisplayand proper path normalization ensures consistent presentation.
98-168: Excellent verbose output implementation.The verbose output mode provides clear hierarchical grouping by file path and validator, with appropriate indentation and detailed output support. The sorting by severity ensures consistent presentation order.
246-248: Helpful verbose mode hint in summary.The addition of the verbose mode hint in the summary message provides good user guidance when validation fails in compact mode.
…ormatting and remove legacy array method
…or improved test flexibility
Summary by CodeRabbit
New Features
Bug Fixes
Tests