Conversation
WalkthroughThis update introduces YAML translation file support alongside existing XLIFF support. It adds new parser and file detector classes for YAML, updates the registry and validator logic to handle multiple formats, and revises documentation and Composer dependencies. Test files and build scripts for YAML validation are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ValidateTranslationCommand
participant Collector
participant FileDetectorRegistry
participant ParserRegistry
participant Parser (XliffParser/YamlParser)
participant Validator
User->>CLI: Run translation validation command
CLI->>ValidateTranslationCommand: Start command
ValidateTranslationCommand->>Collector: collectFiles(paths, detector?)
alt Detector provided
Collector->>FileDetector: Use provided detector
else No detector provided
Collector->>FileDetectorRegistry: getAvailableFileDetectors()
loop For each detector
Collector->>FileDetector: Try mapping files
end
end
Collector->>ParserRegistry: getAvailableParsers()
loop For each file
Collector->>Parser: Instantiate appropriate parser (XliffParser/YamlParser)
Parser->>Validator: Pass to validators
Validator->>Parser: supportsParser()
alt Supported
Validator->>Parser: validate()
else Not supported
Validator-->>Parser: Skip file
end
end
ValidateTranslationCommand->>CLI: Output results
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2)src/Parser/XliffParser.phpsrc/Parser/YamlParser.php📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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
🔭 Outside diff range comments (3)
src/Parser/XliffParser.php (3)
21-29: Add null safety checks.The method assumes
$this->xmlis a valid SimpleXMLElement but doesn't handle the case where XML parsing failed in the constructor.public function extractKeys(): ?array { + if ($this->xml === false) { + return null; + } + $keys = []; foreach ($this->xml->file->body->{'trans-unit'} as $unit) { $keys[] = (string) $unit['id']; } return $keys; }
31-40: Add null safety check.Same issue as
extractKeys()- the method needs to handle the case where XML parsing failed.public function getContentByKey(string $key, string $attribute = 'source'): ?string { + if ($this->xml === false) { + return null; + } + foreach ($this->xml->file->body->{'trans-unit'} as $unit) { if ((string) $unit['id'] === $key) { return ('' !== (string) $unit->{$attribute}) ? (string) $unit->{$attribute} : null; } } return null; }
50-59: Add null safety check for XML access.The method needs to handle the case where XML parsing failed, similar to other methods.
public function getLanguage(): string { if (preg_match('/^([a-z]{2})\./i', $this->getFileName(), $matches)) { $language = $matches[1]; } else { + if ($this->xml === false) { + return ''; + } $language = (string) ($this->xml->file['source-language'] ?? ''); } return $language; }
🧹 Nitpick comments (9)
src/FileDetector/FileDetectorRegistry.php (2)
1-19: Add missing documentation comments for better code maintainability.The static analysis tools correctly identified missing documentation. Consider adding file and class-level documentation to explain the registry's purpose and usage patterns.
+/** + * Registry for managing available file detector implementations. + * + * This class provides a centralized way to retrieve all supported + * file detectors that implement the DetectorInterface. + */ class FileDetectorRegistry { + /** + * Returns all available file detector class names. + * + * @return array<int, class-string<DetectorInterface>> + */ public static function getAvailableFileDetectors(): array
12-18: Consider making the registry more extensible for future enhancements.While the current hardcoded approach works well for the immediate need, consider whether you might want to make this more extensible in the future (e.g., allowing plugins to register additional detectors).
For now, the simple approach is perfectly fine, but if extensibility becomes important, you could consider:
private static array $detectors = [ PrefixFileDetector::class, SuffixFileDetector::class, ]; public static function register(string $detectorClass): void { if (!in_array($detectorClass, self::$detectors, true)) { self::$detectors[] = $detectorClass; } }src/Parser/AbstractParser.php (2)
1-47: Add comprehensive documentation comments for the abstract class.The static analysis correctly identified missing documentation. This abstract class serves as a foundation for all parsers and should be well-documented.
+/** + * Abstract base class for translation file parsers. + * + * Provides common functionality for file validation, path operations, + * and extension checking. Subclasses must implement getSupportedFileExtensions() + * to specify which file extensions they can handle. + */ abstract class AbstractParser { protected string $fileName = ''; + /** + * Constructs a new parser instance with file validation. + * + * @param string $filePath Path to the translation file to parse + * @throws \InvalidArgumentException If file doesn't exist or has unsupported extension + * @throws \RuntimeException If file is not readable + */ public function __construct(protected string $filePath) + /** + * Returns the base filename without path. + */ public function getFileName(): string + /** + * Returns the directory path with trailing separator. + */ public function getFileDirectory(): string + /** + * Returns the full file path. + */ public function getFilePath(): string
33-36: Consider removing redundant DIRECTORY_SEPARATOR in getFileDirectory().The
dirname()function already returns a proper directory path, and addingDIRECTORY_SEPARATORmight create double separators in some cases or be unnecessary depending on how this method is used.public function getFileDirectory(): string { - return dirname($this->filePath).\DIRECTORY_SEPARATOR; + return dirname($this->filePath); }Or if trailing separator is specifically needed by consumers, consider documenting this behavior clearly.
README.md (1)
10-11: Fix markdown formatting inconsistencies.The static analysis correctly identified formatting inconsistencies. The nested list should use consistent bullet style and indentation with the rest of the document.
* Supports the following formats: - - XLIFF - `*.xlf`, `*.xliff` - - Yaml - `*.yml`, `*.yaml` + * XLIFF - `*.xlf`, `*.xliff` + * Yaml - `*.yml`, `*.yaml`src/Validator/ValidatorInterface.php (1)
25-28: Add missing documentation for the new method.The static analysis tool correctly identified that the doc comment is missing a short description. This method is essential for the new multi-parser support functionality.
Apply this diff to add the missing documentation:
+ /** + * Returns the parser classes that this validator supports. + * + * @return class-string<ParserInterface>[] + */ - /** - * @return class-string<ParserInterface>[] - */ public function supportsParser(): array;src/Parser/YamlParser.php (1)
70-77: Consider more flexible language detection pattern.The current regex
/\.(\w{2})\./only matches language codes between two dots (e.g., "messages.en.yaml"). This might miss common patterns like "en.messages.yaml" or "messages_en.yaml".Consider a more flexible pattern:
public function getLanguage(): string { - if (preg_match('/\.(\w{2})\./', $this->getFileName(), $matches)) { + if (preg_match('/(?:^|[._])([a-z]{2})(?:[._]|$)/i', $this->getFileName(), $matches)) { return $matches[1]; } return ''; }This would match language codes at the start, end, or between separators (dots or underscores).
src/Parser/XliffParser.php (2)
7-7: Add class documentation.The class is missing documentation as flagged by static analysis. Consider adding a docblock to describe the class purpose and responsibilities.
+/** + * Parser for XLIFF translation files. + * + * Handles parsing of XLIFF (.xliff, .xlf) format translation files, + * extracting translation keys, content, and language information. + */ class XliffParser extends AbstractParser implements ParserInterface
9-9: Address naming convention and consider error handling strategy.Two concerns:
- Private property should follow naming convention (prefixed with underscore per PEAR standards)
- The
booltype in the union suggests XML loading can fail, but this isn't handled in methods using$this->xml- private readonly \SimpleXMLElement|bool $xml; + private readonly \SimpleXMLElement|bool $_xml;Consider whether the property should be nullable instead of
boolfor clearer intent, or add proper error handling in methods that use this property.
📜 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 (21)
README.md(1 hunks)composer.json(1 hunks)src/Command/ValidateTranslationCommand.php(2 hunks)src/FileDetector/Collector.php(2 hunks)src/FileDetector/FileDetectorRegistry.php(1 hunks)src/FileDetector/PrefixFileDetector.php(1 hunks)src/FileDetector/SuffixFileDetector.php(1 hunks)src/Parser/AbstractParser.php(1 hunks)src/Parser/ParserRegistry.php(1 hunks)src/Parser/XliffParser.php(1 hunks)src/Parser/YamlParser.php(1 hunks)src/Validator/AbstractValidator.php(2 hunks)src/Validator/DuplicatesValidator.php(2 hunks)src/Validator/MismatchValidator.php(2 hunks)src/Validator/SchemaValidator.php(2 hunks)src/Validator/ValidatorInterface.php(1 hunks)tests/Build/app/Language/YAML-fail/messages.de.yaml(1 hunks)tests/Build/app/Language/YAML-fail/messages.en.yaml(1 hunks)tests/Build/app/Language/YAML-success/messages.de.yaml(1 hunks)tests/Build/app/Language/YAML-success/messages.en.yaml(1 hunks)tests/Build/app/composer.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/Parser/ParserRegistry.php (1)
src/Parser/YamlParser.php (1)
YamlParser(9-78)
src/Validator/ValidatorInterface.php (4)
src/Validator/DuplicatesValidator.php (1)
supportsParser(78-81)src/Validator/AbstractValidator.php (1)
supportsParser(73-73)src/Validator/MismatchValidator.php (1)
supportsParser(99-102)src/Validator/SchemaValidator.php (1)
supportsParser(88-91)
src/Validator/DuplicatesValidator.php (5)
src/Parser/XliffParser.php (1)
XliffParser(7-60)src/Validator/AbstractValidator.php (1)
supportsParser(73-73)src/Validator/MismatchValidator.php (1)
supportsParser(99-102)src/Validator/ValidatorInterface.php (1)
supportsParser(28-28)src/Validator/SchemaValidator.php (1)
supportsParser(88-91)
src/Validator/SchemaValidator.php (5)
src/Parser/XliffParser.php (1)
XliffParser(7-60)src/Validator/DuplicatesValidator.php (1)
supportsParser(78-81)src/Validator/AbstractValidator.php (1)
supportsParser(73-73)src/Validator/MismatchValidator.php (1)
supportsParser(99-102)src/Validator/ValidatorInterface.php (1)
supportsParser(28-28)
src/FileDetector/FileDetectorRegistry.php (2)
src/FileDetector/PrefixFileDetector.php (1)
PrefixFileDetector(7-31)src/FileDetector/SuffixFileDetector.php (1)
SuffixFileDetector(7-31)
src/Validator/MismatchValidator.php (6)
src/Parser/XliffParser.php (1)
XliffParser(7-60)src/Parser/YamlParser.php (1)
YamlParser(9-78)src/Validator/DuplicatesValidator.php (1)
supportsParser(78-81)src/Validator/AbstractValidator.php (1)
supportsParser(73-73)src/Validator/ValidatorInterface.php (1)
supportsParser(28-28)src/Validator/SchemaValidator.php (1)
supportsParser(88-91)
src/FileDetector/SuffixFileDetector.php (1)
src/FileDetector/PrefixFileDetector.php (1)
mapTranslationSet(14-30)
src/Validator/AbstractValidator.php (6)
src/Validator/DuplicatesValidator.php (1)
supportsParser(78-81)src/Validator/MismatchValidator.php (1)
supportsParser(99-102)src/Validator/ValidatorInterface.php (1)
supportsParser(28-28)src/Validator/SchemaValidator.php (1)
supportsParser(88-91)src/Parser/AbstractParser.php (1)
getFileName(28-31)src/Parser/ParserInterface.php (1)
getFileName(23-23)
src/Parser/AbstractParser.php (2)
src/Parser/XliffParser.php (2)
__construct(11-16)getSupportedFileExtensions(45-48)src/Parser/YamlParser.php (2)
__construct(14-19)getSupportedFileExtensions(65-68)
src/Parser/YamlParser.php (2)
src/Parser/AbstractParser.php (4)
AbstractParser(7-47)__construct(11-26)getSupportedFileExtensions(46-46)getFileName(28-31)src/Parser/XliffParser.php (5)
__construct(11-16)extractKeys(21-29)getContentByKey(31-40)getSupportedFileExtensions(45-48)getLanguage(50-59)
🪛 markdownlint-cli2 (0.17.2)
README.md
10-10: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 phpcs (3.7.2)
src/Validator/ValidatorInterface.php
[ERROR] 25-25: Missing short description in doc comment
(Generic.Commenting.DocComment.MissingShort)
src/Validator/DuplicatesValidator.php
[ERROR] 75-75: Missing short description in doc comment
(Generic.Commenting.DocComment.MissingShort)
src/FileDetector/FileDetectorRegistry.php
[ERROR] 2-2: Missing file doc comment
(PEAR.Commenting.FileComment.Missing)
[ERROR] 7-7: Missing doc comment for class FileDetectorRegistry
(PEAR.Commenting.ClassComment.Missing)
[ERROR] 9-9: Missing short description in doc comment
(Generic.Commenting.DocComment.MissingShort)
src/Parser/AbstractParser.php
[ERROR] 2-2: Missing file doc comment
(PEAR.Commenting.FileComment.Missing)
[ERROR] 7-7: Missing doc comment for class AbstractParser
(PEAR.Commenting.ClassComment.Missing)
[ERROR] 11-11: Missing doc comment for function __construct()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 28-28: Missing doc comment for function getFileName()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 33-33: Missing doc comment for function getFileDirectory()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 38-38: Missing doc comment for function getFilePath()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 43-43: Missing short description in doc comment
(Generic.Commenting.DocComment.MissingShort)
src/Parser/XliffParser.php
[ERROR] 7-7: Missing doc comment for class XliffParser
(PEAR.Commenting.ClassComment.Missing)
[ERROR] 9-9: Private member variable "xml" must be prefixed with an underscore
(PEAR.NamingConventions.ValidVariableName.PrivateNoUnderscore)
[ERROR] 11-11: Missing doc comment for function __construct()
(PEAR.Commenting.FunctionComment.Missing)
🔇 Additional comments (26)
src/Parser/AbstractParser.php (1)
11-26: Excellent file validation logic in the constructor.The constructor provides comprehensive validation including file existence, readability, and extension checking. The error messages are clear and informative, which will help with debugging.
README.md (1)
9-11: Great addition of YAML support documentation.The documentation accurately reflects the new YAML support with correct file extensions. This properly informs users about the expanded capabilities.
composer.json (1)
22-23: Appropriate dependency addition for YAML support.The
symfony/yamldependency is correctly added with version constraints that match the existing Symfony packages. This is the standard and reliable choice for YAML parsing in PHP projects.src/Validator/SchemaValidator.php (2)
8-8: Correctly imports XliffParser for the supportsParser implementation.The import is properly added to support the new
supportsParser()method implementation.
85-91: Proper implementation of supportsParser() method.The implementation correctly declares that
SchemaValidatoronly supportsXliffParser, which makes sense since schema validation is specific to XML/XLIFF files. The method signature and documentation follow the established patterns from other validators.src/Validator/MismatchValidator.php (2)
8-9: LGTM! Proper imports added for YAML support.The imports for both XliffParser and YamlParser are correctly added to support the new parser functionality.
96-102: LGTM! Correct implementation of parser support method.The
supportsParser()method properly implements the interface requirement and correctly returns both XLIFF and YAML parser classes, which is appropriate for a mismatch validator that should work across different translation file formats.src/Parser/ParserRegistry.php (1)
16-16: LGTM! Correct addition of YAML parser to registry.The YamlParser is properly added to the available parsers list, enabling the system to recognize and use YAML translation files.
tests/Build/app/Language/YAML-success/messages.en.yaml (1)
1-11: LGTM! Well-structured YAML test file.The YAML structure is properly formatted with appropriate nesting and covers various message types (greetings, user actions, form elements, errors). This provides good test coverage for YAML translation validation.
tests/Build/app/Language/YAML-fail/messages.de.yaml (1)
1-10: LGTM! Intentionally incomplete YAML file for mismatch testing.This German translation file correctly omits the
form.cancelkey compared to the English version, making it perfect for testing the MismatchValidator's ability to detect missing translation keys across different language files.src/Validator/DuplicatesValidator.php (2)
8-8: LGTM! Proper import added.The XliffParser import is correctly added to support the new
supportsParser()method.
75-81: ```shell
#!/bin/bashCheck supported parsers in MismatchValidator
rg "public function supportsParser" -A 5 -B 2 src/Validator/MismatchValidator.php
</details> <details> <summary>tests/Build/app/Language/YAML-success/messages.de.yaml (1)</summary> `1-11`: **LGTM! Well-structured YAML test data.** The YAML structure is valid and provides good coverage of typical translation scenarios including nested keys for different UI components. The German translations appear accurate and will serve well for testing YAML parsing functionality. </details> <details> <summary>tests/Build/app/composer.json (1)</summary> `27-32`: **Comprehensive YAML test script coverage added.** The new YAML validation scripts follow the established pattern and provide good test coverage. The naming convention is consistent with existing XLIFF scripts. Note that there's no corresponding `translation:yaml:duplicates` script - this appears intentional based on the validator implementations shown in the relevant code snippets where `DuplicatesValidator` only supports `XliffParser`. </details> <details> <summary>tests/Build/app/Language/YAML-fail/messages.en.yaml (1)</summary> `1-11`: **Verify this file represents a valid failure test case.** This YAML file appears structurally identical to the success case. For effective testing of failure scenarios, the file should contain issues that validators are expected to catch (e.g., missing keys, malformed YAML, or translation mismatches). Please ensure this file actually triggers validation failures as intended for the test suite. </details> <details> <summary>src/Command/ValidateTranslationCommand.php (3)</summary> `41-41`: **Good change to support flexible file detection.** Making the file-detector option optional aligns with the enhanced file detection system that can now use multiple detectors when none is specified. --- `55-55`: **Minor formatting improvement.** The added blank line improves code readability by separating the initialization from the subsequent logic. --- `61-65`: ```shell #!/bin/bash # Show collectFiles method implementation for analysis rg -n -A200 "public function collectFiles" src/FileDetector/Collector.phpsrc/Validator/AbstractValidator.php (2)
41-50: Well-implemented parser support validation.The parser compatibility check correctly prevents validators from processing unsupported file formats and provides appropriate debug logging. The use of
in_array()with strict comparison is good practice.
70-73: Good architectural pattern for parser support declaration.The abstract method requirement ensures all concrete validators explicitly declare their parser compatibility, which enhances maintainability and prevents runtime issues.
src/FileDetector/PrefixFileDetector.php (1)
20-26: Improved regex patterns for better language code handling.The updated patterns correctly handle:
- Language codes with optional region subtags (e.g., "en-US", "en_GB")
- Files without language prefixes but with single dots
The key extraction logic using
$matches[2] ?? $matches[0]appropriately handles both cases.src/FileDetector/Collector.php (2)
25-25: Good flexibility enhancement with optional detector parameter.Making the detector parameter optional allows the collector to dynamically choose appropriate detectors when none is specified.
50-60: Well-implemented fallback detection logic.The conditional logic correctly handles both explicit detector usage and automatic detector selection. The
continue 2statement appropriately breaks out of nested loops when a detector successfully maps translation sets.src/Parser/YamlParser.php (2)
24-43: Efficient recursive key extraction implementation.The recursive approach correctly extracts all leaf keys in dot notation. The implementation handles nested arrays properly and builds full key paths correctly.
46-60: Robust key-based content retrieval.The method correctly navigates nested array structures and validates that the final value is a string before returning it. The null returns for missing keys are appropriate.
src/Parser/XliffParser.php (1)
45-48: LGTM!Static method correctly returns supported XLIFF file extensions.
| preg_match('/^([^.]+)\.[a-z]{2}([-_][A-Z]{2})?(\.ya?ml|\.xlf)?$/i', $basename, $matches) // Suffix | ||
| || preg_match('/^[^.]+\.(ya?ml|xlf)$/i', $basename, $matches) // No prefix, only one dot (e.g. locallang_be.xlf) | ||
| ) { | ||
| $key = $matches[1]; |
There was a problem hiding this comment.
Fix regex patterns and key extraction logic.
There are several issues with the current implementation:
- The first regex pattern makes the file extension optional (
?), which could match files without extensions - The second regex pattern doesn't capture the basename, so
$matches[1]would be the extension, not the base name for grouping - The key extraction logic
$key = $matches[1];won't work correctly for the second pattern
Consider this corrected implementation:
- if (
- preg_match('/^([^.]+)\.[a-z]{2}([-_][A-Z]{2})?(\.ya?ml|\.xlf)?$/i', $basename, $matches) // Suffix
- || preg_match('/^[^.]+\.(ya?ml|xlf)$/i', $basename, $matches) // No prefix, only one dot (e.g. locallang_be.xlf)
- ) {
- $key = $matches[1];
- $groups[$key][] = $file;
- }
+ if (preg_match('/^([^.]+)\.[a-z]{2}(?:[-_][A-Z]{2})?\.(ya?ml|xlf)$/i', $basename, $matches)) {
+ // Files like "messages.en.yaml" or "messages.en-US.xlf"
+ $key = $matches[1];
+ $groups[$key][] = $file;
+ } elseif (preg_match('/^([^.]+)\.(ya?ml|xlf)$/i', $basename, $matches)) {
+ // Files like "messages.yaml" or "locallang_be.xlf"
+ $key = $matches[1];
+ $groups[$key][] = $file;
+ }📝 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.
| preg_match('/^([^.]+)\.[a-z]{2}([-_][A-Z]{2})?(\.ya?ml|\.xlf)?$/i', $basename, $matches) // Suffix | |
| || preg_match('/^[^.]+\.(ya?ml|xlf)$/i', $basename, $matches) // No prefix, only one dot (e.g. locallang_be.xlf) | |
| ) { | |
| $key = $matches[1]; | |
| if (preg_match('/^([^.]+)\.[a-z]{2}(?:[-_][A-Z]{2})?\.(ya?ml|xlf)$/i', $basename, $matches)) { | |
| // Files like "messages.en.yaml" or "messages.en-US.xlf" | |
| $key = $matches[1]; | |
| $groups[$key][] = $file; | |
| } elseif (preg_match('/^([^.]+)\.(ya?ml|xlf)$/i', $basename, $matches)) { | |
| // Files like "messages.yaml" or "locallang_be.xlf" | |
| $key = $matches[1]; | |
| $groups[$key][] = $file; | |
| } |
🤖 Prompt for AI Agents
In src/FileDetector/SuffixFileDetector.php around lines 21 to 24, the regex
patterns and key extraction logic are incorrect. Fix the first regex to require
the file extension by removing the optional quantifier, and update the second
regex to capture the basename before the extension. Adjust the key extraction to
use the correct capture group from the matched pattern so that $key correctly
holds the basename for both regex cases.
Summary by CodeRabbit
New Features
.yml,.yaml) translation files alongside XLIFF formats.Documentation
Chores
.gitignoreto exclude IDE configuration directory.