feat: add KeyNamingConventionValidator with configurable naming conventions#46
feat: add KeyNamingConventionValidator with configurable naming conventions#46konradmichalik merged 11 commits intomainfrom
Conversation
|
""" WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ConfigFactory
participant ValidatorRegistry
participant ValidationRun
participant KeyNamingConventionValidator
User->>CLI: Run translation validation with config
CLI->>ConfigFactory: Load config (with validator-settings)
ConfigFactory-->>CLI: Return TranslationValidatorConfig
CLI->>ValidatorRegistry: Get available validators
ValidatorRegistry-->>CLI: Return validator classes
CLI->>ValidationRun: executeFor(files, validators, config)
loop For each validator
ValidationRun->>KeyNamingConventionValidator: Instantiate
ValidationRun->>KeyNamingConventionValidator: setConfig(config)
KeyNamingConventionValidator-->>ValidationRun: Ready with settings
end
ValidationRun->>KeyNamingConventionValidator: processFile(file)
KeyNamingConventionValidator-->>ValidationRun: Return issues (if any)
ValidationRun-->>CLI: Report results
CLI-->>User: Output validation report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
""" ✨ Finishing Touches🧪 Generate unit tests
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: 1
🧹 Nitpick comments (4)
src/Validator/KeyNamingConventionValidator.php (4)
80-82: Use parameterized logging instead of string concatenation.String concatenation in logging statements can cause unnecessary overhead even when the log level is disabled.
-$this->logger?->error( - 'The source file '.$file->getFileName().' is not valid.', -); +$this->logger?->error( + 'The source file {filename} is not valid.', + ['filename' => $file->getFileName()] +);
121-123: Use parameterized logging for warning messages.Similar to the previous comment, use parameterized logging for better performance and consistency.
-$this->logger?->warning( - 'Invalid convention in config: '.$validatorSettings['convention'].'. '.$e->getMessage(), -); +$this->logger?->warning( + 'Invalid convention in config: {convention}. {error}', + ['convention' => $validatorSettings['convention'], 'error' => $e->getMessage()] +);-$this->logger?->warning( - 'Invalid custom pattern in config: '.$validatorSettings['custom_pattern'].'. '.$e->getMessage(), -); +$this->logger?->warning( + 'Invalid custom pattern in config: {pattern}. {error}', + ['pattern' => $validatorSettings['custom_pattern'], 'error' => $e->getMessage()] +);Also applies to: 131-134
148-157: Avoid using error suppression operator for regex validation.Consider using
preg_last_error()or catching warnings more explicitly instead of the @ operator.-$result = @preg_match($pattern, ''); -if (false === $result) { - throw new InvalidArgumentException('Invalid regex pattern provided'); -} +// Clear any previous errors +preg_match('', ''); +$result = preg_match($pattern, ''); +if (false === $result || preg_last_error() !== PREG_NO_ERROR) { + throw new InvalidArgumentException('Invalid regex pattern provided'); +}
224-226: Cache array count to avoid repeated calculations.Calling
count()in the loop condition causes it to be evaluated on every iteration.-for ($i = 1; $i < count($parts); ++$i) { +$partsCount = count($parts); +for ($i = 1; $i < $partsCount; ++$i) { $result .= ucfirst(strtolower($parts[$i])); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.md(1 hunks)docs/validator-configuration.md(1 hunks)schema/translation-validator.schema.json(2 hunks)src/Command/ValidateTranslationCommand.php(2 hunks)src/Config/ConfigFactory.php(1 hunks)src/Config/TranslationValidatorConfig.php(3 hunks)src/Result/ValidationRun.php(3 hunks)src/Validator/KeyNamingConventionValidator.php(1 hunks)src/Validator/ValidatorRegistry.php(1 hunks)tests/src/Config/TranslationValidatorConfigTest.php(3 hunks)tests/src/Config/ValidatorSettingsConfigTest.php(1 hunks)tests/src/Fixtures/config/unsupported.txt(1 hunks)tests/src/Fixtures/config/validator-settings.json(1 hunks)tests/src/Fixtures/config/validator-settings.yaml(1 hunks)tests/src/Validator/KeyNamingConventionValidatorConfigTest.php(1 hunks)tests/src/Validator/KeyNamingConventionValidatorTest.php(1 hunks)tests/src/Validator/ValidatorRegistryTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/Validator/ValidatorRegistry.php (1)
src/Validator/KeyNamingConventionValidator.php (1)
KeyNamingConventionValidator(35-323)
src/Config/ConfigFactory.php (1)
src/Config/TranslationValidatorConfig.php (1)
setValidatorSettings(274-279)
src/Command/ValidateTranslationCommand.php (1)
src/Result/ValidationRun.php (1)
executeFor(44-97)
src/Result/ValidationRun.php (2)
src/Config/TranslationValidatorConfig.php (1)
TranslationValidatorConfig(26-341)src/Validator/KeyNamingConventionValidator.php (1)
setConfig(64-68)
tests/src/Validator/ValidatorRegistryTest.php (1)
src/Validator/KeyNamingConventionValidator.php (1)
KeyNamingConventionValidator(35-323)
🪛 GitHub Actions: CGL
tests/src/Fixtures/config/unsupported.txt
[error] 1-1: EditorConfigCLI: This file has no final new line given.
🪛 Gitleaks (8.27.2)
tests/src/Validator/KeyNamingConventionValidatorConfigTest.php
74-74: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 PHPStan (2.1.17)
tests/src/Config/ValidatorSettingsConfigTest.php
151-151: Call to method PHPUnit\Framework\Assert::assertSame() with array{new: 'value', additional: 'setting'} and array{initial: 'value'} will always evaluate to false.
(method.impossibleType)
🔇 Additional comments (29)
tests/src/Fixtures/config/validator-settings.yaml (1)
1-14: LGTM! Well-structured configuration fixture.This YAML fixture properly demonstrates the new validator-specific configuration capabilities, including:
- Path configuration for translation files
- Validator-specific settings for both
KeyNamingConventionValidatorandHtmlTagValidator- Global configuration options
The structure aligns well with the new schema and provides good test coverage for the configuration system.
src/Validator/ValidatorRegistry.php (1)
39-39: LGTM! Correctly adds the new validator to the registry.The
KeyNamingConventionValidatoris properly added to the available validators list. Based on the relevant code snippets, this validator implementsValidatorInterfaceand provides comprehensive functionality for validating translation key naming conventions.tests/src/Config/TranslationValidatorConfigTest.php (1)
200-200: LGTM! Test updates correctly reflect the new validator-settings configuration.The addition of the
'validator-settings' => []expectation in all threetoArray()test methods properly accounts for the new per-validator configuration support added toTranslationValidatorConfig. The consistent empty array default value aligns with the expected behavior.Also applies to: 251-251, 271-271
tests/src/Validator/ValidatorRegistryTest.php (2)
29-29: LGTM! Properly imports the new validator class.The import statement for
KeyNamingConventionValidatoris correctly added to support the test assertions.
46-46: LGTM! Test assertions correctly updated for the new validator.The test properly verifies that:
KeyNamingConventionValidatoris included in the available validators list- The total count is updated from 7 to 8 validators
These changes ensure the registry correctly includes the new validator and maintain test accuracy.
Also applies to: 49-49
tests/src/Fixtures/config/validator-settings.json (1)
1-14: LGTM! Well-structured test fixture for validator-specific configuration.This JSON configuration file properly demonstrates the new
validator-settingsfeature, showing how to configure individual validators likeKeyNamingConventionValidatorandHtmlTagValidatoralongside standard configuration options.README.md (2)
88-88: LGTM! Comprehensive documentation for the new validator.The documentation accurately describes the
KeyNamingConventionValidatorfunctionality, including its configurability, supported patterns, and requirement for explicit configuration.
93-95: LGTM! Clear documentation for the new configuration feature.The new section appropriately introduces validator-specific configuration and references detailed documentation, maintaining good documentation structure.
src/Config/ConfigFactory.php (1)
80-83: LGTM! Solid implementation of validator-settings handling.The code properly validates the data type and integrates validator-specific settings into the configuration loading process, following the same defensive programming pattern as other configuration keys.
src/Command/ValidateTranslationCommand.php (2)
137-138: LGTM! Help text properly updated with new validators.The addition of
EncodingValidatorandKeyNamingConventionValidatorto the help text maintains consistency with the existing format and accurately describes their functionality.
206-206: LGTM! Essential change for validator configuration support.Passing the
$configparameter toexecuteForenables validators to access their specific configuration settings, which is crucial for the new validator-specific configuration feature.src/Result/ValidationRun.php (2)
44-44: LGTM! Well-designed method signature change.The addition of the optional
TranslationValidatorConfigparameter maintains backward compatibility while enabling validator-specific configuration support.
57-60: LGTM! Robust config passing implementation.The conditional logic properly checks for both config availability and validator support before setting configuration, enabling optional validator configuration without breaking existing validators.
schema/translation-validator.schema.json (2)
106-128: LGTM! Well-structured validator-settings schema definition.The new
validator-settingsproperty is properly defined with:
- Clear description and type specification
- Pattern properties using regex to validate PHP class names
- Proper use of
additionalProperties: falseto prevent unexpected keys- Comprehensive examples showing realistic usage
168-179: Good addition of practical configuration examples.The new example demonstrates how to configure multiple validators with different settings, providing clear guidance for users implementing validator-specific configurations.
docs/validator-configuration.md (1)
1-166: Excellent comprehensive documentation for validator configuration.This documentation provides:
- Clear examples for both YAML and JSON configuration formats
- Detailed coverage of the KeyNamingConventionValidator with all naming conventions
- Practical guidance for developers implementing custom validators
- Proper error handling examples and best practices
The documentation is well-structured and provides sufficient detail for both users and developers.
tests/src/Config/ValidatorSettingsConfigTest.php (1)
31-153: Comprehensive test coverage for validator settings functionality.The test suite thoroughly covers all aspects of validator settings management:
- Loading from configuration arrays (simulating YAML/JSON)
- Setting and retrieving individual and multiple validator settings
- Edge cases like empty settings and non-existent validators
- Override behavior ensuring complete replacement rather than merging
- Integration with
toArray()methodAll test logic appears correct and follows PHPUnit best practices.
src/Config/TranslationValidatorConfig.php (3)
57-58: Well-typed property for validator settings.The property uses proper PHPDoc type annotation
array<string, array<string, mixed>>which clearly indicates the structure: validator names mapped to their settings arrays.
269-319: Excellent implementation of validator settings management methods.The new methods provide a complete API for managing validator-specific settings:
- Consistent method signatures following existing patterns
- Proper type annotations and return types
- Fluent interface support with
selfreturn types- Safe defaults (empty array) for non-existent settings
- Clear separation between individual and bulk operations
All methods are well-documented and follow the established code style.
338-338: Proper integration with toArray() method.The
validator-settingskey is correctly added to the array representation, maintaining consistency with the configuration schema.tests/src/Validator/KeyNamingConventionValidatorConfigTest.php (1)
34-196: Comprehensive test coverage for KeyNamingConventionValidator configuration.The test suite thoroughly validates all configuration scenarios:
- Loading and applying predefined naming conventions with proper suggestions
- Loading and applying custom regex patterns
- Proper fallback behavior when no configuration is provided
- Error handling with logging for invalid conventions and patterns
- Priority handling where custom patterns override conventions
- Edge cases with null and empty configurations
The tests use appropriate mocks and assertions, ensuring the validator correctly integrates with the configuration system.
src/Validator/KeyNamingConventionValidator.php (4)
35-58: Well-structured naming conventions with appropriate regex patterns.The constant definitions provide a clean and maintainable way to manage supported naming conventions. The regex patterns correctly match their respective formats.
198-207: Robust snake_case conversion implementation.The method correctly handles conversions from multiple naming conventions with proper null handling for regex operations.
231-249: Consistent implementation for dot notation and kebab-case conversions.Both methods follow a similar pattern and correctly handle the conversions with proper null handling.
268-286: Well-structured issue message formatting.The method provides clear, informative messages with appropriate suggestions and handles missing data gracefully.
tests/src/Validator/KeyNamingConventionValidatorTest.php (4)
40-103: Comprehensive test coverage for basic validator functionality.The tests properly cover initialization, configuration, and error handling scenarios with appropriate use of mocks and assertions.
105-217: Excellent use of data providers for parameterized testing.The data providers comprehensively cover valid and invalid cases for each naming convention, including edge cases.
330-389: Thorough testing of naming convention conversions.The conversion tests effectively verify that keys can be transformed between all supported naming conventions correctly.
408-473: Excellent edge case testing for real-world scenarios.The tests cover important edge cases like invalid snake_case patterns and complex acronyms in PascalCase, ensuring the validator handles real-world key naming scenarios correctly.
# Conflicts: # README.md # src/Command/ValidateTranslationCommand.php # src/Validator/ValidatorRegistry.php # tests/src/Validator/ValidatorRegistryTest.php
…improve file path handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Validator/HtmlTagValidator.php (1)
261-261: Address unused variable in foreach loop.The static analysis tool correctly identifies an unused placeholder variable. Consider using a more descriptive variable name if the value might be needed, or use a comment to indicate the intentional discard.
- foreach ($files as $filePath => $_) { + foreach ($files as $filePath => $fileData) {Or if the value is truly not needed:
- foreach ($files as $filePath => $_) { + foreach ($files as $filePath => $fileData) { // $fileData intentionally unusedsrc/Validator/PlaceholderConsistencyValidator.php (1)
213-214: Consider simplifying the loop if file info is unused.The static analysis correctly identified that
$fileInfois not used within the loop. If you only need the file paths, consider usingarray_keys($files)instead:-foreach ($files as $filePath => $fileInfo) { +foreach (array_keys($files) as $filePath) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Command/ValidateTranslationCommand.php(3 hunks)src/FileDetector/Collector.php(2 hunks)src/Result/ValidationRun.php(3 hunks)src/Validator/AbstractValidator.php(4 hunks)src/Validator/DuplicateValuesValidator.php(1 hunks)src/Validator/HtmlTagValidator.php(5 hunks)src/Validator/KeyNamingConventionValidator.php(1 hunks)src/Validator/MismatchValidator.php(5 hunks)src/Validator/PlaceholderConsistencyValidator.php(5 hunks)tests/src/Fixtures/translations/xliff/fail/locallang.xlf(1 hunks)tests/src/Validator/AbstractValidatorTest.php(1 hunks)tests/src/Validator/HtmlTagValidatorTest.php(1 hunks)tests/src/Validator/KeyNamingConventionValidatorConfigTest.php(1 hunks)tests/src/Validator/KeyNamingConventionValidatorTest.php(1 hunks)tests/src/Validator/MismatchValidatorTest.php(1 hunks)tests/src/Validator/PlaceholderConsistencyValidatorTest.php(1 hunks)tests/src/Validator/ValidatorInterfaceTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/src/Fixtures/translations/xliff/fail/locallang.xlf
- tests/src/Validator/ValidatorInterfaceTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Command/ValidateTranslationCommand.php
- src/Result/ValidationRun.php
- tests/src/Validator/KeyNamingConventionValidatorConfigTest.php
- tests/src/Validator/KeyNamingConventionValidatorTest.php
🧰 Additional context used
🧬 Code Graph Analysis (6)
tests/src/Validator/AbstractValidatorTest.php (1)
src/Result/Issue.php (1)
getFile(38-41)
src/FileDetector/Collector.php (3)
src/FileDetector/FileDetectorRegistry.php (2)
FileDetectorRegistry(26-39)getAvailableFileDetectors(31-38)src/FileDetector/PrefixFileDetector.php (1)
mapTranslationSet(33-49)src/FileDetector/SuffixFileDetector.php (1)
mapTranslationSet(33-46)
src/Validator/DuplicateValuesValidator.php (3)
tests/src/Validator/AbstractValidatorTest.php (1)
getFileName(110-113)tests/src/Result/ValidationRunTest.php (3)
getFileName(307-310)getFileName(493-496)getFileName(531-534)src/Parser/AbstractParser.php (1)
getFileName(54-57)
src/Validator/PlaceholderConsistencyValidator.php (1)
tests/src/Validator/AbstractValidatorTest.php (1)
getFileName(110-113)
src/Validator/MismatchValidator.php (4)
tests/src/Validator/AbstractValidatorTest.php (1)
getFileName(110-113)tests/src/Result/ValidationRunTest.php (3)
getFileName(307-310)getFileName(493-496)getFileName(531-534)src/Parser/ParserInterface.php (1)
getFileName(42-42)src/Parser/AbstractParser.php (1)
getFileName(54-57)
src/Validator/HtmlTagValidator.php (1)
tests/src/Validator/AbstractValidatorTest.php (1)
getFileName(110-113)
🪛 PHPMD (2.15.0)
src/Validator/PlaceholderConsistencyValidator.php
213-213: Avoid unused local variables such as '$fileInfo'. (Unused Code Rules)
(UnusedLocalVariable)
src/Validator/HtmlTagValidator.php
261-261: Avoid unused local variables such as '$_'. (Unused Code Rules)
(UnusedLocalVariable)
src/Validator/KeyNamingConventionValidator.php
78-78: Avoid unused local variables such as '$issues'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (30)
src/FileDetector/Collector.php (2)
84-98: LGTM! Improved file organization prevents cross-directory translation sets.The refactoring correctly groups files by their immediate parent directory before applying file detectors. This ensures that translation sets remain within directory boundaries, which is important for proper validation context. The implementation properly handles the directory grouping and path key generation.
200-210: LGTM! Clean and straightforward directory grouping logic.The
groupFilesByDirectorymethod is well-implemented with clear purpose and simple logic. Usingdirname()to group files by their immediate parent directory is the correct approach.src/Validator/KeyNamingConventionValidator.php (10)
37-54: LGTM! Well-defined conventions with clear patterns and descriptions.The predefined naming conventions are comprehensive and include appropriate regex patterns. The kebab-case pattern correctly handles both single characters and multi-character strings with hyphens.
66-100: LGTM! Solid validation logic with appropriate fallback behavior.The
processFilemethod correctly handles both configured conventions and automatic consistency analysis. The logic properly extracts keys, validates them, and generates appropriate issue data.
102-135: LGTM! Robust configuration loading with proper error handling.The configuration loading method properly handles both convention and custom pattern settings, with appropriate validation and logging for invalid configurations. The custom pattern override behavior is correct.
146-155: LGTM! Proper regex validation with error suppression.The custom pattern validation correctly uses error suppression with
@preg_matchand checks forfalsereturn value to detect invalid regex patterns. The logic to override convention when custom pattern is set is appropriate.
157-182: LGTM! Flexible key validation supporting both simple and dot-separated keys.The validation logic correctly handles both single keys and dot-separated keys with segment-wise validation. The early return for no pattern configuration is appropriate.
208-227: LGTM! Clean suggestion logic using match expression.The suggestion generation properly handles dot-separated keys and uses modern PHP match syntax for convention-based corrections.
229-287: LGTM! Comprehensive key conversion methods covering all conventions.The conversion methods handle various input formats (camelCase, PascalCase, snake_case, kebab-case, dot notation) and convert them appropriately to the target convention. The implementations are robust and handle edge cases well.
307-339: LGTM! Well-structured issue message formatting with clear distinction.The issue formatting properly handles both mixed convention inconsistencies and explicit convention violations, providing clear and informative messages with appropriate suggestions.
384-440: LGTM! Intelligent consistency analysis for detecting mixed conventions.The analysis logic correctly identifies the dominant convention and reports inconsistencies. The approach of finding matching conventions for each key and determining the most common one is sound.
447-500: LGTM! Thorough convention detection for both simple and dot-separated keys.The detection methods properly handle dot-separated keys by ensuring all segments follow consistent conventions. The logic for handling mixed conventions and unknown patterns is well-implemented.
tests/src/Validator/MismatchValidatorTest.php (1)
228-229: LGTM! Test update aligns with unified file path handling.The test correctly updates file paths to use absolute paths, consistent with the broader changes in validator file path handling. This ensures test expectations match the actual validator behavior.
tests/src/Validator/AbstractValidatorTest.php (1)
336-336: LGTM! Test assertion correctly expects full file path.The assertion update properly expects the full file path (
'/path/to/some_file.xlf') instead of just the filename, aligning with the consistent file path handling improvements across the validation framework.tests/src/Validator/HtmlTagValidatorTest.php (1)
235-236: LGTM! Test data updated to use consistent full file paths.The file paths in the test Issue object are correctly updated to use full paths (
'/test/path/en.xlf','/test/path/de.xlf'), maintaining consistency with the validator's file path handling improvements.tests/src/Validator/PlaceholderConsistencyValidatorTest.php (1)
353-356: Test correctly updated to reflect new file path handling.The test assertions have been properly updated to expect basename keys (
en.xlf,de.xlf) instead of full file paths in the distribution array. This aligns with the unified file path handling changes where validators now use full paths internally but display basenames for user-facing output.src/Validator/DuplicateValuesValidator.php (1)
56-57: Good improvement to file path handling consistency.The change to prefer
$this->currentFilePathover$file->getFileName()as the internal key aligns with the broader standardization across validators. This ensures consistent use of full file paths internally while maintaining backward compatibility through the fallback logic.src/Validator/AbstractValidator.php (4)
39-39: New property supports enhanced file path tracking.The addition of
$currentFilePathproperty enables validators to access the full file path being processed, supporting the unified file path handling across the validation framework.
63-63: Proper initialization of file path tracking.Setting
$currentFilePathat the start of each file iteration ensures validators have access to the full file path throughout the processing lifecycle.
110-129: Excellent enhancement to support multiple issues per file.The updated validation result handling intelligently detects whether
processFilereturns multiple issues (array of arrays) or single issue data, creating appropriate Issue objects for each case. Using the full$filePathinstead of$file->getFileName()improves consistency in issue reporting.This change enables validators like the new KeyNamingConventionValidator to report multiple naming violations per file.
206-213: Simplified and more accurate issue distribution.The updated logic directly uses the full file path from Issue objects instead of reconstructing paths, eliminating potential inconsistencies and simplifying the distribution logic.
src/Validator/MismatchValidator.php (2)
56-57: Consistent file path handling implementation.The change to prefer
$this->currentFilePathover$file->getFileName()aligns with the standardized approach across validators, ensuring full file paths are used internally for accurate referencing.
115-115: Proper use of basename for display consistency.The explicit use of
basename()ensures consistent display of filenames in user-facing output while maintaining full paths in internal data structures. This approach provides clarity in error messages and detailed output.Also applies to: 143-143, 185-185, 202-202
src/Validator/HtmlTagValidator.php (3)
61-62: Consistent internal file path handling.The change to use
$this->currentFilePathwhen available maintains consistency with other validators and ensures accurate internal referencing using full file paths.
192-193: Proper basename usage for user-facing messages.Using
basename()ensures error messages display clear, readable filenames while maintaining full path accuracy in the underlying data structures.
298-299: Appropriate file path handling in detailed output.The consistent use of full file paths as keys with
basename()extraction for display maintains the unified approach across the validation framework while ensuring clear, readable output.Also applies to: 312-312
src/Validator/PlaceholderConsistencyValidator.php (4)
61-62: LGTM: Consistent file identifier handling.The change to use full file paths as internal keys while maintaining backward compatibility with the fallback to
getFileName()aligns well with the file identifier unification effort across validators.
171-172: Good separation of internal vs display concerns.The approach of using full file paths internally (
$fileNames[$i]) while extracting basenames for user-facing messages maintains clean separation between data integrity and user experience.
250-251: Consistent file path handling with defensive programming.The basename extraction for display purposes is correct and follows the unified approach. The defensive string cast on
$filePathis good practice, though likely unnecessary in this context.
264-264: Efficient basename extraction for display ordering.The use of a static closure with
array_mapefficiently converts full file paths to basenames for display ordering, completing the consistent file identifier handling pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/src/Validator/XliffSchemaValidatorTest.php (1)
59-158: Comprehensive test coverage for file processing scenarios.The tests cover all important scenarios: valid files, missing files, invalid XML, and unsupported versions. The use of temporary files with proper cleanup and mock expectations for logging is well implemented.
Consider extracting the temporary file creation/cleanup pattern into helper methods to reduce code duplication and improve maintainability.
Example helper methods to reduce duplication:
private function createTempFileWithContent(string $content): string { $tempFile = tempnam(sys_get_temp_dir(), 'xliff_test_'); file_put_contents($tempFile, $content); return $tempFile; } private function cleanupTempFile(string $filePath): void { if (file_exists($filePath)) { unlink($filePath); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/src/FileDetector/CollectorTest.php(1 hunks)tests/src/FileDetector/DirectoryFileDetectorTest.php(1 hunks)tests/src/Validator/XliffSchemaValidatorTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/src/FileDetector/DirectoryFileDetectorTest.php (1)
src/FileDetector/DirectoryFileDetector.php (2)
DirectoryFileDetector(26-64)mapTranslationSet(39-63)
tests/src/FileDetector/CollectorTest.php (1)
src/FileDetector/Collector.php (2)
Collector(34-211)collectFiles(46-103)
🪛 GitHub Actions: CGL
tests/src/FileDetector/CollectorTest.php
[error] 445-445: EditorConfig: Trailing whitespaces found.
[error] 448-448: EditorConfig: Trailing whitespaces found.
[error] 451-451: EditorConfig: Trailing whitespaces found.
[error] 494-494: EditorConfig: Trailing whitespaces found.
[error] 495-495: EditorConfig: Trailing whitespaces found.
[error] 509-509: EditorConfig: Trailing whitespaces found.
[error] 512-512: EditorConfig: Trailing whitespaces found.
🔇 Additional comments (5)
tests/src/Validator/XliffSchemaValidatorTest.php (3)
1-44: LGTM! Well-structured test class with proper setup.The file header, license, namespace, and class structure follow PHP best practices. The setUp method properly initializes the validator with a mock logger for testing.
46-57: Good coverage of basic validator functionality.These tests properly verify instantiation flexibility and parser support. The assertions are specific and appropriate.
160-244: Excellent test coverage for issue message formatting.The formatting tests comprehensively cover all scenarios: complete error details, different severity levels, empty details, and incomplete error arrays. The tests properly verify console color formatting and fallback behavior for missing data.
tests/src/FileDetector/CollectorTest.php (1)
458-475: LGTM! Well-structured test for edge case handling.This test effectively uses reflection to test the private
normalizePathmethod's fallback behavior whenrealpath()fails. The test logic correctly verifies that the method returns the trimmed path as expected according to the implementation insrc/FileDetector/Collector.php.tests/src/FileDetector/DirectoryFileDetectorTest.php (1)
278-298: LGTM! Comprehensive test for invalid path handling.This test thoroughly covers edge cases for invalid file paths and correctly verifies that the
DirectoryFileDetector::mapTranslationSetmethod properly filters out invalid paths while preserving valid ones. The test cases align perfectly with the implementation logic insrc/FileDetector/DirectoryFileDetector.php, which usesarray_pop()to extract filename and language directory components and skips files when either component is missing.The test scenarios effectively cover:
- Files without directory structure
- Files with insufficient path segments
- Valid directory-based files
- Empty path strings
| public function testCollectFilesWithGlobFailure(): void | ||
| { | ||
| // Create a path that doesn't exist and will cause glob to return false | ||
| $nonExistentPath = '/this/path/absolutely/does/not/exist/anywhere'; | ||
|
|
||
| $logger = $this->createMock(LoggerInterface::class); | ||
| // Don't expect any specific log calls since non-existent paths might not trigger warning logs | ||
|
|
||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $collector = new Collector($logger); | ||
|
|
||
| $result = $collector->collectFiles([$nonExistentPath], $detector, null); | ||
|
|
||
| // Should return empty array when path doesn't exist | ||
| $this->assertEmpty($result); | ||
| } |
There was a problem hiding this comment.
Fix the test logic and trailing whitespace issues.
This test has several issues:
-
Incorrect test assumption: The test attempts to test
globfailure by using a non-existent path, but according to theCollector::collectFilesmethod insrc/FileDetector/Collector.php(lines 47-52), non-existent paths are caught byFilesystem::exists()check and logged as errors before reaching theglob()call. -
Trailing whitespace: Lines 445, 448, and 451 have trailing whitespace as indicated by the pipeline failures.
To properly test glob failure, you need a path that exists but causes glob() to return false. Apply this diff:
public function testCollectFilesWithGlobFailure(): void
{
- // Create a path that doesn't exist and will cause glob to return false
- $nonExistentPath = '/this/path/absolutely/does/not/exist/anywhere';
-
+ // Create a path that exists but will cause glob to return false
+ // by using special characters that make glob fail
+ $problematicPath = $this->tempDir . '/path[with{invalid}glob]characters';
+ mkdir($problematicPath, 0755, true);
+
$logger = $this->createMock(LoggerInterface::class);
- // Don't expect any specific log calls since non-existent paths might not trigger warning logs
-
- $detector = $this->createMock(DetectorInterface::class);
+ $logger->expects($this->atLeastOnce())
+ ->method('warning')
+ ->with($this->stringContains('Failed to glob files in path'));
+
+ $detector = $this->createMock(DetectorInterface::class);
$collector = new Collector($logger);
-
- $result = $collector->collectFiles([$nonExistentPath], $detector, null);
+
+ $result = $collector->collectFiles([$problematicPath], $detector, null);
- // Should return empty array when path doesn't exist
+ // Should return empty array when glob fails
$this->assertEmpty($result);
}📝 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 testCollectFilesWithGlobFailure(): void | |
| { | |
| // Create a path that doesn't exist and will cause glob to return false | |
| $nonExistentPath = '/this/path/absolutely/does/not/exist/anywhere'; | |
| $logger = $this->createMock(LoggerInterface::class); | |
| // Don't expect any specific log calls since non-existent paths might not trigger warning logs | |
| $detector = $this->createMock(DetectorInterface::class); | |
| $collector = new Collector($logger); | |
| $result = $collector->collectFiles([$nonExistentPath], $detector, null); | |
| // Should return empty array when path doesn't exist | |
| $this->assertEmpty($result); | |
| } | |
| public function testCollectFilesWithGlobFailure(): void | |
| { | |
| // Create a path that exists but will cause glob to return false | |
| // by using special characters that make glob fail | |
| $problematicPath = $this->tempDir . '/path[with{invalid}glob]characters'; | |
| mkdir($problematicPath, 0755, true); | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $logger->expects($this->atLeastOnce()) | |
| ->method('warning') | |
| ->with($this->stringContains('Failed to glob files in path')); | |
| $detector = $this->createMock(DetectorInterface::class); | |
| $collector = new Collector($logger); | |
| $result = $collector->collectFiles([$problematicPath], $detector, null); | |
| // Should return empty array when glob fails | |
| $this->assertEmpty($result); | |
| } |
🧰 Tools
🪛 GitHub Actions: CGL
[error] 445-445: EditorConfig: Trailing whitespaces found.
[error] 448-448: EditorConfig: Trailing whitespaces found.
[error] 451-451: EditorConfig: Trailing whitespaces found.
🤖 Prompt for AI Agents
In tests/src/FileDetector/CollectorTest.php around lines 441 to 456, the test
incorrectly assumes that using a non-existent path will cause glob() to fail,
but the method checks for path existence first and logs errors before glob() is
called. To fix this, modify the test to use a path that exists but causes glob()
to return false, such as by mocking glob() or using a valid directory with a
pattern that triggers glob failure. Also, remove trailing whitespace on lines
445, 448, and 451 to resolve formatting issues.
| public function testCollectFilesRecursiveHandlesException(): void | ||
| { | ||
| // Create a directory that we'll try to access but then make inaccessible | ||
| $restrictedDir = $this->tempDir.'/restricted'; | ||
| mkdir($restrictedDir, 0755); | ||
| file_put_contents($restrictedDir.'/test.xlf', 'content'); | ||
|
|
||
| // Make the directory unreadable to trigger an exception | ||
| chmod($restrictedDir, 0000); | ||
|
|
||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $logger->expects($this->atLeastOnce()) | ||
| ->method('error') | ||
| ->with($this->stringContains('Error during recursive file search:')); | ||
|
|
||
| $detector = $this->createMock(DetectorInterface::class); | ||
| $collector = new Collector($logger); | ||
|
|
||
| $result = $collector->collectFiles([$restrictedDir], $detector, null, true); | ||
|
|
||
| // Restore permissions for cleanup | ||
| chmod($restrictedDir, 0755); | ||
|
|
||
| $this->assertEmpty($result); | ||
| } |
There was a problem hiding this comment.
Fix trailing whitespace and improve test robustness.
The test concept is good for testing exception handling during recursive file search, but there are several issues:
- Trailing whitespace: Lines 509 and 512 have trailing whitespace as indicated by pipeline failures.
- Platform compatibility: Permission-based tests may not work consistently across all operating systems or file systems.
Apply this diff to fix the whitespace issues:
// Create a directory that we'll try to access but then make inaccessible
$restrictedDir = $this->tempDir.'/restricted';
mkdir($restrictedDir, 0755);
file_put_contents($restrictedDir.'/test.xlf', 'content');
-
+
// Make the directory unreadable to trigger an exception
chmod($restrictedDir, 0000);
-
+
$logger = $this->createMock(LoggerInterface::class);Optional improvement: Consider adding a platform check or try-catch around the permission changes to make the test more robust:
// Skip test on Windows or if chmod doesn't work as expected
if (DIRECTORY_SEPARATOR === '\\') {
$this->markTestSkipped('Permission-based test not reliable on Windows');
}🧰 Tools
🪛 GitHub Actions: CGL
[error] 509-509: EditorConfig: Trailing whitespaces found.
[error] 512-512: EditorConfig: Trailing whitespaces found.
🤖 Prompt for AI Agents
In tests/src/FileDetector/CollectorTest.php around lines 503 to 527, remove
trailing whitespace on lines 509 and 512 to fix pipeline failures. Additionally,
add a platform check at the start of the test to skip it on Windows systems by
checking if DIRECTORY_SEPARATOR equals '\\' and calling markTestSkipped,
ensuring the permission-based test runs only on compatible platforms.
Optionally, wrap chmod calls in try-catch blocks to handle potential permission
change failures gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Fixtures/examples/README.md (1)
7-18: Add a language identifier to the directory-tree code blockMarkdown-lint (MD040) flags fenced blocks without a language.
Addingtext(orbash, if preferred) avoids the warning and improves syntax highlighting.-``` +```text examples/ ├── duplicate-keys/ # DuplicateKeysValidator examples ... └── xliff-schema/ # XliffSchemaValidator examplesdocs/validators.md (1)
46-54: Specify a language for every console-output code fenceSeveral fenced blocks that show CLI output lack a language identifier, triggering MD040.
Useconsole,bash, or at leasttextfor consistency and lint compliance. Example fix:-``` +```console Fixtures/examples/duplicate-keys/messages.en.xlf ... [ERROR] Language validation failed with errors.Apply the same change to all nine affected blocks (approx. lines 46-54, 91-100, 150-158, 194-202, 238-252, 297-303, 353-371, 413-431, 477-487).
Also applies to: 91-100, 150-158, 194-202, 238-252, 297-303, 353-371, 413-431, 477-487
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.md(1 hunks)docs/validators.md(1 hunks)tests/Fixtures/examples/README.md(1 hunks)tests/Fixtures/examples/duplicate-keys/messages.en.xlf(1 hunks)tests/Fixtures/examples/duplicate-keys/messages.en.yaml(1 hunks)tests/Fixtures/examples/duplicate-values/errors.en.yaml(1 hunks)tests/Fixtures/examples/empty-values/navigation.de.xlf(1 hunks)tests/Fixtures/examples/encoding/special.en.json(1 hunks)tests/Fixtures/examples/html-tags/messages.de.yaml(1 hunks)tests/Fixtures/examples/html-tags/messages.en.yaml(1 hunks)tests/Fixtures/examples/key-naming/mixed.en.yaml(1 hunks)tests/Fixtures/examples/key-naming/translation-validator.yaml(1 hunks)tests/Fixtures/examples/mismatch/buttons.de.yaml(1 hunks)tests/Fixtures/examples/mismatch/buttons.en.yaml(1 hunks)tests/Fixtures/examples/mismatch/buttons.fr.yaml(1 hunks)tests/Fixtures/examples/placeholders/notifications.de.yaml(1 hunks)tests/Fixtures/examples/placeholders/notifications.en.yaml(1 hunks)tests/Fixtures/examples/xliff-schema/malformed.xlf(1 hunks)
✅ Files skipped from review due to trivial changes (15)
- tests/Fixtures/examples/html-tags/messages.en.yaml
- tests/Fixtures/examples/mismatch/buttons.de.yaml
- tests/Fixtures/examples/placeholders/notifications.de.yaml
- tests/Fixtures/examples/key-naming/mixed.en.yaml
- tests/Fixtures/examples/placeholders/notifications.en.yaml
- tests/Fixtures/examples/encoding/special.en.json
- tests/Fixtures/examples/html-tags/messages.de.yaml
- tests/Fixtures/examples/key-naming/translation-validator.yaml
- tests/Fixtures/examples/mismatch/buttons.en.yaml
- tests/Fixtures/examples/duplicate-keys/messages.en.yaml
- tests/Fixtures/examples/duplicate-values/errors.en.yaml
- tests/Fixtures/examples/xliff-schema/malformed.xlf
- tests/Fixtures/examples/mismatch/buttons.fr.yaml
- tests/Fixtures/examples/empty-values/navigation.de.xlf
- tests/Fixtures/examples/duplicate-keys/messages.en.xlf
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/validators.md
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
150-150: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
297-297: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
353-353: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
413-413: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
tests/Fixtures/examples/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| **File: `mixed.en.yaml`** | ||
| ```yaml | ||
| user_name: "Username" # ✅ Good | ||
| userEmail: "Email" # 🚨 camelCase | ||
| user-phone: "Phone" # 🚨 kebab-case | ||
| User.Address: "Address" # 🚨 Mixed styles | ||
| ``` | ||
|
|
||
| ### Console Output | ||
| ``` | ||
| [OK] Language validation succeeded. | ||
| ``` | ||
|
|
||
| **Note:** The current fixture file contains only valid snake_case keys. To see validation errors, you would need to include keys that violate the naming convention (like camelCase or kebab-case). | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Key-naming example contradicts the “all good” console output
The YAML snippet deliberately includes camelCase, kebab-case, and mixed-style keys that should violate the configured snake_case pattern (lines 289-294), yet the console output claims [OK] Language validation succeeded and the note says the fixture contains only valid keys.
Either:
- Replace the invalid keys with valid
snake_caseones, or - Keep the invalid keys but change the expected console output to show WARNINGS and update the explanatory note.
Leaving the contradiction will confuse users and undermine trust in the new validator’s documentation.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
297-297: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/validators.md around lines 288 to 302, the YAML example includes invalid
key styles (camelCase, kebab-case, mixed) but the console output incorrectly
shows a success message and the note claims only valid keys are present. To fix
this, either replace the invalid keys with valid snake_case keys to match the
success output, or keep the invalid keys and update the console output to show
validation warnings and revise the note to explain the presence of invalid keys
and expected errors. This will ensure consistency between the example, output,
and explanation.
…iptions and examples
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/file-detector.md (4)
122-130: Specify a language for the fenced code block
markdownlintflags this block because the language is missing.
Addtext(orbash) to suppress the linter and improve syntax-highlighting.-``` +```text Resources/Private/Language/ ├── locallang.xlf # Default English ...
137-148: Add language hint to Symfony structure blockSame linter issue as above. Recommend
text:-``` +```text translations/ ├── messages.en.yaml ...
156-173: Add language hint to Laravel structure block-``` +```text resources/lang/ ├── en/ ...
213-227: Add language hint to “Examples by Format” block-``` +```text # Simple language codes en/messages.php → locale: "en" ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)docs/file-detector.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/file-detector.md
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes