Skip to content

Commit

Permalink
Fix Store API response for very large price amounts (woocommerce#49361)
Browse files Browse the repository at this point in the history
* Fixed overflow when formatting price for Store API responses

* Added explanation comment.

* Added changelog.

* Linting.

* Ensure wc_format_decimal doesn't return decimal points and trims .00

* Update comment.

* Removed unnecessary rounding modes.

* Updated comment.

* Updated comment.

* Updated comment.

* Updated Unit Tests.

* Lint.

* Fix tests.

* Re-add rounding modes.

* Prevented a fatal if an array is supplied to the method. This was the old behaviour, although it will produce erroneous prices, but before we let this throw a fatal we need to warn devs and track usage.

* Added doing_it_wrong() for unexpected types for $value arg.

* Early return, removed translation, renamed unit test method.

* Added expect notice to unit test.

* Add further tests to rounding modes.

* Renamed $mock_formatter. This is not a mock.

* Fixed tests and added provider for types.

* Linting.
  • Loading branch information
wavvves authored Jul 15, 2024
1 parent 64ce7bd commit f3cafa2
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Fixed a bug that would cause incorrect pricing at checkout for very large amounts.
41 changes: 30 additions & 11 deletions plugins/woocommerce/src/StoreApi/Formatters/MoneyFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,24 @@
*/
class MoneyFormatter implements FormatterInterface {
/**
* Format a given value and return the result.
* Format a given price value and return the result as a string without decimals.
*
* @param mixed $value Value to format.
* @param array $options Options that influence the formatting.
* @return mixed
* @param int|float|string $value Value to format. Int is allowed, as it may also represent a valid price.
* @param array $options Options that influence the formatting.
* @return string
*/
public function format( $value, array $options = [] ) {

if ( ! is_int( $value ) && ! is_string( $value ) && ! is_float( $value ) ) {
wc_doing_it_wrong(
__FUNCTION__,
'Function expects a $value arg of type INT, STRING or FLOAT.',
'9.2'
);

return '';
}

$options = wp_parse_args(
$options,
[
Expand All @@ -23,12 +34,20 @@ public function format( $value, array $options = [] ) {
]
);

return (string) intval(
round(
( (float) wc_format_decimal( $value ) ) * ( 10 ** absint( $options['decimals'] ) ),
0,
absint( $options['rounding_mode'] )
)
);
// Ensure rounding mode is valid.
$rounding_modes = [ PHP_ROUND_HALF_UP, PHP_ROUND_HALF_DOWN, PHP_ROUND_HALF_EVEN, PHP_ROUND_HALF_ODD ];
$options['rounding_mode'] = absint( $options['rounding_mode'] );
if ( ! in_array( $options['rounding_mode'], $rounding_modes, true ) ) {
$options['rounding_mode'] = PHP_ROUND_HALF_UP;
}

$value = floatval( $value );

// Remove the price decimal points for rounding purposes.
$value = $value * pow( 10, absint( $options['decimals'] ) );
$value = round( $value, 0, $options['rounding_mode'] );

// This ensures returning the value as a string without decimal points ready for price parsing.
return wc_format_decimal( $value, 0, true );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,81 @@
*/
class TestMoneyFormatter extends \WP_UnitTestCase {

private $mock_formatter;
/**
* @var MoneyFormatter
*/
private $formatter;

/**
* Setup test product data. Called before every test.
*/
protected function setUp(): void {
parent::setUp();

$this->mock_formatter = new MoneyFormatter();
$this->formatter = new MoneyFormatter();
}

/**
* Test formatting.
*/
public function test_format() {
$this->assertEquals( "1000", $this->mock_formatter->format( 10 ) );
$this->assertEquals( "1000", $this->mock_formatter->format( "10" ) );
$this->assertEquals( '1000', $this->formatter->format( 10 ) );
$this->assertEquals( '1000', $this->formatter->format( '10' ) );
}

/**
* Test formatting with custom DP.
*/
public function test_format_dp() {
$this->assertEquals( "100000", $this->mock_formatter->format( 10, [ 'decimals' => 4 ] ) );
$this->assertEquals( "100000", $this->mock_formatter->format( "10", [ 'decimals' => 4 ] ) );
$this->assertEquals( '100000', $this->formatter->format( 10, [ 'decimals' => 4 ] ) );
$this->assertEquals( '100000', $this->formatter->format( '10', [ 'decimals' => 4 ] ) );
}

/**
* Test formatting with custom DP.
*/
public function test_format_rounding_mode() {
$this->assertEquals( "156", $this->mock_formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_UP ] ) );
$this->assertEquals( "155", $this->mock_formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_DOWN ] ) );
$this->assertEquals( '156', $this->formatter->format( 1.555 ) );
$this->assertEquals( '156', $this->formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_UP ] ) );
$this->assertEquals( '155', $this->formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_DOWN ] ) );
$this->assertEquals( '156', $this->formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_EVEN ] ) );
$this->assertEquals( '155', $this->formatter->format( 1.555, [ 'rounding_mode' => PHP_ROUND_HALF_ODD ] ) );
$this->assertEquals( '156', $this->formatter->format( 1.555, [ 'rounding_mode' => 123456 ] ) );
}

/**
* Test formatting for int overflow values.
*/
public function test_format_int_overflow() {
$this->assertEquals( '922337203685477580800', $this->formatter->format( '9223372036854775808' ) );
$this->assertEquals( '922337203685477580800', $this->formatter->format( floatval( '9223372036854775808' ) ) );
}

/**
* Data provider for invalid param types.
*/
public function invalidTypesProvider() {
return [
[ true ],
[ null ],
[ [ 'Not right' ] ],
[ new \StdClass() ],
];
}

/**
* Test formatting returns '' if a $value of type INT, STRING or FLOAT is not provided.
* @dataProvider invalidTypesProvider
*
* @param mixed $invalid_type The invalid type to test.
*/
public function test_format_unexpected_param_types( $invalid_type ) {
$this->expected_doing_it_wrong = array_merge(
$this->expected_doing_it_wrong,
[ 'format' ]
);

// Assert that the format method returns an empty string for invalid types.
$this->assertEquals( '', $this->formatter->format( $invalid_type ) );
}
}

0 comments on commit f3cafa2

Please sign in to comment.