Fix Stripe/Trans amount parser truncating locale-mismatched separators - #3380
vivi-the-going-merry[bot] wants to merge 2 commits into
Conversation
maybe_use_decimal()/normalize_number() decided the decimal separator purely from the currency's configured thousand/decimal separators, never checking whether the amount string already contained the other separator character in a conflicting role. A user typing an amount in a different locale's format than the form's configured currency (e.g. US-style "1,030.21" on a EUR form expecting '.' as the thousand separator) got both punctuation marks collided into one string, and PHP's float cast silently truncated the result by ~1000x (1030.21 -> 1.03). When both '.' and ',' appear in the string, the real decimal separator is now detected from the string itself (whichever separator appears last), instead of trusting the currency's configured roles -- which are only reliable when a single separator character appears. Closes #3379
Self-review surfaced two problems with the first pass: - maybe_use_decimal() and normalize_number() split one decision (which character is the real decimal separator) across two methods, coordinated only by an early-return contract and an intermediate by-reference mutation -- fragile, and redundant (the conflict check ran twice on the same string). - The conflict-case fix picked the right decimal separator character but then swapped every occurrence of it via str_replace. A second occurrence of that same character (e.g. "1,234.567,89") collided into a second decimal point and reproduced the exact ~1000x truncation the fix was written to close, just via three separators instead of two. Both are replaced by a single find_decimal_position() that returns the rightmost real decimal separator's position (or false for a whole-number amount), which normalize_number() then uses to split the string into integer/fractional parts and strip every other separator occurrence as grouping noise -- so a repeated separator character can no longer collide into a second decimal point.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Sep 18, 2026 4:56p.m. | Review ↗ |
| JavaScript | |
Sep 18, 2026 4:56p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid fix — the find_decimal_position() design (rightmost separator wins when both appear, single-separator heuristic preserved exactly) is correct and the new test coverage (mixed separators, repeated-separator collision, thousands-only) exercises the right edge cases. One blocking CI failure below; everything else is clean.
| } | ||
|
|
||
| $present = false !== $last_dot ? '.' : ','; | ||
| $position = false !== $last_dot ? $last_dot : $last_comma; |
There was a problem hiding this comment.
Blocking — CI is red on this line (PHPStan: Only numeric types are allowed in -, int|false given on the right side at line 425, https://github.com/Strategy11/formidable-forms/actions/runs/35371337494/job/105685768338).
Runtime-wise $position can never actually be false here — the two guards above establish exactly one of $last_dot/$last_comma is non-false — but PHPStan can't derive that XOR relationship across two separate if blocks, so it sees $position as int|false going into the subtraction on line 425. Add an explicit narrowing guard right after the assignment:
| $position = false !== $last_dot ? $last_dot : $last_comma; | |
| $position = false !== $last_dot ? $last_dot : $last_comma; | |
| if ( false === $position ) { | |
| // Unreachable given the guards above -- narrows the type for PHPStan. | |
| return false; | |
| } |
What was broken
FrmTransLiteActionsController::maybe_use_decimal()/normalize_number()decided the decimal separator purely from the currency's configuredthousand_separator/decimal_separator, never checking whether the amount string already contained the other separator character in a conflicting role.A user typing an amount in a different locale's format than the form's configured currency (e.g. US-style
1,030.21on a EUR form expecting.as the thousand separator) got both punctuation marks collided into one string (1.030.21), and PHP's(float)cast silently stopped parsing at the second., truncating the charge amount by ~1000x (1030.21→1.03).What changed
maybe_use_decimal()andnormalize_number()are replaced by a singlefind_decimal_position()that returns the position of the real decimal separator (orfalsefor a whole-number amount):.and,appear in the string, whichever one appears last is the real decimal point, regardless of what the currency configures..with a 1-2 digit tail on a.-thousands currency still reads as a decimal point; a lone,on a,-thousands currency is never reinterpreted as decimal — both existing, tested behaviors).normalize_number()then splits the string at that position and strips every other occurrence of either separator character as grouping noise, rather than blanket-replacing by character — closing a second gap found during self-review, where a repeated occurrence of the resolved decimal character (e.g.1,234.567,89) could still collide into a second decimal point and reproduce the same truncation via three separators instead of two.How it was verified
1030.21→1.03truncation and its GBP-form mirror image), then confirmed green after the fix.find_decimal_position()unit coverage (tests/phpunit/stripe/test_FrmTransLiteActionsController.php) and end-to-endprepare_amount()regression cases (tests/phpunit/square/test_FrmSquareLiteAppController.php's sharedamount_format_provider), including the reported bug, its mirror image, and the repeated-separator adversarial case.run testslabel for CI's own PHPUnit run.Known follow-up (out of scope for this PR)
FrmCurrencyHelper::prepare_price()/maybe_use_decimal()(classes/helpers/FrmCurrencyHelper.php) has the same root-cause bug in a separate code path (Total field price calculations, consumed byFrmFieldTotal) — confirmed via the same trace, not touched here since it's a different consumer than what this issue reports. Filing a follow-up issue for it.Closes #3379