Do not crash Diagnostics on php-intl missing #3807
Conversation
📝 WalkthroughWalkthroughThe CURRENCY validation case in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Models/Configs.php (1)
156-176: Excellent exception handling for missing php-intl extension.The try-catch wrapper correctly prevents crashes when the php-intl extension is missing. The error message is clear and helpful, and all execution paths (currency found, not found, or extension missing) are handled correctly.
Optional micro-optimization: The currency lookup could be slightly more efficient by checking key existence directly rather than looping:
try { $bundle = \ResourceBundle::create('en', 'ICUDATA-curr'); $currencies = $bundle->get('Currencies'); - $found = false; - foreach ($currencies as $code => $data) { - $found = ($code === $candidate_value); - if ($found) { - break; // we found it, stop searching - } - } - if (!$found) { + if (!isset($currencies[$candidate_value])) { $message = sprintf($message_template, 'a valid ISO 4217 currency code'); break; } break; } catch (\Throwable) {However, the current implementation is perfectly acceptable and clear—this optimization can safely be deferred or skipped entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Models/Configs.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3698
File: resources/js/lychee.d.ts:33-44
Timestamp: 2025-09-20T17:43:08.152Z
Learning: Lychee's currency validation in app/Models/Configs.php uses PHP's ResourceBundle with ICU currency data ('ICUDATA-curr') to validate ISO 4217 currency codes. This authoritative approach using ICU data is comprehensive and stays current automatically, making frontend validation redundant.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3698
File: resources/js/lychee.d.ts:33-44
Timestamp: 2025-09-20T17:43:08.152Z
Learning: Backend currency validation in Lychee is handled by the Configs::sanity() method using ICU currency data bundle to validate ISO-4217 currency codes. Frontend validation is not required as backend validation is sufficient.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3703
File: app/Services/MoneyService.php:29-32
Timestamp: 2025-09-21T20:22:26.400Z
Learning: In the Lychee webshop implementation, configuration keys like 'webshop_currency' are guaranteed to exist through the migration system and ConfigIntegrity middleware (SE_FIELDS), making additional null checks or fallbacks unnecessary when ildyria has architectural guarantees in place.
📚 Learning: 2025-09-20T17:43:08.152Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3698
File: resources/js/lychee.d.ts:33-44
Timestamp: 2025-09-20T17:43:08.152Z
Learning: Lychee's currency validation in app/Models/Configs.php uses PHP's ResourceBundle with ICU currency data ('ICUDATA-curr') to validate ISO 4217 currency codes. This authoritative approach using ICU data is comprehensive and stays current automatically, making frontend validation redundant.
Applied to files:
app/Models/Configs.php
📚 Learning: 2025-09-20T17:43:08.152Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3698
File: resources/js/lychee.d.ts:33-44
Timestamp: 2025-09-20T17:43:08.152Z
Learning: Backend currency validation in Lychee is handled by the Configs::sanity() method using ICU currency data bundle to validate ISO-4217 currency codes. Frontend validation is not required as backend validation is sufficient.
Applied to files:
app/Models/Configs.php
📚 Learning: 2025-09-20T17:19:38.868Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3698
File: app/Services/MoneyService.php:31-32
Timestamp: 2025-09-20T17:19:38.868Z
Learning: In the Lychee codebase, Configs::getValueAsString() calls should not include fallback handling or try/catch blocks, as config validation is handled at the insertion level in the Config system.
Applied to files:
app/Models/Configs.php
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
Summary by CodeRabbit