fix: simplify getContentByKey method by removing unused attribute parameter#38
Conversation
WalkthroughThis update removes the unused Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser (Json/Php/Yaml/Xliff)
participant XliffParser (specialized)
Client->>Parser: getContentByKey(key)
alt XliffParser
Parser->>XliffParser: getContentByKey(key)
XliffParser->>XliffParser: hasTargetLanguage()
alt hasTargetLanguage = true
XliffParser->>XliffParser: Return 'target' if not empty, else fallback to 'source'
else hasTargetLanguage = false
XliffParser->>XliffParser: Return 'source' if not empty, else fallback to 'target'
end
XliffParser-->>Client: content or null
else Other Parsers
Parser-->>Client: content or null
end
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: 1
🧹 Nitpick comments (6)
tests/src/Parser/PhpParserTest.php (1)
118-120: Consider refactoring or removing this test method.Since the
$attributeparameter has been removed fromgetContentByKey, this test method no longer serves its original purpose. The three identical method calls don't add meaningful test coverage.You could either:
- Remove this test method entirely since the attribute parameter no longer exists
- Rename and refactor it to test something else meaningful about the
getContentByKeymethod- public function testGetContentByKeyIgnoresAttributeParameter(): void - { - $filePath = __DIR__.'/../Fixtures/translations/php/success/messages.en.php'; - $parser = new PhpParser($filePath); - - // The attribute parameter should be ignored for PHP files - $this->assertSame('Welcome to our application!', $parser->getContentByKey('welcome')); - $this->assertSame('Welcome to our application!', $parser->getContentByKey('welcome')); - $this->assertSame('Welcome to our application!', $parser->getContentByKey('welcome')); - }src/Parser/YamlParser.php (1)
55-58: LGTM! Method signature updated correctly, but comment needs updating.The method signature has been properly updated to match the simplified interface. However, the comment on lines 57-58 is now outdated since the
$attributeparameter no longer exists in theParserInterface.Consider updating or removing the outdated comment:
- // Note: the $attribute parameter is required by ParserInterface - // but is not used for YAML, since YAML has no source/target concept. + // YAML files have no source/target concept, so we directly return the value.src/Parser/JsonParser.php (1)
63-66: Update the outdated comment after parameter removal.The method signature change correctly aligns with the
ParserInterface, but the comment still references the removed$attributeparameter.- // Note: the $attribute parameter is required by ParserInterface - // but is not used for JSON, since JSON has no source/target concept. + // Note: JSON has no source/target concept, so content is retrieved directly by key.src/Parser/PhpParser.php (1)
57-60: Update the outdated comment after parameter removal.The method signature change correctly aligns with the
ParserInterface, but the comment still references the removed$attributeparameter.- // Note: the $attribute parameter is required by ParserInterface - // but is not used for PHP files, since PHP has no source/target concept. + // Note: PHP files have no source/target concept, so content is retrieved directly by key.tests/src/Result/ValidationRunTest.php (1)
472-475: Address unused parameter warnings in test mocks.The static analysis tool correctly identifies that the
$keyparameter is unused in these mock implementations. Consider usingunset($key)to explicitly indicate the parameter is intentionally unused.public function getContentByKey(string $key): ?string { + unset($key); return null; }This pattern is already used elsewhere in the file (e.g., line 270) and makes the intention explicit.
Also applies to: 510-513
src/Parser/XliffParser.php (1)
58-70: Review the fallback logic for correctnessThe fallback logic appears to have a potential logical issue:
- When
hasTargetLanguage()is true, we use 'target' as primary but fall back to 'source' (lines 58-63)- When
hasTargetLanguage()is false, we use 'source' as primary but fall back to 'target' (lines 65-70)This seems counterintuitive. If a file has a target language, why would we fall back to source content? The logic should be:
- If target language exists and target content is empty, fall back to source
- If target language doesn't exist and source content is empty, fall back to target
The current implementation seems correct upon closer inspection, but the condition checks are redundant.
Consider simplifying the fallback logic:
- if ('target' === $attribute && $this->hasTargetLanguage()) { + if ('target' === $attribute) { $fallbackContent = (string) $unit->source; if ('' !== $fallbackContent) { return $fallbackContent; } } - if ('source' === $attribute && !$this->hasTargetLanguage()) { + if ('source' === $attribute) { $fallbackContent = (string) $unit->target; if ('' !== $fallbackContent) { return $fallbackContent; } }The additional conditions are redundant since
$attributeis already determined byhasTargetLanguage().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.php-cs-fixer.php(1 hunks)composer.json(1 hunks)src/Parser/JsonParser.php(1 hunks)src/Parser/ParserInterface.php(1 hunks)src/Parser/PhpParser.php(1 hunks)src/Parser/XliffParser.php(2 hunks)src/Parser/YamlParser.php(1 hunks)tests/src/Fixtures/translations/xliff/fail/de.locallang.xlf(1 hunks)tests/src/Fixtures/translations/xliff/fail/de.locallang_be.xlf(1 hunks)tests/src/Fixtures/translations/xliff/success/de.locallang.xlf(1 hunks)tests/src/Fixtures/translations/xliff/success/de.locallang_be.xlf(1 hunks)tests/src/Fixtures/translations/xliff/success/fr.locallang.xlf(1 hunks)tests/src/Parser/PhpParserTest.php(1 hunks)tests/src/Parser/XliffParserTest.php(4 hunks)tests/src/Result/ValidationRunTest.php(3 hunks)tests/src/Validator/AbstractValidatorTest.php(1 hunks)tests/src/Validator/DuplicateValuesValidatorTest.php(3 hunks)tests/src/Validator/EmptyValuesValidatorTest.php(5 hunks)tests/src/Validator/PlaceholderConsistencyValidatorTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/src/Validator/AbstractValidatorTest.php (6)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/JsonParser.php (1)
getContentByKey(63-79)src/Parser/XliffParser.php (1)
getContentByKey(48-75)src/Parser/PhpParser.php (1)
getContentByKey(57-73)src/Parser/YamlParser.php (1)
getContentByKey(55-71)tests/src/Result/ValidationRunTest.php (3)
getContentByKey(278-281)getContentByKey(472-475)getContentByKey(510-513)
src/Parser/YamlParser.php (4)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/JsonParser.php (1)
getContentByKey(63-79)src/Parser/XliffParser.php (1)
getContentByKey(48-75)src/Parser/PhpParser.php (1)
getContentByKey(57-73)
src/Parser/ParserInterface.php (6)
src/Parser/JsonParser.php (1)
getContentByKey(63-79)src/Parser/XliffParser.php (1)
getContentByKey(48-75)src/Parser/PhpParser.php (1)
getContentByKey(57-73)src/Parser/YamlParser.php (1)
getContentByKey(55-71)tests/src/Validator/AbstractValidatorTest.php (1)
getContentByKey(81-84)tests/src/Result/ValidationRunTest.php (3)
getContentByKey(278-281)getContentByKey(472-475)getContentByKey(510-513)
src/Parser/JsonParser.php (4)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/XliffParser.php (1)
getContentByKey(48-75)src/Parser/PhpParser.php (1)
getContentByKey(57-73)src/Parser/YamlParser.php (1)
getContentByKey(55-71)
tests/src/Result/ValidationRunTest.php (5)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/JsonParser.php (1)
getContentByKey(63-79)src/Parser/XliffParser.php (1)
getContentByKey(48-75)src/Parser/PhpParser.php (1)
getContentByKey(57-73)src/Parser/YamlParser.php (1)
getContentByKey(55-71)
src/Parser/XliffParser.php (4)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/JsonParser.php (1)
getContentByKey(63-79)src/Parser/PhpParser.php (1)
getContentByKey(57-73)src/Parser/YamlParser.php (1)
getContentByKey(55-71)
tests/src/Parser/XliffParserTest.php (2)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)src/Parser/XliffParser.php (2)
getContentByKey(48-75)XliffParser(7-104)
🪛 PHPMD (2.15.0)
tests/src/Validator/AbstractValidatorTest.php
81-81: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationRunTest.php
472-472: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
510-510: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (20)
tests/src/Fixtures/translations/xliff/fail/de.locallang_be.xlf (1)
3-3: LGTM: Fixture update supports parser refactoring.The addition of
target-language="de"attribute correctly specifies German as the target language, supporting the simplifiedgetContentByKeymethod that now determines the correct attribute internally.tests/src/Fixtures/translations/xliff/success/de.locallang.xlf (1)
3-3: LGTM: Consistent fixture update for German target language.The addition of
target-language="de"attribute is consistent with the parser refactoring goals and properly specifies German as the target language for this success test fixture.tests/src/Fixtures/translations/xliff/success/fr.locallang.xlf (1)
3-3: LGTM: French target language fixture properly configured.The addition of
target-language="fr"attribute correctly specifies French as the target language, demonstrating the parser's support for multiple target languages in the refactored implementation.tests/src/Fixtures/translations/xliff/fail/de.locallang.xlf (1)
3-3: LGTM: Failure test fixture properly updated.The addition of
target-language="de"attribute is consistent with the parser refactoring and properly configures this failure test fixture to specify German as the target language.tests/src/Fixtures/translations/xliff/success/de.locallang_be.xlf (1)
3-3: LGTM: Backend-specific German fixture properly configured.The addition of
target-language="de"attribute is consistent with the parser refactoring and properly configures this backend-specific German translation fixture.src/Parser/ParserInterface.php (1)
21-21: LGTM! Interface simplification improves API clarity.Removing the unused
$attributeparameter from thegetContentByKeymethod signature is a good simplification. Based on the relevant code snippets, the XLIFF parser now handles attribute selection internally, while other parsers (JSON, PHP, YAML) never used this parameter.tests/src/Validator/PlaceholderConsistencyValidatorTest.php (2)
26-29: LGTM! Mock return map correctly updated for new method signature.The mock return map has been properly updated to reflect the simplified
getContentByKeymethod signature, removing the unused'source'attribute parameter from the return value mappings.
419-422: LGTM! Mock return maps consistently updated.The mock return maps in the complex scenario test have been correctly updated to match the simplified
getContentByKeymethod signature, maintaining consistency with the interface changes.Also applies to: 427-430
tests/src/Validator/AbstractValidatorTest.php (1)
81-84: LGTM! Test stub correctly updated to match interface.The method signature has been properly updated to match the simplified
ParserInterface. The static analysis hint about the unused$keyparameter is expected behavior for a test stub implementation that simply returnsnull.tests/src/Validator/DuplicateValuesValidatorTest.php (1)
32-34: LGTM! Test mocks correctly updated for simplified method signature.The
willReturnMaparrays have been properly updated to remove the unused'source'parameter, correctly aligning with the simplifiedgetContentByKey(string $key)method signature.Also applies to: 58-60, 257-259
tests/src/Validator/EmptyValuesValidatorTest.php (1)
25-27: LGTM! Comprehensive test mock updates correctly align with simplified interface.All
willReturnMaparrays have been consistently updated across multiple test methods to remove the unused'source'parameter, properly reflecting the simplifiedgetContentByKey(string $key)method signature.Also applies to: 45-47, 65-66, 84-87, 245-249
tests/src/Result/ValidationRunTest.php (1)
278-281: LGTM! Mock parser signature correctly updated.The method signature has been properly updated to align with the simplified
ParserInterface::getContentByKey(string $key)method.src/Parser/XliffParser.php (3)
48-48: LGTM: Method signature properly simplifiedThe method signature now correctly matches the
ParserInterfacedefinition, removing the unused$attributeparameter and aligning with other parser implementations.
50-50: Good approach: Automatic attribute determinationUsing
hasTargetLanguage()to automatically determine whether to prioritize 'target' or 'source' is a logical approach that simplifies the API while maintaining functionality.
100-103: LGTM: Clean helper method implementationThe
hasTargetLanguage()method is well-implemented, focused, and provides a clear abstraction for determining the presence of a target language attribute.tests/src/Parser/XliffParserTest.php (4)
51-72: LGTM: Comprehensive target language fixtureThe new fixture file properly includes the
target-language="de"attribute and provides good test data for verifying the enhanced fallback logic, including scenarios with empty target content.
153-156: LGTM: Method calls correctly updatedThe test method calls have been properly updated to use the simplified
getContentByKey(string $key)signature, removing the unused$attributeparameter.
159-167: LGTM: Excellent test coverage for target language behaviorThis test properly verifies the new automatic behavior:
- Returns target content when available
- Falls back to source content when target is empty (key3)
- Returns null for non-existent keys
The test effectively validates the core functionality of the simplified method.
255-319: LGTM: Comprehensive edge case coverageThe additional test methods provide excellent coverage of edge cases:
testGetContentByKeyFallsBackToTargetWhenSourceIsEmpty: Tests fallback in source-primary filestestGetContentByKeyFallsBackToSourceWhenTargetIsEmptyInTargetLanguageFile: Tests fallback in target-primary filestestGetContentByKeyReturnsNullWhenBothSourceAndTargetAreEmpty: Tests behavior when both are emptyThese tests ensure the fallback logic works correctly in all scenarios and provide confidence in the implementation.
.php-cs-fixer.php (1)
16-16: ConfirmsetUnsupportedPhpVersionAllowed()exists in the locked php-cs-fixer versionThe helper was introduced in php-cs-fixer v3.39. Because
composer.jsononly constrains the dependency to^3.52, that should be safe – as long as everybody already rancomposer update.
If a contributor still has an older lock file, the call will trigger a fatal error at runtime.- ->setUnsupportedPhpVersionAllowed(true) + ->setUnsupportedPhpVersionAllowed(true) // requires php-cs-fixer ≥ 3.39Please verify the project’s
composer.lock(and CI) really resolves to ≥ 3.39, otherwise bump the constraint or add an explicit minimum.
| "fix:composer": "@composer normalize", | ||
| "fix:editorconfig": "@lint:editorconfig --fix", | ||
| "fix:php": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix", | ||
| "fix:php": "php-cs-fixer fix", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dropping the PHP_CS_FIXER_IGNORE_ENV safeguard might break on unsupported runtimes
The new config flag should supersede the env-var, but php-cs-fixer aborts before reading the config when the running PHP version is too new/old (< 7.4 or > current supported). In such a scenario the script will exit before setUnsupportedPhpVersionAllowed(true) can take effect.
Consider keeping the environment override for full backward-compatibility, or document the minimum php-cs-fixer version required for this workflow.
- "fix:php": "php-cs-fixer fix",
+ "fix:php": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix",(or leave as-is but make sure CI never runs with an unsupported PHP version).
🏁 Script executed:
#!/bin/bash
echo "php-cs-fixer constraint in composer.json:"
rg -n '"friendsofphp/php-cs-fixer"' composer.json
echo ""
echo "Nearby lines around require-dev block:"
rg -n '"require-dev"' -n -A10 composer.json
echo ""
echo "Listing php-cs-fixer config files in repo:"
fd -I ".php-cs-fixer" -t f || echo "No php-cs-fixer config found"Length of output: 909
🏁 Script executed:
#!/bin/bash
echo "Searching for setUnsupportedPhpVersionAllowed occurrences:"
rg -n "setUnsupportedPhpVersionAllowed" || echo "None found"
echo ""
echo "Listing potential php-cs-fixer config files:"
fd --hidden -t f -e php \
-g ".php-cs-fixer.php" \
-g "php-cs-fixer.php" \
-g ".php-cs-fixer.dist.php" \
-g ".php_cs.dist" \
-g ".php_cs" || echo "None found"Length of output: 744
Re-introduce the PHP_CS_FIXER_IGNORE_ENV safeguard for unsupported PHP versions
Without a project‐level php-cs-fixer config, php-cs-fixer will abort before reading any setUnsupportedPhpVersionAllowed(true) call on a too-new/old PHP runtime. Re-adding the env override ensures backward-compatibility:
File: composer.json (line 69)
- "fix:php": "php-cs-fixer fix",
+ "fix:php": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix",Alternatively, you can:
- Add a
.php-cs-fixer.phpconfig file at the repo root that callssetUnsupportedPhpVersionAllowed(true). - Or document and enforce the minimum/maximum PHP version in CI to match php-cs-fixer ^3.52.
📝 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.
| "fix:php": "php-cs-fixer fix", | |
| "fix:php": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix", |
🤖 Prompt for AI Agents
In composer.json at line 69, re-add the PHP_CS_FIXER_IGNORE_ENV environment
variable safeguard to the "fix:php" script to prevent php-cs-fixer from aborting
on unsupported PHP versions. Modify the script to include this environment
variable before the php-cs-fixer command, ensuring backward compatibility
without requiring a project-level config or strict PHP version enforcement in
CI.
Summary by CodeRabbit
Refactor
Tests