Issue/533 lang adapters#561
Conversation
📝 WalkthroughWalkthroughRefactors the Lang package around configurable adapters. File translations move to ChangesLang adapter architecture
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Application
participant LangFactory
participant Lang
participant DeepLAdapter
participant RemoteAdapterTrait
participant Cache
participant DeepL
Application->>LangFactory: request configured adapter
LangFactory->>DeepLAdapter: create adapter for locale
LangFactory-->>Application: return Lang instance
Application->>Lang: getTranslation(key, params)
Lang->>DeepLAdapter: get(key, params)
DeepLAdapter->>RemoteAdapterTrait: build source text and cache key
RemoteAdapterTrait->>Cache: read cached translation
alt cache miss
RemoteAdapterTrait->>DeepL: send translation request
DeepL-->>RemoteAdapterTrait: return provider response
RemoteAdapterTrait->>Cache: store translation
end
DeepLAdapter-->>Lang: return translated text
Lang-->>Application: return translated text
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d2e64c17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Lang/Lang.php (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
getTranslationreturn type could bestringinstead of?string.The adapter's
get()always returnsstring(either the translation or the key itself), so?stringis overly permissive. Tightening this tostringwould give callers a more precise contract.🔧 Suggested refinement
- public function getTranslation(string $key, array|string $params = null): ?string + public function getTranslation(string $key, array|string $params = null): string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Lang/Lang.php` around lines 61 - 64, Update the return type of Lang::getTranslation from ?string to string to match the adapter->get contract, while preserving the existing delegation and parameters.src/Lang/Adapters/FileAdapter.php (1)
88-100: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider using
!== nullinstead of truthiness for$paramscheck.Line 96 uses
return $params ? _message($message, $params) : $message;. A string param of'0'or an empty array[]would be falsy and skip interpolation. While[]correctly means "no params," the'0'edge case is surprising. Using$params !== nullwould be more precise and avoid the'0'pitfall.♻️ Suggested refinement
- return $params ? _message($message, $params) : $message; + return $params !== null ? _message($message, $params) : $message;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Lang/Adapters/FileAdapter.php` around lines 88 - 100, Update the parameter check in FileAdapter::get to use an explicit null comparison, ensuring a string value of "0" still reaches _message while null skips interpolation; preserve the existing translation lookup and fallback behavior.src/Lang/Adapters/GoogleTranslateAdapter.php (1)
65-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMove API key from URL query parameter to
x-goog-api-keyheader.The API key is currently embedded in the request URL via
http_build_query($query), exposing it in proxy logs, access logs, and any HttpClient URL logging. Google Cloud APIs support thex-goog-api-keyheader as a more secure alternative.🔒 Proposed fix
$query = [ 'q' => $text, 'target' => $this->lang, 'format' => 'text', - 'key' => $apiKey, ]; if (!empty($this->params['source_locale'])) { $query['source'] = (string) $this->params['source_locale']; } $response = $this->sendRequest( (string) ($this->params['api_url'] ?? self::API_URL) . '?' . http_build_query($query, '', '&'), null, - [], + ['x-goog-api-key' => $apiKey], 'POST' );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Lang/Adapters/GoogleTranslateAdapter.php` around lines 65 - 81, Update the request construction in GoogleTranslateAdapter so the API key is removed from the $query parameters and sent through the x-goog-api-key header passed to sendRequest. Keep the remaining query parameters and POST behavior unchanged, ensuring the key is no longer included in the URL generated by http_build_query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Lang/Exceptions/LangException.php`:
- Around line 46-54: Update LangException::providerRequestFailed so the
null-details branch does not add an extra period, while retaining the existing
colon-and-details formatting when details are provided. Ensure the
PROVIDER_REQUEST_FAILED template supplies the single final period in the
no-details message.
- Around line 46-54: Update the call to LangException::providerRequestFailed in
RemoteAdapterTrait::sendRequest to pass the adapter’s existing human-readable
provider label instead of static::class. Preserve the current details argument
and error behavior, matching the display names used by other language errors
such as DeepL and Google Translate.
In `@src/Lang/Factories/LangFactory.php`:
- Around line 61-79: Update LangFactory::resolve to validate the adapter after
applying the configured lang.default value, and throw a descriptive
LangException when it is missing or null before calling getAdapterClass or
indexing instances. Preserve the existing adapter resolution and instance
creation flow for valid adapter strings.
---
Nitpick comments:
In `@src/Lang/Adapters/FileAdapter.php`:
- Around line 88-100: Update the parameter check in FileAdapter::get to use an
explicit null comparison, ensuring a string value of "0" still reaches _message
while null skips interpolation; preserve the existing translation lookup and
fallback behavior.
In `@src/Lang/Adapters/GoogleTranslateAdapter.php`:
- Around line 65-81: Update the request construction in GoogleTranslateAdapter
so the API key is removed from the $query parameters and sent through the
x-goog-api-key header passed to sendRequest. Keep the remaining query parameters
and POST behavior unchanged, ensuring the key is no longer included in the URL
generated by http_build_query.
In `@src/Lang/Lang.php`:
- Around line 61-64: Update the return type of Lang::getTranslation from ?string
to string to match the adapter->get contract, while preserving the existing
delegation and parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c94dd36-ca62-4aaa-bb26-d727f872ee03
📒 Files selected for processing (21)
CHANGELOG.mdsrc/App/Adapters/WebAppAdapter.phpsrc/App/Traits/WebAppTrait.phpsrc/Lang/Adapters/DeepLAdapter.phpsrc/Lang/Adapters/FileAdapter.phpsrc/Lang/Adapters/GoogleTranslateAdapter.phpsrc/Lang/Contracts/LangAdapterInterface.phpsrc/Lang/Enums/ExceptionMessages.phpsrc/Lang/Enums/LangType.phpsrc/Lang/Exceptions/LangException.phpsrc/Lang/Factories/LangFactory.phpsrc/Lang/Lang.phpsrc/Lang/Traits/RemoteAdapterTrait.phptests/Unit/Lang/Adapters/DeepLAdapterTest.phptests/Unit/Lang/Adapters/FileAdapterTest.phptests/Unit/Lang/Adapters/GoogleTranslateAdapterTest.phptests/Unit/Lang/Factories/LangFactoryTest.phptests/Unit/Lang/Helpers/LangHelperFunctionsTest.phptests/Unit/Lang/LangTest.phptests/Unit/Lang/TranslatorTest.phptests/_root/shared/config/lang.php
💤 Files with no reviewable changes (4)
- src/App/Adapters/WebAppAdapter.php
- tests/Unit/Lang/TranslatorTest.php
- tests/Unit/Lang/Helpers/LangHelperFunctionsTest.php
- src/App/Traits/WebAppTrait.php
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Lang/Adapters/DeepLAdapter.php (1)
99-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisleading exception for local
json_encodefailure.
buildPayload()throwsLangException::invalidProviderResponse('DeepL')whenjson_encodefails, but this is a local payload encoding failure, not an invalid provider response. The resulting message ("The providerDeepLreturned an invalid translation response") will mislead debugging. Consider a more accurate error such as aLangExceptionwith a message indicating payload encoding failure, or a genericRuntimeException.The same pattern exists in
GoogleTranslateAdapter::buildPayload()(line 103).Proposed fix
if ($payloadJson === false) { - throw LangException::invalidProviderResponse('DeepL'); + throw new LangException('Failed to encode DeepL request payload: ' . json_last_error_msg()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Lang/Adapters/DeepLAdapter.php` around lines 99 - 103, Update the json_encode failure handling in buildPayload() for both DeepLAdapter and GoogleTranslateAdapter so it throws an exception describing local payload encoding failure instead of LangException::invalidProviderResponse('DeepL'). Use an appropriate existing LangException factory or RuntimeException, and preserve the current behavior for successful encoding.
🧹 Nitpick comments (1)
tests/Unit/Lang/LangTest.php (1)
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
testLangFlushResetsTranslationsto reflect actual behavior.After
flush(),FileAdapter::get()lazily reloads translations, so the assertion is identical before and after. The test would pass even ifflush()were a no-op. A name liketestLangFlushDoesNotBreakTranslationsor adding an assertion that verifies the cache was actually cleared (e.g., via a spy or by checking reload count) would better communicate intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Lang/LangTest.php` around lines 54 - 61, The test name testLangFlushResetsTranslations does not match its assertions, which only verify translations remain available after flushing. Rename it to describe that flush preserves translation access, such as testLangFlushDoesNotBreakTranslations, without changing the test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Lang/Adapters/DeepLAdapter.php`:
- Around line 69-76: Update DeepLAdapter’s sendRequest call to configure a
finite CURLOPT_TIMEOUT through the existing HttpClient setOpt() mechanism before
executing the request, using the adapter’s established timeout configuration or
constant if available. Ensure all DeepL requests have a bounded timeout instead
of hanging indefinitely.
In `@src/Lang/Adapters/GoogleTranslateAdapter.php`:
- Around line 100-104: Update the json_encode failure handling in
GoogleTranslateAdapter’s payload-building flow to throw the exception intended
for local payload encoding/serialization failures rather than
LangException::invalidProviderResponse('Google Translate'). Keep
provider-response validation errors separate and preserve the existing behavior
for successful encoding.
In `@tests/Unit/Storage/HttpClientTestCase.php`:
- Around line 18-19: Initialize the $data property in HttpClientTestCase to an
empty array instead of leaving it uninitialized, preserving getData() behavior
when setData() has not been called.
---
Outside diff comments:
In `@src/Lang/Adapters/DeepLAdapter.php`:
- Around line 99-103: Update the json_encode failure handling in buildPayload()
for both DeepLAdapter and GoogleTranslateAdapter so it throws an exception
describing local payload encoding failure instead of
LangException::invalidProviderResponse('DeepL'). Use an appropriate existing
LangException factory or RuntimeException, and preserve the current behavior for
successful encoding.
---
Nitpick comments:
In `@tests/Unit/Lang/LangTest.php`:
- Around line 54-61: The test name testLangFlushResetsTranslations does not
match its assertions, which only verify translations remain available after
flushing. Rename it to describe that flush preserves translation access, such as
testLangFlushDoesNotBreakTranslations, without changing the test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f54bc5a-bb45-45e8-93d7-59cf6aba566f
📒 Files selected for processing (14)
src/Lang/Adapters/DeepLAdapter.phpsrc/Lang/Adapters/GoogleTranslateAdapter.phpsrc/Lang/Contracts/LangAdapterInterface.phpsrc/Lang/Enums/ExceptionMessages.phpsrc/Lang/Exceptions/LangException.phpsrc/Lang/Factories/LangFactory.phpsrc/Lang/Lang.phptests/Unit/Lang/Adapters/DeepLAdapterTest.phptests/Unit/Lang/Adapters/FileAdapterTest.phptests/Unit/Lang/Adapters/GoogleTranslateAdapterTest.phptests/Unit/Lang/Exceptions/LangExceptionTest.phptests/Unit/Lang/Factories/LangFactoryTest.phptests/Unit/Lang/LangTest.phptests/Unit/Storage/HttpClientTestCase.php
💤 Files with no reviewable changes (2)
- src/Lang/Contracts/LangAdapterInterface.php
- src/Lang/Lang.php
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Unit/Lang/Adapters/FileAdapterTest.php
- tests/Unit/Lang/Adapters/GoogleTranslateAdapterTest.php
- src/Lang/Factories/LangFactory.php
- tests/Unit/Lang/Factories/LangFactoryTest.php
- src/Lang/Enums/ExceptionMessages.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Lang/Traits/RemoteAdapterTrait.php (1)
48-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the
FileAdapterinstance to avoid repeated disk reads.
buildSourceTextcreatesnew FileAdapter($sourceLocale)on every call when source catalog is enabled. SinceFileAdapter::get()lazily loads translations from disk per instance, translating N keys per request triggers N disk reads. Storing the adapter as a property eliminates this redundancy.♻️ Proposed refactor: reuse FileAdapter instance
trait RemoteAdapterTrait { protected HttpClient $httpClient; + protected ?FileAdapter $sourceCatalogAdapter = null; + /** * `@var` array<string, mixed> */ protected array $params = []; /** * `@param` array<int|string, mixed>|string|null $params * `@throws` LangException|LoaderException|ConfigException|DiException|BaseException|ReflectionException */ protected function buildSourceText(string $key, array|string $params = null): string { if ($this->usesSourceCatalog()) { $sourceLocale = $this->getSourceLocale(); if ($sourceLocale === null || $sourceLocale === '') { throw LangException::missingConfig($this->getSourceLocaleConfigPath()); } - return (new FileAdapter($sourceLocale))->get($key, $params); + if ($this->sourceCatalogAdapter === null) { + $this->sourceCatalogAdapter = new FileAdapter($sourceLocale); + } + + return $this->sourceCatalogAdapter->get($key, $params); } return $params ? _message($key, $params) : $key; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Lang/Traits/RemoteAdapterTrait.php` at line 48, Update buildSourceText to reuse a cached FileAdapter property for the source locale instead of constructing a new FileAdapter on each call. Initialize or refresh the property when the source locale changes, then invoke get on that shared instance while preserving the existing fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Lang/Traits/RemoteAdapterTrait.php`:
- Line 48: Update buildSourceText to reuse a cached FileAdapter property for the
source locale instead of constructing a new FileAdapter on each call. Initialize
or refresh the property when the source locale changes, then invoke get on that
shared instance while preserving the existing fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1efd2644-bdf1-4feb-ac33-3b9131a8154b
📒 Files selected for processing (6)
src/Lang/Adapters/DeepLAdapter.phpsrc/Lang/Adapters/GoogleTranslateAdapter.phpsrc/Lang/Traits/RemoteAdapterTrait.phptests/Unit/Lang/Adapters/DeepLAdapterTest.phptests/Unit/Lang/Adapters/GoogleTranslateAdapterTest.phptests/_root/shared/config/lang.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/_root/shared/config/lang.php
- src/Lang/Adapters/DeepLAdapter.php
- src/Lang/Adapters/GoogleTranslateAdapter.php
3 participants
Closes #533
Summary by CodeRabbit
lang.default(instead ofenabled/translator-style enablement).lang.default_locale.lang.enabledand theLang::isEnabled()API (and the related behavior it implied).