-
-
Notifications
You must be signed in to change notification settings - Fork 362
Do not crash Diagnostics on php-intl missing #3807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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