feat: add configuration files and schema validation for translation validator#23
feat: add configuration files and schema validation for translation validator#23konradmichalik merged 3 commits intomainfrom
Conversation
WalkthroughThis update introduces comprehensive configuration file support for the translation validator, adding a new configuration schema, loader, and validation logic. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ValidateTranslationCommand
participant ConfigReader
participant SchemaValidator
participant TranslationValidatorConfig
User->>CLI: Run validate-translations [--config path]
CLI->>ValidateTranslationCommand: Initialize command
ValidateTranslationCommand->>ConfigReader: loadConfiguration(input)
alt --config option provided
ConfigReader->>ConfigReader: read(configPath)
else composer.json config present
ConfigReader->>ConfigReader: readFromComposerJson(path)
else auto-detect config
ConfigReader->>ConfigReader: autoDetect()
end
ConfigReader->>SchemaValidator: validate(config data)
SchemaValidator-->>ConfigReader: Success or exception
ConfigReader-->>ValidateTranslationCommand: TranslationValidatorConfig
ValidateTranslationCommand->>TranslationValidatorConfig: Get settings (paths, validators, etc.)
ValidateTranslationCommand->>CLI: Execute validation and output result
Possibly related PRs
Poem
✨ 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: 3
♻️ Duplicate comments (1)
tests/src/Fixtures/config/valid.json (1)
1-13: Same dashed-key concern as incomplete.jsonEnsure the reader/schema accept
"file-detectors","dry-run", etc., otherwise these settings will silently be ignored.
🧹 Nitpick comments (12)
tests/src/Fixtures/config/valid.yml (1)
1-4: Nit: unify extension to.yamlfor consistencyThe project uses both
.yamland.yml. Standardising on one extension (commonly.yaml) avoids duplicate globbing patterns in tooling.README.md (1)
63-66: Minor wording nit – “YAML language files”“YAML” already implies “Yet Another Markup Language”; the extra “language” is redundant.
-| [Yaml](https://en.wikipedia.org/wiki/YAML) | Supports yaml language files. +| [YAML](https://en.wikipedia.org/wiki/YAML) | Supports YAML files.tests/composer.json (1)
28-28: Inconsistent formatter flag between fail and fail:check
translation:yaml:failnow enforces--format=json, but its:checkcounterpart does not. If the tests rely on parsing JSON output in both cases, add the same flag for consistency.tests/src/FileDetector/FileDetectorRegistryTest.php (1)
23-28: Consider consolidating with existing test.This test only checks that the array is non-empty, which is already covered by the existing
testGetAvailableFileDetectors()that verifies specific classes and count.tests/src/Validator/ValidatorRegistryTest.php (2)
25-30: Consider consolidating with existing test.This test only checks that the array is non-empty, which is already covered by the existing
testGetAvailableValidators()that verifies specific classes and count.
54-59: Consider consolidating with existing test.This test verifies the presence of
DuplicateValuesValidator, which is already covered by the existingtestGetAvailableValidators()that checks for specific validators.src/Config/SchemaValidator.php (1)
9-9: Consider making the schema path configurable.The hard-coded schema path may limit flexibility for testing or different deployment scenarios. Consider making it configurable via constructor parameter or environment variable.
- private const SCHEMA_PATH = __DIR__.'/../../schema/translation-validator.schema.json'; + public function __construct(private readonly string $schemaPath = __DIR__.'/../../schema/translation-validator.schema.json') + { + }tests/src/Config/SchemaValidatorTest.php (1)
88-111: Potential file system race condition in schema file manipulation.The test temporarily renames the schema file to test error conditions, but this could interfere with parallel test execution. Consider using a mock or temporary schema path instead.
Consider mocking the schema path or using dependency injection to avoid file system manipulation:
+ public function testLoadSchemaFileNotFoundThrowsException(): void + { + $validator = new SchemaValidator('/non/existent/path/schema.json'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('JSON Schema file not found:'); + + $validator->validate(['paths' => ['test']]); + }tests/src/Result/ValidationRunTest.php (1)
468-471: Address unused parameter warnings in mock classes.The static analysis correctly identifies unused parameters in mock methods. Since these are test mocks, you can either use the parameters or explicitly mark them as unused.
Apply this diff to address the static analysis warnings:
- public function getContentByKey(string $key, string $attribute = 'source'): ?string + public function getContentByKey(string $key, string $attribute = 'source'): ?string { + unset($key, $attribute); // Explicitly mark as unused in mock return null; }Also applies to: 506-509
src/Config/ConfigReader.php (1)
117-206: Consider refactoring to reduce code duplication.The method has repetitive validation patterns that could be extracted into a helper method.
Consider extracting the repetitive validation logic:
+ /** + * @param array<string, mixed> $data + * @param string $key + * @param string $expectedType + * @throws \RuntimeException + */ + private function validateArrayField(array $data, string $key, string $expectedType = 'array'): mixed + { + if (!isset($data[$key])) { + return null; + } + + $value = $data[$key]; + $actualType = gettype($value); + + if ($actualType !== $expectedType && !($expectedType === 'boolean' && is_bool($value))) { + throw new \RuntimeException("Configuration '{$key}' must be " . ($expectedType === 'boolean' ? 'a boolean' : "an {$expectedType}")); + } + + return $value; + }Then simplify the validation blocks:
- if (isset($data['paths'])) { - if (!is_array($data['paths'])) { - throw new \RuntimeException("Configuration 'paths' must be an array"); - } - $config->setPaths($data['paths']); - } + $paths = $this->validateArrayField($data, 'paths'); + if ($paths !== null) { + $config->setPaths($paths); + }src/Config/TranslationValidatorConfig.php (1)
131-136: Inconsistent method naming for fluent setters.The methods
only()andskip()don't follow the naming convention of other setters in this class. They should be namedaddOnly()andaddSkip()to matchaddValidator(),addFileDetector(), andaddParser().- public function only(string $validator): self + public function addOnly(string $validator): self { $this->only[] = $validator; return $this; } - public function skip(string $validator): self + public function addSkip(string $validator): self { $this->skip[] = $validator; return $this; }Also applies to: 156-161
src/Command/ValidateTranslationCommand.php (1)
226-239: Document limitation: Only first file detector is used.The implementation only uses the first file detector from the configuration array. Consider documenting this behavior or supporting multiple file detectors in the future.
Add a comment to clarify this behavior:
private function resolveFileDetector(TranslationValidatorConfig $config): ?DetectorInterface { $configFileDetectors = $config->getFileDetectors(); + // Currently only the first file detector is used $fileDetectorClass = !empty($configFileDetectors) ? $configFileDetectors[0] : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
README.md(1 hunks)rector.php(2 hunks)schema/translation-validator.schema.json(1 hunks)src/Command/ValidateTranslationCommand.php(5 hunks)src/Config/ConfigReader.php(1 hunks)src/Config/SchemaValidator.php(1 hunks)src/Config/TranslationValidatorConfig.php(1 hunks)src/Result/ValidationResultJsonRenderer.php(2 hunks)tests/composer.json(1 hunks)tests/src/Command/ValidateTranslationCommandConfigTestSimple.php(1 hunks)tests/src/Command/ValidateTranslationCommandTest.php(1 hunks)tests/src/Config/ConfigReaderTest.php(1 hunks)tests/src/Config/SchemaValidatorTest.php(1 hunks)tests/src/Config/TranslationValidatorConfigTest.php(1 hunks)tests/src/FileDetector/CollectorTest.php(1 hunks)tests/src/FileDetector/FileDetectorRegistryTest.php(1 hunks)tests/src/Fixtures/config/auto-detect/translation-validator.json(1 hunks)tests/src/Fixtures/config/auto-detect/translation-validator.php(1 hunks)tests/src/Fixtures/config/auto-detect/translation-validator.yaml(1 hunks)tests/src/Fixtures/config/complete.json(1 hunks)tests/src/Fixtures/config/invalid-format.json(1 hunks)tests/src/Fixtures/config/invalid-paths.json(1 hunks)tests/src/Fixtures/config/invalid.json(1 hunks)tests/src/Fixtures/config/invalid.php(1 hunks)tests/src/Fixtures/config/invalid.yaml(1 hunks)tests/src/Fixtures/config/unsupported.txt(1 hunks)tests/src/Fixtures/config/valid.json(1 hunks)tests/src/Fixtures/config/valid.php(1 hunks)tests/src/Fixtures/config/valid.yaml(1 hunks)tests/src/Fixtures/config/valid.yml(1 hunks)tests/src/Parser/AbstractParserTest.php(1 hunks)tests/src/Parser/ParserCacheTest.php(1 hunks)tests/src/Parser/ParserRegistryTest.php(1 hunks)tests/src/PluginTest.php(1 hunks)tests/src/Result/FormatTypeTest.php(1 hunks)tests/src/Result/IssueTest.php(1 hunks)tests/src/Result/OutputTest.php(1 hunks)tests/src/Result/ValidationResultJsonRendererTest.php(4 hunks)tests/src/Result/ValidationRunTest.php(2 hunks)tests/src/Result/ValidationStatisticsTest.php(1 hunks)tests/src/Utility/PathUtilityExtendedTest.php(0 hunks)tests/src/Utility/PathUtilityTest.php(1 hunks)tests/src/Validator/ResultTypeTest.php(1 hunks)tests/src/Validator/ValidatorRegistryTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/src/Utility/PathUtilityExtendedTest.php
🧰 Additional context used
🧬 Code Graph Analysis (14)
rector.php (1)
src/Config/TranslationValidatorConfig.php (1)
skip(156-161)
tests/src/Fixtures/config/auto-detect/translation-validator.php (1)
src/Config/TranslationValidatorConfig.php (2)
TranslationValidatorConfig(7-266)setPaths(41-46)
tests/src/Result/IssueTest.php (1)
src/Result/Issue.php (4)
Issue(7-55)getDetails(28-31)toArray(46-54)getFile(20-23)
tests/src/PluginTest.php (1)
src/Plugin.php (4)
Plugin(14-34)activate(16-18)deactivate(20-22)uninstall(24-26)
tests/src/Fixtures/config/valid.php (1)
src/Config/TranslationValidatorConfig.php (5)
TranslationValidatorConfig(7-266)setPaths(41-46)setStrict(199-204)setFormat(223-228)setVerbose(235-240)
tests/src/Validator/ValidatorRegistryTest.php (2)
src/Validator/ValidatorRegistry.php (2)
ValidatorRegistry(7-21)getAvailableValidators(12-20)src/Validator/DuplicateValuesValidator.php (1)
DuplicateValuesValidator(12-98)
src/Result/ValidationResultJsonRenderer.php (2)
src/Result/ValidationResult.php (1)
getOverallResult(52-55)src/Validator/ResultType.php (1)
notFullySuccessful(20-23)
tests/src/FileDetector/FileDetectorRegistryTest.php (1)
src/FileDetector/FileDetectorRegistry.php (2)
FileDetectorRegistry(7-19)getAvailableFileDetectors(12-18)
tests/src/Result/OutputTest.php (2)
src/Result/ValidationResult.php (1)
ValidationResult(11-69)src/Result/Output.php (2)
Output(11-41)summarize(29-40)
tests/src/Parser/AbstractParserTest.php (1)
src/Parser/AbstractParser.php (1)
AbstractParser(7-47)
tests/src/Result/ValidationStatisticsTest.php (1)
src/Result/ValidationStatistics.php (2)
ValidationStatistics(7-51)getExecutionTimeFormatted(23-30)
tests/src/Parser/ParserRegistryTest.php (3)
src/Parser/XliffParser.php (1)
XliffParser(7-76)src/Parser/ParserRegistry.php (3)
ParserRegistry(9-41)resolveParserClass(25-40)getAvailableParsers(14-20)src/Parser/YamlParser.php (1)
YamlParser(9-82)
tests/src/Utility/PathUtilityTest.php (2)
src/Utility/PathUtility.php (2)
PathUtility(7-30)normalizeFolderPath(9-29)tests/src/Utility/PathUtilityExtendedTest.php (1)
testNormalizeFolderPathWithNonExistentPath(14-18)
tests/src/Validator/ResultTypeTest.php (1)
src/Validator/ResultType.php (3)
toString(38-45)toColorString(47-54)max(15-18)
🪛 Biome (1.9.4)
tests/src/Fixtures/config/invalid.json
[error] 1-1: Property key must be double quoted
(parse)
[error] 1-1: expected : but instead found json
Remove json
(parse)
[error] 1-1: expected , but instead found syntax
Remove syntax
(parse)
[error] 1-1: expected : but instead the file ends
the file ends here
(parse)
🪛 YAMLlint (1.37.1)
tests/src/Fixtures/config/invalid.yaml
[error] 1-1: syntax error: mapping values are not allowed here
(syntax)
🪛 LanguageTool
README.md
[style] ~66-~66: This phrase is redundant (‘L’ stands for ‘language’). Use simply “yaml”.
Context: ...n.wikipedia.org/wiki/YAML) | Supports yaml language files. | [Symfony F...
(ACRONYM_TAUTOLOGY)
🪛 PHPMD (2.15.0)
tests/src/Result/ValidationRunTest.php
468-468: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
468-468: Avoid unused parameters such as '$attribute'. (Unused Code Rules)
(UnusedFormalParameter)
506-506: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
506-506: Avoid unused parameters such as '$attribute'. (Unused Code Rules)
(UnusedFormalParameter)
src/Command/ValidateTranslationCommand.php
137-137: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (80)
rector.php (2)
8-8: LGTM! Import added correctly.The import follows the existing pattern and is properly organized with other rector imports.
25-25: LGTM! Skip configuration added appropriately.The skip configuration for
AddDoesNotPerformAssertionToNonAssertingTestRectoris correctly added and follows the same pattern as other skip configurations in the file. This prevents automatic addition ofdoesNotPerformAssertions()annotations to test methods.tests/src/Result/IssueTest.php (2)
113-128: LGTM! Comprehensive edge case testing.This test method provides excellent coverage for various edge cases including null values, special characters, unicode, empty strings, zero, and boolean false. The assertions verify that the Issue class preserves these diverse data types correctly in both
getDetails()andtoArray()methods.
130-136: LGTM! Long file path edge case covered.This test appropriately covers the edge case of very long file paths, ensuring the Issue class handles them without truncation or corruption. The test is concise and effective.
tests/src/PluginTest.php (3)
23-32: LGTM! Proper lifecycle method testing.The test correctly covers the
activate()method by mocking the required dependencies and verifying no exceptions are thrown. The use ofaddToAssertionCount(1)is appropriate for testing empty methods that should execute without errors.
34-43: LGTM! Consistent deactivate method testing.The test follows the same pattern as the activate test and provides necessary coverage for the
deactivate()lifecycle method.
45-54: LGTM! Complete lifecycle coverage.The test completes the lifecycle method coverage by testing
uninstall()with the same consistent pattern used for activate and deactivate.tests/src/Result/ValidationStatisticsTest.php (3)
105-110: LGTM! Large value formatting test.This test appropriately covers the formatting of large execution times, verifying that the thousands separator and decimal precision are handled correctly. The expected result "1,000.12s" matches the
number_format()behavior with 2 decimal places.
112-117: LGTM! Boundary value testing.This test covers an important boundary case just below 1 second (0.999), ensuring it's correctly formatted as milliseconds ("999ms") rather than seconds. This validates the < 1.0 threshold logic.
119-124: LGTM! Microsecond precision edge case.This test covers the extreme edge case of very small execution times (1 microsecond), verifying that the formatting rounds down to "0ms" as expected. This ensures the method handles precision limits gracefully.
tests/src/Fixtures/config/unsupported.txt (1)
1-1: LGTM! Simple test fixture for unsupported formats.This fixture file appropriately contains plain text content that can be used to test error handling when the configuration system encounters unsupported file formats. The minimal content serves its purpose as a test fixture.
tests/src/Fixtures/config/invalid.json (1)
1-2: Fixture correctly simulates malformed JSONThe deliberately broken syntax fulfils its purpose for negative-path tests. No action required.
tests/src/Fixtures/config/auto-detect/translation-validator.yaml (1)
1-2: Auto-detection sample looks fineMinimal YAML is valid and representative. 👍
tests/src/Fixtures/config/invalid.yaml (1)
1-1: Intentionally invalid YAML is good for negative testsThe malformed mapping triggers the expected parse failure path. No changes needed.
tests/src/Fixtures/config/invalid-format.json (1)
1-4: Negative fixture correctly exercises schema rejectionThe unsupported
"format"value should cause validation to fail, as intended. Looks good.tests/src/Fixtures/config/invalid-paths.json (1)
1-3: LGTM! Good test fixture for invalid configuration.This fixture correctly demonstrates an invalid configuration where
pathsis a string instead of the expected array, which will help test schema validation error handling.tests/src/Fixtures/config/auto-detect/translation-validator.json (1)
1-3: LGTM! Valid auto-detection fixture.This fixture correctly provides a valid JSON configuration with proper array structure for
paths, supporting the auto-detection testing functionality.tests/src/Fixtures/config/invalid.php (1)
1-5: LGTM! Effective negative test fixture.This fixture correctly demonstrates an invalid PHP configuration that returns a string instead of the expected
TranslationValidatorConfiginstance, which will help test error handling in the configuration reader.tests/src/Parser/ParserCacheTest.php (1)
144-148: LGTM! Good edge case test coverage.This test properly verifies that
ParserCache::getreturnsfalsewhen passed anullparser class, improving the robustness of the parser cache functionality.tests/src/Fixtures/config/valid.php (1)
1-11: LGTM! Well-structured valid configuration fixture.This fixture correctly demonstrates the proper usage of
TranslationValidatorConfigwith fluent interface method chaining. The configuration values are appropriate for testing and the file follows PHP best practices with strict type declarations.tests/src/Fixtures/config/auto-detect/translation-validator.php (1)
7-8: Fixture looks goodThe PHP fixture is minimal yet valid: strict types are declared and the fluent setter chain returns the expected
TranslationValidatorConfiginstance. No issues spotted.tests/src/Fixtures/config/complete.json (1)
1-13: Dashed property names are already supportedThe JSON schema and
ConfigReaderexplicitly handle dashed keys like"file-detectors"and"dry-run", so no changes are necessary.Key evidence:
- schema/translation-validator.schema.json:31 defines
"file-detectors"- src/Config/ConfigReader.php:141 checks
isset($data['file-detectors'])- src/Config/TranslationValidatorConfig.php:255 maps
'file-detectors'to$this->fileDetectorstests/composer.json (1)
32-32: Escaping looks correct – LGTMThe double-escaped backslashes in the
--onlyargument are required inside JSON. No action needed.tests/src/Fixtures/config/valid.yaml (1)
1-21: LGTM: Well-structured YAML configuration fixture.The YAML configuration fixture contains comprehensive settings that align with the translation validator's schema. All field types appear correct and the values are appropriate for testing purposes.
tests/src/Command/ValidateTranslationCommandConfigTestSimple.php (1)
1-24: LGTM: Well-structured test for config option validation.The test properly validates the presence and configuration of the
--configoption, including its shortcut and description. The implementation follows PHPUnit best practices with proper namespace, imports, and coverage attributes.src/Result/ValidationResultJsonRenderer.php (2)
7-7: LGTM: Appropriate import for enhanced result handling.The import of
ResultTypesupports the improved message generation logic that distinguishes between different validation result states.
40-55: LGTM: Enhanced message generation with better specificity.The refactored
generateMessagemethod provides more descriptive messages by:
- Using
ResultType::notFullySuccessful()for cleaner success checking- Distinguishing between ERROR and WARNING states
- Providing specific messages for dry-run mode combinations
- Using a match expression for clear, readable logic
The implementation correctly handles all expected result type combinations with an appropriate default fallback.
tests/src/Command/ValidateTranslationCommandTest.php (1)
179-179: LGTM: Test assertion updated to match enhanced error messaging.The updated assertion correctly expects the more specific error message that distinguishes between errors and warnings, aligning with the improved message generation in
ValidationResultJsonRenderer.tests/src/Result/FormatTypeTest.php (1)
1-25: LGTM: Comprehensive enum testing with proper error handling.The test class effectively validates the
FormatTypeenum behavior by:
- Testing exception handling for invalid values with
ValueError- Verifying the presence of expected enum cases (
CLIandJSON)- Following PHPUnit best practices with proper structure and naming
The implementation provides good coverage for the enum's core functionality.
tests/src/Result/ValidationResultJsonRendererTest.php (4)
71-71: Test assertion updated to match enhanced error messaging.The updated assertion reflects improved specificity in error messages, distinguishing between errors and warnings in the JSON output.
109-109: Test assertion updated for dry-run mode messaging.Good enhancement to verify that dry-run mode is properly reflected in the JSON output message.
141-141: Test assertion updated for strict mode warning handling.The assertion correctly verifies that warnings are properly handled in strict mode scenarios.
173-173: Test assertion updated for non-strict warning handling.The assertion correctly verifies that warnings are properly handled in non-strict mode scenarios.
tests/src/FileDetector/FileDetectorRegistryTest.php (2)
30-42: Excellent validation of registry integrity.This test provides valuable verification that all registered classes exist and implement the required interface, which is crucial for runtime safety.
44-50: Good consistency verification.This test ensures the registry returns detectors in a consistent order, which is important for predictable behavior.
tests/src/Validator/ValidatorRegistryTest.php (2)
32-44: Excellent validation of registry integrity.This test provides valuable verification that all registered classes exist and implement the required interface, which is crucial for runtime safety.
46-52: Good consistency verification.This test ensures the registry returns validators in a consistent order, which is important for predictable behavior.
tests/src/Validator/ResultTypeTest.php (3)
68-73: Excellent coverage of string representation.This test verifies the
toString()method returns the expected string values for all enum cases.
75-80: Good coverage of color mapping.This test verifies the
toColorString()method returns the expected color strings for console output formatting.
82-86: Important edge case coverage.This test covers the edge case where
max()is called with identical values, ensuring the method handles this correctly.tests/src/Config/TranslationValidatorConfigTest.php (1)
1-283: Comprehensive test suite for configuration management.This test file provides excellent coverage of the
TranslationValidatorConfigclass with well-structured tests for:✅ All getter/setter methods
✅ Collection management (add/set operations)
✅ Default value verification
✅ Array conversion functionality
✅ Fluent interface support
✅ Method chaining behaviorThe tests are well-organized with clear test names and proper assertions. The use of
#[CoversClass]attribute is a good practice for ensuring focused coverage tracking.src/Config/SchemaValidator.php (4)
25-26: The JSON encode/decode conversion is correct but potentially inefficient.This approach correctly converts PHP arrays to objects for schema validation, but creates unnecessary JSON serialization overhead. This is acceptable for configuration validation which happens infrequently.
28-35: Excellent error formatting and aggregation.The error handling properly collects all validation errors and formats them in a user-friendly way with property paths and messages.
43-55: Robust schema file handling with comprehensive error checking.The method properly handles file existence, read errors, and JSON parsing errors with specific error messages.
60-63: Clean dependency availability check.Simple and effective way to handle optional dependency gracefully.
tests/src/Result/OutputTest.php (5)
91-107: Good test coverage for warning scenarios.Properly tests that warnings return SUCCESS exit code in normal mode and checks the appropriate message.
109-129: Comprehensive JSON format failure testing.Correctly validates both exit code and JSON structure for error scenarios.
131-151: JSON format warning handling is well tested.Validates that warnings produce SUCCESS exit code and proper JSON message format.
153-170: Dry run mode behavior is correctly tested.Properly verifies that dry run mode returns SUCCESS even with errors, which is the expected behavior for preview/validation scenarios.
172-190: Strict mode logic is properly validated.Correctly tests that strict mode treats warnings as failures, returning FAILURE exit code.
schema/translation-validator.schema.json (2)
110-127: Conditional logic for skip/only relationship is well implemented.The schema correctly handles the mutual exclusivity where
skipis ignored whenonlyis specified, which makes logical sense for validator selection.
129-147: Comprehensive and helpful examples.The examples demonstrate both simple and complex configuration scenarios, which will help users understand the schema structure.
tests/src/Config/SchemaValidatorTest.php (4)
21-27: Good availability check test.Properly verifies that the JSON schema package is available in the test environment.
29-63: Comprehensive valid data testing.Tests both simple and complex configuration scenarios that should pass validation.
113-139: File manipulation test has proper cleanup.The test correctly uses try/finally to ensure the original schema file is restored even if the test fails.
154-171: Excellent multiple error validation test.Properly verifies that all validation errors are collected and reported together, which is important for user experience.
tests/src/Result/ValidationRunTest.php (3)
232-244: Good test coverage for parser exception handling.Properly verifies that parser exceptions are handled gracefully without breaking the validation run, while correctly tracking file counts.
246-257: Comprehensive test for null key handling.Correctly tests the scenario where parsers return null keys, ensuring the statistics are properly maintained.
456-492: Well-designed mock classes for error testing.The mock parser classes effectively simulate the error conditions needed for testing parser exception handling and null return scenarios.
Also applies to: 494-530
tests/src/Parser/AbstractParserTest.php (2)
11-17: Well-structured test helper class.The
TestParserimplementation correctly extendsAbstractParserand provides the requiredgetSupportedFileExtensions()method for testing purposes.
19-127: Comprehensive test coverage for AbstractParser.The test suite thoroughly covers all aspects of the AbstractParser class including:
- Valid and invalid file scenarios
- Permission handling with proper cleanup
- Extension validation
- All getter methods
- Edge cases
The use of try-finally blocks in tests that modify file permissions ensures proper cleanup even if assertions fail.
tests/src/FileDetector/CollectorTest.php (4)
108-124: Good edge case coverage for empty glob results.This test properly verifies the collector's behavior when directories exist but contain no matching files, ensuring appropriate debug logging occurs.
126-142: Validates YAML file collection with both extensions.The test correctly verifies that both
.yamland.ymlfiles are collected and associated with theYamlParserclass.
144-162: Thorough test of wildcard exclude patterns.This test effectively validates that wildcard patterns in the exclude list properly filter out both files and directories matching the pattern.
164-187: Validates multi-path collection with proper cleanup.The test correctly verifies collection from multiple directories and ensures proper cleanup of all temporary resources.
tests/src/Parser/ParserRegistryTest.php (4)
30-44: Comprehensive extension resolution testing.This test provides excellent coverage of parser resolution for various file extensions, including all supported XLIFF and YAML variants, as well as verification that unsupported formats return null.
46-52: Good edge case coverage for filename parsing.The test correctly handles edge cases including empty strings and files with only extensions. The behavior for
.xlfis correct aspathinfo()does extract 'xlf' as the extension.
54-66: Excellent validation of parser registry integrity.This test ensures that all parsers returned by the registry are valid classes that implement the required
ParserInterface, preventing runtime errors from misconfigured parsers.
68-74: Validates method consistency.Simple but important test ensuring that
getAvailableParsers()returns consistent results across multiple calls.tests/src/Utility/PathUtilityTest.php (4)
87-91: Tests combined path normalization scenarios.Correctly verifies that both
./prefix and trailing slashes are removed during normalization.
99-103: Potential environment-dependent test behavior.This test expects
.to normalize to an empty string, which assumes it resolves to the current working directory. The behavior might vary depending on the test execution context.Consider making this test more robust by explicitly setting the working directory or documenting the expected execution context.
117-133: Good coverage of non-existent path scenarios.These tests correctly verify that non-existent paths (absolute, relative, and with
..segments) are normalized by only removing trailing slashes while preserving the path structure.
111-115: Confirm intended behavior for root directory normalizationThe implementation of
PathUtility::normalizeFolderPath()usesrealpath()followed byrtrim(..., DIRECTORY_SEPARATOR). For the root directory/,realpath('/')returns/, andrtrim('/', '/')yields an empty string—so the test’s expectation of''matches the current logic.Please verify whether this is the desired behavior:
• If the root folder should be preserved, consider adding a special-case after trimming, for example:
$normalizedPath = rtrim($realPath, DIRECTORY_SEPARATOR); if ($normalizedPath === '') { $normalizedPath = DIRECTORY_SEPARATOR; }and update the test to expect
/.
• Otherwise, if returning''for/is intentional, please document this edge case clearly in the method’s docblock or README.Relevant locations:
- src/Utility/PathUtility.php (normalizeFolderPath realpath + rtrim logic)
- tests/src/Utility/PathUtilityTest.php lines 111–115 (test for
/input)tests/src/Config/ConfigReaderTest.php (1)
13-453: Excellent comprehensive test coverage for ConfigReader.This test suite provides thorough coverage of the ConfigReader functionality including:
- All supported configuration formats (PHP, JSON, YAML, YML)
- Auto-detection logic with priority handling
- Composer.json integration with relative/absolute paths
- Extensive validation testing for all configuration fields
- Proper error handling and edge cases
- Good use of fixtures and temporary files with cleanup
The test structure is well-organized and follows testing best practices.
src/Config/ConfigReader.php (3)
18-36: LGTM! Clean implementation of file format detection.The method properly validates file existence and readability before dispatching to format-specific readers.
38-54: Good implementation of configuration auto-detection.The method properly handles the edge case where
getcwd()returns false and searches for configuration files in a predefined order.
56-76: Well-structured composer.json integration.The method correctly reads the configuration file path from composer.json and handles relative paths appropriately.
src/Command/ValidateTranslationCommand.php (3)
40-124: Excellent addition of comprehensive help documentation.The help text provides clear examples and detailed explanations of all options, validators, and configuration methods.
126-132: Good practice: Early initialization of IO components.The
initialize()method properly sets up the IO components early in the command lifecycle, following Symfony best practices.
187-211: Well-structured configuration loading with proper fallback chain.The method implements a logical cascade: explicit config file → composer.json → auto-detection → default config.
README.md
Outdated
|
|
||
| ```bash | ||
| composer validate-translations [<path>] [-dr|--dry-run] [-f|--format cli|json] [-e|--exclude PATTERN] [-s|--skip VALIDATOR] [-o|--only VALIDATOR] [-v|--verbose] | ||
| composer validate-translations [<path>] [-dr|--dry-run] [-f|--format cli|json] [-s|--skip VALIDATOR] [-o|--only VALIDATOR] [-v|--verbose] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Illegal multi-character short option -dr
Most CLI helpers (incl. Symfony Console) allow only single-character shortcuts. Documenting -dr is misleading; users will need --dry-run (or a single char like -d if supported).
-composer validate-translations [<path>] [-dr|--dry-run] [-f|--format cli|json] ...
+composer validate-translations [<path>] [--dry-run] [-f|--format cli|json] ...Ensure the help output generated by ValidateTranslationCommand matches the README.
📝 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.
| composer validate-translations [<path>] [-dr|--dry-run] [-f|--format cli|json] [-s|--skip VALIDATOR] [-o|--only VALIDATOR] [-v|--verbose] | |
| composer validate-translations [<path>] [--dry-run] [-f|--format cli|json] [-s|--skip VALIDATOR] [-o|--only VALIDATOR] [-v|--verbose] |
🤖 Prompt for AI Agents
In README.md at line 42, the documented CLI option `-dr` is invalid as
multi-character short options are not supported. Replace `-dr` with the correct
single-character short option (e.g., `-d`) if available, or remove it and use
the full `--dry-run` option instead. Verify that the
`ValidateTranslationCommand` help output matches this corrected usage to avoid
confusion.
| "description": "List of validator class names to use", | ||
| "items": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z_][A-Za-z0-9_\\\\]*$" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the class name regex pattern covers all valid PHP class names.
The regex pattern ^[A-Za-z_][A-Za-z0-9_\\\\]*$ should properly match PHP fully qualified class names. Let me verify this handles all edge cases correctly.
Also applies to: 36-36, 47-47, 58-58, 69-69
🏁 Script executed:
#!/bin/bash
# Description: Test the regex pattern against various PHP class names to ensure it's comprehensive.
# Test the regex pattern with various PHP class name examples
python3 << 'EOF'
import re
pattern = r'^[A-Za-z_][A-Za-z0-9_\\]*$'
test_cases = [
# Valid cases
('ClassName', True),
('Class_Name', True),
('_ClassName', True),
('Class123', True),
('Namespace\\ClassName', True),
('Deep\\Nested\\Namespace\\ClassName', True),
('_Internal\\Class_Name', True),
# Invalid cases
('123Class', False),
('Class-Name', False),
('Class Name', False),
('Class.Name', False),
('', False),
('\\LeadingSlash', False),
('TrailingSlash\\', False),
]
print("Testing PHP class name regex pattern:")
print(f"Pattern: {pattern}")
print()
for test_class, expected in test_cases:
result = bool(re.match(pattern, test_class))
status = "✓" if result == expected else "✗"
print(f"{status} '{test_class}' -> {result} (expected: {expected})")
print()
print("Note: The pattern should allow leading underscores, namespaces with backslashes,")
print("and alphanumeric characters, while rejecting leading numbers and special chars.")
EOFLength of output: 851
Update PHP class name regex to disallow trailing namespace separators
The current pattern
"pattern": "^[A-Za-z_][A-Za-z0-9_\\\\]*$"incorrectly accepts a trailing backslash (e.g. "TrailingSlash\"). We need to require that every \ is followed by a valid segment and forbid it at the end.
Apply this update at:
- schema/translation-validator.schema.json — lines 25, 36, 47, 58, 69
Suggested replacement (note JSON escaping of backslashes):
- "pattern": "^[A-Za-z_][A-Za-z0-9_\\\\]*$"
+ "pattern": "^[A-Za-z_][A-Za-z0-9_]*(?:\\\\\\\\[A-Za-z_][A-Za-z0-9_]*)*$"This enforces:
- Start with a letter or underscore
- Followed by letters, digits or underscores
- Zero or more groups of
\+ valid segment - No trailing
\allowed
🤖 Prompt for AI Agents
In schema/translation-validator.schema.json at lines 25, 36, 47, 58, and 69,
update the regex pattern for PHP class names to disallow trailing backslashes.
Replace the current pattern "^[A-Za-z_][A-Za-z0-9_\\\\]*$" with one that
enforces starting with a letter or underscore, followed by letters, digits, or
underscores, and zero or more groups of a backslash plus a valid segment,
ensuring no trailing backslash is allowed. This will prevent invalid class names
ending with a backslash.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
69-69: Fix redundant phrase in YAML descriptionThe phrase "yaml language files" is redundant since YAML already stands for "YAML Ain't Markup Language".
-| [Yaml](https://en.wikipedia.org/wiki/YAML) | Supports yaml language files. | [Symfony Framework](https://symfony.com/) | `messages.en.yaml`, `messages.de.yaml` | +| [Yaml](https://en.wikipedia.org/wiki/YAML) | Supports yaml files. | [Symfony Framework](https://symfony.com/) | `messages.en.yaml`, `messages.de.yaml` |tests/src/Config/ConfigReaderTest.php (1)
270-270: Consider extracting temporary directory creation patternThe test methods use various approaches for creating unique temporary directories. Consider extracting this into a helper method to ensure consistency and reduce code duplication.
private function createTempDir(string $prefix = 'translation-validator-'): string { $tempDir = sys_get_temp_dir() . '/' . $prefix . uniqid('', true); mkdir($tempDir, 0777, true); return $tempDir; }
📜 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 (11)
README.md(1 hunks)composer.json(1 hunks)docs/config-file.md(1 hunks)docs/file-detector.md(1 hunks)docs/schema.md(1 hunks)schema/translation-validator.schema.json(1 hunks)src/Command/ValidateTranslationCommand.php(6 hunks)src/Config/ConfigReader.php(1 hunks)tests/composer.json(1 hunks)tests/src/Config/ConfigReaderTest.php(1 hunks)tests/src/Fixtures/config/unsupported.txt(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/composer.json
- docs/file-detector.md
- docs/schema.md
- docs/config-file.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/src/Fixtures/config/unsupported.txt
- schema/translation-validator.schema.json
- src/Config/ConfigReader.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Command/ValidateTranslationCommand.php
137-137: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 LanguageTool
README.md
[style] ~69-~69: This phrase is redundant (‘L’ stands for ‘language’). Use simply “yaml”.
Context: ...n.wikipedia.org/wiki/YAML) | Supports yaml language files. | [Symfony F...
(ACRONYM_TAUTOLOGY)
🔇 Additional comments (7)
composer.json (1)
17-17: LGTM - Appropriate dependency for JSON schema validationThe addition of
justinrainbow/json-schemais well-suited for the configuration schema validation functionality introduced in this PR. The version constraint^6.4follows semantic versioning best practices.README.md (1)
33-57: Excellent documentation improvementsThe expanded usage section with examples and detailed option table significantly improves user guidance. The addition of the
--configoption and removal of deprecated options aligns well with the new configuration-driven approach.tests/src/Config/ConfigReaderTest.php (1)
1-457: Excellent comprehensive test coverageThis test suite provides thorough coverage of the
ConfigReaderfunctionality with well-structured test methods covering:
- Multiple configuration file formats (PHP, JSON, YAML, YML)
- Auto-detection and composer.json integration
- Error handling and edge cases
- Schema validation scenarios
- Proper cleanup of temporary files and directories
The test organization and assertions are appropriate for each scenario.
src/Command/ValidateTranslationCommand.php (4)
87-123: Excellent comprehensive help documentationThe expanded help text significantly improves user experience by providing:
- Clear usage examples
- Available validators with descriptions
- Configuration options explanation
- Output format and mode descriptions
This greatly enhances the CLI usability.
187-211: Well-structured configuration loading with proper priorityThe configuration loading follows a logical priority order:
- Explicit config file (highest priority)
- Config from composer.json
- Auto-detection
- Default configuration (fallback)
This approach provides flexibility while maintaining predictable behavior.
137-137: Static analysis false positive - unused parameterThe
$outputparameter is required by the parent class interfaceCommand::execute()method signature. This is a false positive from the static analysis tool.
244-272: Clean validator resolution logic with proper precedenceThe validator resolution properly handles the precedence between CLI options and configuration values, giving priority to CLI input when present. The logic for handling
onlyvsskipoptions is correct and clear.
| private function resolvePaths(InputInterface $input, TranslationValidatorConfig $config): array | ||
| { | ||
| $inputPaths = $input->getArgument('path'); | ||
| $configPaths = $config->getPaths(); | ||
|
|
||
| $paths = !empty($inputPaths) ? $inputPaths : $configPaths; | ||
|
|
||
| return array_map(static fn ($path) => str_starts_with((string) $path, '/') ? $path : getcwd().'/'.$path, $paths); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve path resolution robustness
The path resolution logic could be more robust in handling edge cases.
- private function resolvePaths(InputInterface $input, TranslationValidatorConfig $config): array
- {
- $inputPaths = $input->getArgument('path');
- $configPaths = $config->getPaths();
-
- $paths = !empty($inputPaths) ? $inputPaths : $configPaths;
-
- return array_map(static fn ($path) => str_starts_with((string) $path, '/') ? $path : getcwd().'/'.$path, $paths);
- }
+ private function resolvePaths(InputInterface $input, TranslationValidatorConfig $config): array
+ {
+ $inputPaths = $input->getArgument('path');
+ $configPaths = $config->getPaths();
+
+ $paths = !empty($inputPaths) ? $inputPaths : $configPaths;
+
+ return array_map(static function ($path) {
+ $path = (string) $path;
+ if (str_starts_with($path, '/')) {
+ return $path;
+ }
+ $cwd = getcwd();
+ if ($cwd === false) {
+ throw new \RuntimeException('Cannot determine current working directory');
+ }
+ return $cwd . '/' . $path;
+ }, $paths);
+ }📝 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 resolvePaths(InputInterface $input, TranslationValidatorConfig $config): array | |
| { | |
| $inputPaths = $input->getArgument('path'); | |
| $configPaths = $config->getPaths(); | |
| $paths = !empty($inputPaths) ? $inputPaths : $configPaths; | |
| return array_map(static fn ($path) => str_starts_with((string) $path, '/') ? $path : getcwd().'/'.$path, $paths); | |
| } | |
| private function resolvePaths(InputInterface $input, TranslationValidatorConfig $config): array | |
| { | |
| $inputPaths = $input->getArgument('path'); | |
| $configPaths = $config->getPaths(); | |
| $paths = !empty($inputPaths) ? $inputPaths : $configPaths; | |
| return array_map(static function ($path) { | |
| $path = (string) $path; | |
| if (str_starts_with($path, '/')) { | |
| return $path; | |
| } | |
| $cwd = getcwd(); | |
| if ($cwd === false) { | |
| throw new \RuntimeException('Cannot determine current working directory'); | |
| } | |
| return $cwd . '/' . $path; | |
| }, $paths); | |
| } |
🤖 Prompt for AI Agents
In src/Command/ValidateTranslationCommand.php around lines 216 to 224, the
current path resolution logic does not handle edge cases such as empty paths,
relative paths with './' or '../', or inconsistent directory separators. Improve
robustness by normalizing each path after resolving it: trim whitespace, resolve
relative segments to absolute paths using realpath or equivalent, and ensure
consistent directory separators. Also, handle cases where getcwd() might fail or
paths are empty, providing fallback or error handling as needed.
Summary by CodeRabbit
New Features
--configoption for the command-line tool.Bug Fixes
Documentation
Tests