From 3c7857d71dff6606f27f56747843da9674369199 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:47:16 -0600 Subject: [PATCH 1/3] Fix Stripe/Trans amount parser truncating locale-mismatched separators 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 --- .../FrmTransLiteActionsController.php | 35 +++++++++++++++++-- .../test_FrmSquareLiteAppController.php | 8 +++++ .../test_FrmTransLiteActionsController.php | 8 +++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/stripe/controllers/FrmTransLiteActionsController.php b/stripe/controllers/FrmTransLiteActionsController.php index ad2d4355ee..049f2ffe9c 100755 --- a/stripe/controllers/FrmTransLiteActionsController.php +++ b/stripe/controllers/FrmTransLiteActionsController.php @@ -365,6 +365,13 @@ private static function get_amount_from_string( $amount ) { * @return void */ private static function maybe_use_decimal( &$amount, $currency ) { + // When both '.' and ',' are present, normalize_number() determines the real decimal + // separator from the string itself instead of trusting the currency's configured + // separator -- doing the swap here first would collide it with the other separator. + if ( self::has_conflicting_separators( $amount ) ) { + return; + } + if ( $currency['thousand_separator'] !== '.' ) { return; } @@ -383,6 +390,15 @@ private static function maybe_use_decimal( &$amount, $currency ) { } } + /** + * @param string $amount + * + * @return bool + */ + private static function has_conflicting_separators( $amount ) { + return strpos( $amount, '.' ) !== false && strpos( $amount, ',' ) !== false; + } + /** * @param string $amount * @param array $currency @@ -390,8 +406,23 @@ private static function maybe_use_decimal( &$amount, $currency ) { * @return void */ private static function normalize_number( &$amount, $currency ) { - $amount = str_replace( $currency['thousand_separator'], '', $amount ); - $amount = str_replace( $currency['decimal_separator'], '.', $amount ); + if ( self::has_conflicting_separators( $amount ) ) { + // A user can type an amount in a different locale's format than the form's + // configured currency expects (e.g. US-style "1,030.21" on a form whose currency + // configures '.' as the thousand separator). Trusting the currency's separators + // blindly in that case treats the amount's real decimal point as the thousand + // separator and vice versa, colliding both into one and truncating the value by + // orders of magnitude. Whichever separator appears last in the string is + // unambiguously the real decimal point. + $decimal_separator = strrpos( $amount, '.' ) > strrpos( $amount, ',' ) ? '.' : ','; + $thousand_separator = '.' === $decimal_separator ? ',' : '.'; + } else { + $decimal_separator = $currency['decimal_separator']; + $thousand_separator = $currency['thousand_separator']; + } + + $amount = str_replace( $thousand_separator, '', $amount ); + $amount = str_replace( $decimal_separator, '.', $amount ); $amount = number_format( (float) $amount, $currency['decimals'], '.', '' ); } diff --git a/tests/phpunit/square/test_FrmSquareLiteAppController.php b/tests/phpunit/square/test_FrmSquareLiteAppController.php index e05f68294e..0abe9d5422 100644 --- a/tests/phpunit/square/test_FrmSquareLiteAppController.php +++ b/tests/phpunit/square/test_FrmSquareLiteAppController.php @@ -145,6 +145,14 @@ public function amount_format_provider(): \Iterator { // a shopper typing a European style amount into a GBP form is read as 123 pounds. yield 'GBP comma is never a decimal' => array( 'gbp', '1,23', '123.00', '12300' ); + // A shopper can type in a different locale's format than the form's configured + // currency expects. Both separators appearing together is unambiguous regardless of + // currency -- whichever one appears last is the real decimal point. Trusting the + // currency's configured separators here used to collide the two into one, silently + // truncating "1,030.21" to 1.03 (formidable-forms#3379). + yield 'EUR form with a US-style amount' => array( 'eur', '€1,030.21', '1030.21', '103021' ); + yield 'GBP form with a EU-style amount' => array( 'gbp', '£1.030,21', '1030.21', '103021' ); + // A currency with no fractional unit keeps the two paths identical, and rounds. yield 'JPY with a thousands comma' => array( 'jpy', '1,234', '1234', '1234' ); yield 'JPY rounds away a decimal' => array( 'jpy', '1234.56', '1235', '1235' ); diff --git a/tests/phpunit/stripe/test_FrmTransLiteActionsController.php b/tests/phpunit/stripe/test_FrmTransLiteActionsController.php index 48c8a97244..9a6caa3a1c 100644 --- a/tests/phpunit/stripe/test_FrmTransLiteActionsController.php +++ b/tests/phpunit/stripe/test_FrmTransLiteActionsController.php @@ -67,6 +67,14 @@ public function test_maybe_use_decimal() { $amount = '111.500'; $this->maybe_use_decimal( $amount, $currency ); $this->assertSame( '111.500', $amount ); + + // A string already carrying both separators is a different locale's format (e.g. a + // US-style "1,111.50" typed into a form with this dot-thousands currency), not a + // misplaced decimal point -- leave it alone for normalize_number to resolve instead + // of colliding it with the currency's own decimal_separator. + $amount = '1,111.50'; + $this->maybe_use_decimal( $amount, $currency ); + $this->assertSame( '1,111.50', $amount ); } /** From 718fdda8a9793a725021ca422e91d1b1e0783f07 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:55:38 -0600 Subject: [PATCH 2/3] Consolidate separator detection and close a multi-separator gap 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. --- .../FrmTransLiteActionsController.php | 97 ++++++++++--------- .../test_FrmSquareLiteAppController.php | 8 +- .../test_FrmTransLiteActionsController.php | 58 ++++++----- 3 files changed, 88 insertions(+), 75 deletions(-) diff --git a/stripe/controllers/FrmTransLiteActionsController.php b/stripe/controllers/FrmTransLiteActionsController.php index 049f2ffe9c..21e0889a4a 100755 --- a/stripe/controllers/FrmTransLiteActionsController.php +++ b/stripe/controllers/FrmTransLiteActionsController.php @@ -317,7 +317,6 @@ public static function prepare_amount( $amount, $atts = array() ) { foreach ( (array) $amount as $a ) { $this_amount = self::get_amount_from_string( $a ); - self::maybe_use_decimal( $this_amount, $currency ); self::normalize_number( $this_amount, $currency ); $total += $this_amount; @@ -364,66 +363,68 @@ private static function get_amount_from_string( $amount ) { * * @return void */ - private static function maybe_use_decimal( &$amount, $currency ) { - // When both '.' and ',' are present, normalize_number() determines the real decimal - // separator from the string itself instead of trusting the currency's configured - // separator -- doing the swap here first would collide it with the other separator. - if ( self::has_conflicting_separators( $amount ) ) { - return; - } - - if ( $currency['thousand_separator'] !== '.' ) { - return; - } - - $amount_parts = explode( '.', $amount ); + private static function normalize_number( &$amount, $currency ) { + $decimal_position = self::find_decimal_position( $amount, $currency ); - if ( 2 !== count( $amount_parts ) ) { - return; + if ( false === $decimal_position ) { + $amount = str_replace( array( '.', ',' ), '', $amount ); + } else { + $integer_part = str_replace( array( '.', ',' ), '', substr( $amount, 0, $decimal_position ) ); + $fractional_part = str_replace( array( '.', ',' ), '', substr( $amount, $decimal_position + 1 ) ); + $amount = $integer_part . '.' . $fractional_part; } - $strlen = strlen( $amount_parts[1] ); - $used_for_decimal = $strlen === 1 || $strlen === 2; - - if ( $used_for_decimal ) { - $amount = str_replace( '.', $currency['decimal_separator'], $amount ); - } + $amount = number_format( (float) $amount, $currency['decimals'], '.', '' ); } /** - * @param string $amount + * Find the position of the amount's real decimal separator, or false if it has none (a + * whole-number amount, possibly with thousands grouping). + * + * A user can type an amount in a different locale's format than the form's configured + * currency expects (e.g. US-style "1,030.21" on a form whose currency configures '.' as + * the thousand separator). Trusting the currency's configured separator in that case -- + * and blindly replacing every occurrence of it -- is what let this silently truncate: + * when both '.' and ',' appear, or the same character repeats, only the rightmost + * occurrence is ever the real decimal point; everything else gets stripped as grouping + * noise by the caller. * - * @return bool - */ - private static function has_conflicting_separators( $amount ) { - return strpos( $amount, '.' ) !== false && strpos( $amount, ',' ) !== false; - } - - /** * @param string $amount * @param array $currency * - * @return void + * @return int|false */ - private static function normalize_number( &$amount, $currency ) { - if ( self::has_conflicting_separators( $amount ) ) { - // A user can type an amount in a different locale's format than the form's - // configured currency expects (e.g. US-style "1,030.21" on a form whose currency - // configures '.' as the thousand separator). Trusting the currency's separators - // blindly in that case treats the amount's real decimal point as the thousand - // separator and vice versa, colliding both into one and truncating the value by - // orders of magnitude. Whichever separator appears last in the string is - // unambiguously the real decimal point. - $decimal_separator = strrpos( $amount, '.' ) > strrpos( $amount, ',' ) ? '.' : ','; - $thousand_separator = '.' === $decimal_separator ? ',' : '.'; - } else { - $decimal_separator = $currency['decimal_separator']; - $thousand_separator = $currency['thousand_separator']; + private static function find_decimal_position( $amount, $currency ) { + $last_dot = strrpos( $amount, '.' ); + $last_comma = strrpos( $amount, ',' ); + + if ( false !== $last_dot && false !== $last_comma ) { + return max( $last_dot, $last_comma ); } - $amount = str_replace( $thousand_separator, '', $amount ); - $amount = str_replace( $decimal_separator, '.', $amount ); - $amount = number_format( (float) $amount, $currency['decimals'], '.', '' ); + if ( false === $last_dot && false === $last_comma ) { + return false; + } + + $present = false !== $last_dot ? '.' : ','; + $position = false !== $last_dot ? $last_dot : $last_comma; + + if ( $present === $currency['decimal_separator'] ) { + return $position; + } + + // The lone separator matches the currency's thousand separator instead. A dot in a + // comma-decimal currency is ambiguous -- a single occurrence with a 1-2 digit tail + // still reads as a decimal point even though the currency expects '.' as its thousand + // separator. A comma in a dot-decimal currency is never ambiguous this way: it's + // always thousands grouping (e.g. "1,23" on a GBP form is 123, not 1.23). + if ( '.' !== $present || 1 !== substr_count( $amount, $present ) ) { + return false; + } + + $tail_length = strlen( $amount ) - $position - 1; + + return in_array( $tail_length, array( 1, 2 ), true ) ? $position : false; } /** diff --git a/tests/phpunit/square/test_FrmSquareLiteAppController.php b/tests/phpunit/square/test_FrmSquareLiteAppController.php index 0abe9d5422..295bd6ba47 100644 --- a/tests/phpunit/square/test_FrmSquareLiteAppController.php +++ b/tests/phpunit/square/test_FrmSquareLiteAppController.php @@ -135,7 +135,7 @@ public function amount_format_provider(): \Iterator { yield 'BRL with a thousands dot' => array( 'brl', 'R$1.234,50', '1234.50', '123450' ); // A dot in a comma decimal currency is ambiguous. One or two trailing digits are - // read as a decimal, three are read as thousands. See maybe_use_decimal. + // read as a decimal, three are read as thousands. See find_decimal_position. yield 'EUR dot with two digits is a decimal' => array( 'eur', '€20.00', '20.00', '2000' ); yield 'EUR dot with one digit is a decimal' => array( 'eur', '€1.5', '1.50', '150' ); yield 'EUR dot with three digits is thousands' => array( 'eur', '€1.234', '1234.00', '123400' ); @@ -153,6 +153,12 @@ public function amount_format_provider(): \Iterator { yield 'EUR form with a US-style amount' => array( 'eur', '€1,030.21', '1030.21', '103021' ); yield 'GBP form with a EU-style amount' => array( 'gbp', '£1.030,21', '1030.21', '103021' ); + // A repeated occurrence of whichever character turns out to be the decimal separator + // used to get blanket-replaced entirely, colliding into a second decimal point and + // truncating the value again just like the original bug -- only the rightmost + // occurrence is ever the real decimal point; earlier ones are grouping noise. + yield 'repeated decimal-separator character is still just noise' => array( 'eur', '1,234.567,89', '1234567.89', '123456789' ); + // A currency with no fractional unit keeps the two paths identical, and rounds. yield 'JPY with a thousands comma' => array( 'jpy', '1,234', '1234', '1234' ); yield 'JPY rounds away a decimal' => array( 'jpy', '1234.56', '1235', '1235' ); diff --git a/tests/phpunit/stripe/test_FrmTransLiteActionsController.php b/tests/phpunit/stripe/test_FrmTransLiteActionsController.php index 9a6caa3a1c..4dac5c301a 100644 --- a/tests/phpunit/stripe/test_FrmTransLiteActionsController.php +++ b/tests/phpunit/stripe/test_FrmTransLiteActionsController.php @@ -43,47 +43,53 @@ private function get_fields_for_price( $action ) { } /** - * @covers FrmTransLiteActionsController::maybe_use_decimal + * @covers FrmTransLiteActionsController::find_decimal_position */ - public function test_maybe_use_decimal() { - // We need a currency with a . thousands separator. + public function test_find_decimal_position() { + // A dot-thousands currency (e.g. EUR). $currency = array( 'thousand_separator' => '.', 'decimal_separator' => ',', ); - // Test with two decimal places. - $amount = '111.50'; - $this->maybe_use_decimal( $amount, $currency ); - $this->assertSame( '111,50', $amount ); + // A single dot with a 1-2 digit tail reads as a decimal point even though this + // currency configures '.' as its thousand separator. + $this->assertSame( 3, $this->find_decimal_position( '111.50', $currency ) ); + $this->assertSame( 3, $this->find_decimal_position( '111.5', $currency ) ); - // Test with a single decimal place. - $amount = '111.5'; - $this->maybe_use_decimal( $amount, $currency ); - $this->assertSame( '111,5', $amount ); + // Three digits after a single dot reads as thousands grouping instead. + $this->assertFalse( $this->find_decimal_position( '111.500', $currency ) ); - // Test to make sure that three decimal places does not convert. - // It should be interpreted as thousands. - $amount = '111.500'; - $this->maybe_use_decimal( $amount, $currency ); - $this->assertSame( '111.500', $amount ); + // Repeated dots and no comma at all is unambiguous thousands grouping. + $this->assertFalse( $this->find_decimal_position( '1.234.567', $currency ) ); - // A string already carrying both separators is a different locale's format (e.g. a - // US-style "1,111.50" typed into a form with this dot-thousands currency), not a - // misplaced decimal point -- leave it alone for normalize_number to resolve instead - // of colliding it with the currency's own decimal_separator. - $amount = '1,111.50'; - $this->maybe_use_decimal( $amount, $currency ); - $this->assertSame( '1,111.50', $amount ); + // Both separators present is a different locale's format (e.g. a US-style + // "1,111.50" typed into a form with this dot-thousands currency) -- whichever + // separator appears last is the real decimal point, regardless of currency config. + $this->assertSame( 5, $this->find_decimal_position( '1,111.50', $currency ) ); + $this->assertSame( 5, $this->find_decimal_position( '1.111,50', $currency ) ); + + // A repeated occurrence of the character that turns out to be the decimal separator + // doesn't move the split point off the rightmost occurrence (formidable-forms#3379: + // blindly replacing every occurrence of it is what caused the original truncation). + $this->assertSame( 9, $this->find_decimal_position( '1,234.567,89', $currency ) ); + + // A comma-thousands currency (e.g. GBP): a lone comma is always thousands grouping, + // never reinterpreted as a decimal point, regardless of its tail length. + $currency = array( + 'thousand_separator' => ',', + 'decimal_separator' => '.', + ); + $this->assertFalse( $this->find_decimal_position( '1,23', $currency ) ); } /** * @param string $amount * @param array $currency * - * @return string + * @return int|false */ - private function maybe_use_decimal( &$amount, $currency ) { - return $this->run_private_method( array( 'FrmTransLiteActionsController', 'maybe_use_decimal' ), array( &$amount, $currency ) ); + private function find_decimal_position( $amount, $currency ) { + return $this->run_private_method( array( 'FrmTransLiteActionsController', 'find_decimal_position' ), array( $amount, $currency ) ); } } From ea0736d0580941bfa71af00465b0714f62eed3c0 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 05:31:29 -0600 Subject: [PATCH 3/3] Fix PHPStan minus.rightNonNumeric on find_decimal_position() 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. --- stripe/controllers/FrmTransLiteActionsController.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/stripe/controllers/FrmTransLiteActionsController.php b/stripe/controllers/FrmTransLiteActionsController.php index 21e0889a4a..bd03ada260 100755 --- a/stripe/controllers/FrmTransLiteActionsController.php +++ b/stripe/controllers/FrmTransLiteActionsController.php @@ -402,13 +402,16 @@ private static function find_decimal_position( $amount, $currency ) { return max( $last_dot, $last_comma ); } - if ( false === $last_dot && false === $last_comma ) { + if ( false !== $last_dot ) { + $present = '.'; + $position = $last_dot; + } elseif ( false !== $last_comma ) { + $present = ','; + $position = $last_comma; + } else { return false; } - $present = false !== $last_dot ? '.' : ','; - $position = false !== $last_dot ? $last_dot : $last_comma; - if ( $present === $currency['decimal_separator'] ) { return $position; }