Skip to content

Conversation

@ildyria
Copy link
Member

@ildyria ildyria commented Nov 15, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced currency validation with improved error handling. The system now gracefully handles environments missing required PHP extensions and provides clearer feedback when validation cannot be completed.

@ildyria ildyria requested a review from a team as a code owner November 15, 2025 11:33
@coderabbitai
Copy link

coderabbitai bot commented Nov 15, 2025

📝 Walkthrough

Walkthrough

The CURRENCY validation case in app/Models/Configs.php now wraps ISO 4217 code validation logic in a try/catch block. The catch handles missing php-intl extension by setting an appropriate error message. Existing validation behavior is preserved.

Changes

Cohort / File(s) Summary
Currency validation error handling
app/Models/Configs.php
Wrapped CURRENCY case validation logic in try/catch block to handle missing php-intl extension; explicit "not found" handling moved inside try block; Throwable catch sets error message for unavailable intl extension

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify that catching Throwable is the appropriate exception type versus a more specific exception class
  • Confirm error message strings are consistent with project conventions and user-facing communication standards
  • Validate that the not-found case handling logic inside the try block matches previous behavior and intent

Poem

🐰 A currency caught in a try-catch dance,
Where intl extensions need their chance,
Error handling hops through the code so clean,
The finest validation ever seen! ✨

Pre-merge checks

✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between b27b6c5 and b9c7620.

📒 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

@ildyria ildyria enabled auto-merge (squash) November 15, 2025 11:39
@codecov
Copy link

codecov bot commented Nov 15, 2025

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.37%. Comparing base (fad833e) to head (b9c7620).
⚠️ Report is 2 commits behind head on master.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ildyria ildyria merged commit 6d1bfeb into master Nov 15, 2025
45 checks passed
@ildyria ildyria deleted the no-crash-no-php-intl branch November 15, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants