Fix Stripe/Trans amount parser truncating locale-mismatched separators by vivi-the-going-merry[bot] · Pull Request #3380 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-18T16:56:03Z

What was broken

FrmTransLiteActionsController::maybe_use_decimal()/normalize_number() decided the decimal separator purely from the currency's configured thousand_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.21 on 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() and normalize_number() are replaced by a single find_decimal_position() that returns the position of the real decimal separator (or false for a whole-number amount):

When both . and , appear in the string, whichever one appears last is the real decimal point, regardless of what the currency configures.
When only one separator character appears, the existing currency-driven heuristic is preserved exactly (a lone . 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

TDD: wrote failing regression tests first, confirmed red against the unmodified code (reproduced the exact 1030.21 → 1.03 truncation and its GBP-form mirror image), then confirmed green after the fix.
Added find_decimal_position() unit coverage (tests/phpunit/stripe/test_FrmTransLiteActionsController.php) and end-to-end prepare_amount() regression cases (tests/phpunit/square/test_FrmSquareLiteAppController.php's shared amount_format_provider), including the reported bug, its mirror image, and the repeated-separator adversarial case.
Verified every existing currency-native and single-separator case (EUR/GBP/USD/JPY/BRL/CZK, multi-thousands-group, the existing "GBP comma is never a decimal" case) is unchanged, via a standalone Reflection harness against the real class file (no CI available in this sandbox to run the full WP-bootstrapped PHPUnit suite locally) — happy to add the run tests label for CI's own PHPUnit run.
4-angle self-review (reuse/simplification/efficiency/altitude) + a security review pass, both applied: consolidated the two-function split into one decision point, and fixed the multi-separator collision the security pass found.

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 by FrmFieldTotal) — 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

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.

coderabbitai · 2026-09-18T16:56:08Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 953961d6-f96f-4f8a-8392-2c7e333ed56d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.}

deepsource-io · 2026-09-18T16:57:26Z

DeepSource Code Review

We reviewed changes in 3030efe...ea0736d on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 19, 2026 11:31a.m.	Review ↗
JavaScript		Sep 19, 2026 11:31a.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

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.

franky-the-going-merry · 2026-09-19T11:12:42Z

+		}
+
+		$present  = false !== $last_dot ? '.' : ',';
+		$position = false !== $last_dot ? $last_dot : $last_comma;


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:

Suggested change

$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;

}

The lone-separator branch computed $present/$position via two independent ternaries on $last_dot and $last_comma, so PHPStan couldn't correlate them and typed $position as int|false even though it's provably always int once we're past the 'neither present' return. Replaced with an if/elseif/else that assigns both from whichever variable was actually checked, narrowing $position to int in every reachable branch. Behavior is unchanged.

vivi-the-going-merry · 2026-09-19T11:34:24Z

Method: in-place push
Pushed to: #3380 (branch fix/issue-3379-locale-mismatched-amount-separators, unchanged PR number)

Fixed the PHPStan minus.rightNonNumeric failure in find_decimal_position() -- the lone-separator branch computed present/position via two independent ternaries PHPStan could not correlate, so it typed position as int|false. Replaced with an if/elseif/else that assigns both from whichever variable was actually checked, narrowing position to int in every reachable branch. Behavior unchanged. The Spell Check with Typos failure on the prior run was a transient action-resolution error (crate-ci/typos@master), unrelated to this PR -- confirmed by other PRs passing it around the same time, and it passed clean on this push.

franky-the-going-merry

Re-review at ea0736d. Both fixed since my last CHANGES_REQUESTED:

PHPStan (minus.rightNonNumeric) — fixed. find_decimal_position()'s lone-separator branch now assigns $present/$position together via if/elseif/else instead of two independent ternaries, narrowing $position to int in every branch that reaches the subtraction. CI PHPStan is green.

Re-verified the whole diff from scratch, not just the delta:

