diff --git a/stripe/controllers/FrmTransLiteActionsController.php b/stripe/controllers/FrmTransLiteActionsController.php index ad2d4355ee..bd03ada260 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,35 +363,71 @@ private static function get_amount_from_string( $amount ) { * * @return void */ - private static function maybe_use_decimal( &$amount, $currency ) { - 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'], '.', '' ); } /** + * 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. + * * @param string $amount * @param array $currency * - * @return void + * @return int|false */ - private static function normalize_number( &$amount, $currency ) { - $amount = str_replace( $currency['thousand_separator'], '', $amount ); - $amount = str_replace( $currency['decimal_separator'], '.', $amount ); - $amount = number_format( (float) $amount, $currency['decimals'], '.', '' ); + 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 ); + } + + if ( false !== $last_dot ) { + $present = '.'; + $position = $last_dot; + } elseif ( false !== $last_comma ) { + $present = ','; + $position = $last_comma; + } else { + return false; + } + + 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 e05f68294e..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' ); @@ -145,6 +145,20 @@ 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 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 48c8a97244..4dac5c301a 100644 --- a/tests/phpunit/stripe/test_FrmTransLiteActionsController.php +++ b/tests/phpunit/stripe/test_FrmTransLiteActionsController.php @@ -43,39 +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 ) ); + + // 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 ) ); } }