Refactor: Modularize Locale service and fix DSN path parsing#11152
Refactor: Modularize Locale service and fix DSN path parsing#11152khoinp1012 wants to merge 1 commit intoappwrite:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR performs a major architectural refactoring by removing the legacy Swoole-based HTTP server implementation ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes NotesThe review requires verification across multiple dimensions: confirming the removed HTTP server logic has been properly replaced by the new modular architecture, ensuring all eight migrated locale endpoints function equivalently to their predecessors, validating that no request handling or error logic is lost during the transition, and checking that module registration is correctly integrated. While the eight new endpoint classes follow a highly repetitive pattern (reducing individual review burden per file), the two large file deletions—particularly 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Modules/Locale/Http/Locale/Get.php`:
- Around line 71-75: The foreach loop is declaring an unused key variable
($code) and keeps scanning after a match; change the loop to iterate over values
only (foreach ($currencies as $element)) and add a break once you assign
$currency inside the conditional that checks isset($element['locations']) &&
isset($element['code']) && in_array($record['country']['iso_code'],
$element['locations']) so the loop stops after the first match.
In `@src/Appwrite/Platform/Modules/Locale/Http/Locale/ListCountriesPhones.php`:
- Around line 52-60: The foreach currently declares ($list as $code => $name)
but still references $list[$code], leaving $name unused; update the loop in
ListCountriesPhones (the foreach over $list) to use the provided $name variable
where the phone value is needed (replace $list[$code] with $name) when
constructing the Document (the 'code' => '+' . ... expression) so you avoid the
redundant array lookup and the unused $name variable.
| foreach ($currencies as $code => $element) { | ||
| if (isset($element['locations']) && isset($element['code']) && \in_array($record['country']['iso_code'], $element['locations'])) { | ||
| $currency = $element['code']; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unused loop variable $code.
The static analysis tool correctly identifies that $code is unused in this loop. Additionally, the loop continues iterating even after finding a matching currency, which is inefficient.
Proposed fix
- foreach ($currencies as $code => $element) {
- if (isset($element['locations']) && isset($element['code']) && \in_array($record['country']['iso_code'], $element['locations'])) {
- $currency = $element['code'];
- }
- }
+ foreach ($currencies as $element) {
+ if (isset($element['locations'], $element['code']) && \in_array($record['country']['iso_code'], $element['locations'])) {
+ $currency = $element['code'];
+ break;
+ }
+ }🧰 Tools
🪛 PHPMD (2.15.0)
71-71: Avoid unused local variables such as '$code'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Appwrite/Platform/Modules/Locale/Http/Locale/Get.php` around lines 71 -
75, The foreach loop is declaring an unused key variable ($code) and keeps
scanning after a match; change the loop to iterate over values only (foreach
($currencies as $element)) and add a break once you assign $currency inside the
conditional that checks isset($element['locations']) && isset($element['code'])
&& in_array($record['country']['iso_code'], $element['locations']) so the loop
stops after the first match.
| \asort($list); | ||
|
|
||
| foreach ($list as $code => $name) { | ||
| if ($locale->getText('countries.' . \strtolower($code), false) !== false) { | ||
| $output[] = new Document([ | ||
| 'code' => '+' . $list[$code], | ||
| 'countryCode' => $code, | ||
| 'countryName' => $locale->getText('countries.' . \strtolower($code)), | ||
| ]); |
There was a problem hiding this comment.
Use the foreach value to avoid an unused variable.
Line 54 introduces $name but it is never used; use it directly and avoid the extra $list[$code] lookup.
🛠️ Proposed fix
- foreach ($list as $code => $name) {
+ foreach ($list as $code => $name) {
if ($locale->getText('countries.' . \strtolower($code), false) !== false) {
$output[] = new Document([
- 'code' => '+' . $list[$code],
+ 'code' => '+' . $name,
'countryCode' => $code,
'countryName' => $locale->getText('countries.' . \strtolower($code)),
]);
}
}🧰 Tools
🪛 PHPMD (2.15.0)
54-54: Avoid unused local variables such as '$name'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Appwrite/Platform/Modules/Locale/Http/Locale/ListCountriesPhones.php`
around lines 52 - 60, The foreach currently declares ($list as $code => $name)
but still references $list[$code], leaving $name unused; update the loop in
ListCountriesPhones (the foreach over $list) to use the provided $name variable
where the phone value is needed (replace $list[$code] with $name) when
constructing the Document (the 'code' => '+' . ... expression) so you avoid the
redundant array lookup and the unused $name variable.
1 participant
This Pull Request refactors the Locale Service into a modular, action-based architecture, aligning it with the latest Appwrite Platform standards. It also includes a critical bug fix for database connection strings and improves system startup resilience.
🚀 Key Changes:
Modular Locale Actions: Migrated the monolithic Locale controller logic into individual, reusable Action classes under src/Appwrite/Platform/Modules/Locale/Http/Locale/.
Get: General locale information.
ListCodes: Available locale codes.
ListContinents, ListCountries, ListCountriesEU, ListCountriesPhones: Specialized geography listings.
ListCurrencies: Currency information.
ListLanguages: Language support.
Platform Integration: Integrated the new Locale module into the Appwrite Platform module registration (Appwrite.php).
DSN Path Bug Fix: Fixed an issue in app/init/registers.php where database names were parsed incorrectly if a leading slash was present in the DSN path (e.g., mysql://user:pass@host/db).
Startup Resilience: Increased database connection retry logic in app/http.php to ensure the server starts reliably even when MariaDB takes longer to initialize in some environments.
🛠 Verification:
Created and executed standalone unit tests within the Docker environment to verify the new Action classes.
Verified that all Locale endpoints correctly load their respective data from the Utopia\Locale source.
Validated that the ltrim fix correctly resolves the "Unknown database" errors caused by DSN formatting.