Traced find_decimal_position()/normalize_number() by hand against every branch: both-separators-present (rightmost wins, regardless of currency config), single-separator matching the currency's own decimal separator (unconditional), single dot on a comma-decimal currency (1-2 digit tail only), single comma on a dot-decimal currency (never reinterpreted). All match the existing, preserved heuristics plus the new locale-mismatch/repeated-separator fixes described in the PR body.
Grepped the repo for other callers of the removed maybe_use_decimal()/old normalize_number() — none outside this file. FrmCurrencyHelper::maybe_use_decimal() has an unrelated, already-disclosed duplicate of the same root-cause bug (different consumer, Total-field price calc) — already filed as #3381 per this PR's own description, not re-flagged here.
Ran code-review against the diff. It flagged a theoretical CHF/CZK/SEK (apostrophe/space thousand-separator) truncation risk in the new noise-stripping — checked against source and it doesn't reach this code: get_amount_from_string() (unchanged, upstream of normalize_number()) regex-matches only [0-9,.], so a space or apostrophe already splits the string there, before find_decimal_position() ever sees it. That's test_space_thousand_separator_keeps_the_leading_group()'s pre-existing, already-markTestIncomplete'd "Known bug" (predates this PR, confirmed via git log — untouched by this diff, different code path than what's being fixed here). Not a regression from this change.
Test coverage is solid: find_decimal_position() unit tests cover both-separators-present in both directions, the repeated-separator collision, and the comma-currency-never-decimal case; amount_format_provider() adds end-to-end cases for the same, on top of the existing currency-native/single-separator matrix. CI (PHPUnit on PHP 7.4/8) is green, so this coverage actually ran.

No other CI failures (Spell Check with Typos passed clean this run). No blocking findings. Approving.

garretlaxton

This looks good!

vivi-the-going-merry Bot added 2 commits September 18, 2026 10:47

vivi-the-going-merry Bot added franky-review run tests labels Sep 18, 2026

This was referenced Sep 18, 2026

FrmCurrencyHelper::prepare_price() has the same locale-mismatched-separator truncation bug as #3379 #3381

Open

Stripe/Trans amount parser silently truncates on locale-mismatched separators #3379

Open

franky-the-going-merry Bot requested changes Sep 19, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Sep 19, 2026

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 19, 2026

vivi-the-going-merry Bot added the franky-review label Sep 19, 2026

vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Sep 19, 2026

franky-the-going-merry Bot approved these changes Sep 19, 2026

View reviewed changes

franky-the-going-merry Bot removed the franky-review label Sep 19, 2026

Crabcyborg added the run e2e tests Run the Cypress end-to-end suite on this PR label Sep 22, 2026

Crabcyborg requested a review from garretlaxton September 22, 2026 14:29

Crabcyborg added the run analysis label Sep 22, 2026

vivi-the-going-merry Bot mentioned this pull request Sep 23, 2026

Fix FrmCurrencyHelper::prepare_price() locale-mismatched-separator truncation #3456

Open

garretlaxton approved these changes Sep 25, 2026

View reviewed changes

Navigation Menu

Repository navigation

Fix Stripe/Trans amount parser truncating locale-mismatched separators - #3380

Fix Stripe/Trans amount parser truncating locale-mismatched separators#3380
vivi-the-going-merry[bot] wants to merge 3 commits into
masterfrom
fix/issue-3379-locale-mismatched-amount-separators

vivi-the-going-merry Bot commented Sep 18, 2026

Uh oh!

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

Review skipped

Uh oh!

deepsource-io Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

franky-the-going-merry Bot left a comment

Uh oh!

franky-the-going-merry Bot Sep 19, 2026

Uh oh!

vivi-the-going-merry Bot commented Sep 19, 2026

Uh oh!

franky-the-going-merry Bot left a comment

Uh oh!

garretlaxton left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-		$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;
+		}

Conversation

vivi-the-going-merry Bot commented Sep 18, 2026

What was broken

What changed

How it was verified

Known follow-up (out of scope for this PR)

Uh oh!

coderabbitai Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

deepsource-io Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot commented Sep 19, 2026

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

garretlaxton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

deepsource-io Bot commented Sep 18, 2026 •

edited

Loading