Skip to content

Commit

Permalink
Rounding in NumberFormatter (#2399)
Browse files Browse the repository at this point in the history
Fix #2385. NumberFormatter is using sprintf on a float, and is seeing inconsistent rounding as a result (it will also occasionally result in `-0`). Change to round the number before presenting it to sprintf.
  • Loading branch information
oleibman authored Nov 26, 2021
1 parent a561385 commit 3257ae5
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 40 deletions.
30 changes: 0 additions & 30 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -5995,36 +5995,6 @@ parameters:
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/FractionFormatter.php

-
message: "#^Cannot cast mixed to int\\.$#"
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Cannot cast mixed to string\\.$#"
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Parameter \\#1 \\$number of function abs expects float\\|int\\|string, mixed given\\.$#"
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Parameter \\#1 \\$number of function number_format expects float, mixed given\\.$#"
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Parameter \\#1 \\$x of function fmod expects float, mixed given\\.$#"
count: 1
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Parameter \\#2 \\.\\.\\.\\$values of function sprintf expects bool\\|float\\|int\\|string\\|null, mixed given\\.$#"
count: 2
path: src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\PercentageFormatter\\:\\:format\\(\\) has parameter \\$value with no type specified\\.$#"
count: 1
Expand Down
29 changes: 19 additions & 10 deletions src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ private static function mergeComplexNumberFormatMasks(array $numbers, array $mas
*/
private static function processComplexNumberFormatMask($number, string $mask): string
{
/** @var string */
$result = $number;
$maskingBlockCount = preg_match_all('/0+/', $mask, $maskingBlocks, PREG_OFFSET_CAPTURE);

Expand All @@ -45,8 +46,10 @@ private static function processComplexNumberFormatMask($number, string $mask): s
$divisor = 10 ** $size;
$offset = $block[1];

$blockValue = sprintf("%0{$size}d", fmod($number, $divisor));
$number = floor($number / $divisor);
/** @var float */
$numberFloat = $number;
$blockValue = sprintf("%0{$size}d", fmod($numberFloat, $divisor));
$number = floor($numberFloat / $divisor);
$mask = substr_replace($mask, $blockValue, $offset, $size);
}
if ($number > 0) {
Expand All @@ -64,7 +67,9 @@ private static function processComplexNumberFormatMask($number, string $mask): s
private static function complexNumberFormatMask($number, string $mask, bool $splitOnPoint = true): string
{
$sign = ($number < 0.0) ? '-' : '';
$number = (string) abs($number);
/** @var float */
$numberFloat = $number;
$number = (string) abs($numberFloat);

if ($splitOnPoint && strpos($mask, '.') !== false && strpos($number, '.') !== false) {
$numbers = explode('.', $number);
Expand All @@ -88,6 +93,8 @@ private static function complexNumberFormatMask($number, string $mask, bool $spl
*/
private static function formatStraightNumericValue($value, string $format, array $matches, bool $useThousands): string
{
/** @var float */
$valueFloat = $value;
$left = $matches[1];
$dec = $matches[2];
$right = $matches[3];
Expand All @@ -96,7 +103,7 @@ private static function formatStraightNumericValue($value, string $format, array
$minWidth = strlen($left) + strlen($dec) + strlen($right);
if ($useThousands) {
$value = number_format(
$value,
$valueFloat,
strlen($right),
StringHelper::getDecimalSeparator(),
StringHelper::getThousandsSeparator()
Expand All @@ -107,17 +114,19 @@ private static function formatStraightNumericValue($value, string $format, array

if (preg_match('/[0#]E[+-]0/i', $format)) {
// Scientific format
return sprintf('%5.2E', $value);
return sprintf('%5.2E', $valueFloat);
} elseif (preg_match('/0([^\d\.]+)0/', $format) || substr_count($format, '.') > 1) {
if ($value == (int) $value && substr_count($format, '.') === 1) {
if ($value == (int) $valueFloat && substr_count($format, '.') === 1) {
$value *= 10 ** strlen(explode('.', $format)[1]);
}

return self::complexNumberFormatMask($value, $format);
}

$sprintf_pattern = "%0$minWidth." . strlen($right) . 'f';
$value = sprintf($sprintf_pattern, $value);
/** @var float */
$valueFloat = $value;
$value = sprintf($sprintf_pattern, round($valueFloat, strlen($right)));

return self::pregReplace(self::NUMBER_REGEX, $value, $format);
}
Expand Down Expand Up @@ -196,15 +205,15 @@ public static function format($value, string $format): string
}

/**
* @param mixed $value
* @param array|string $value
*/
private static function makeString($value): string
{
return is_array($value) ? '' : (string) $value;
return is_array($value) ? '' : "$value";
}

private static function pregReplace(string $pattern, string $replacement, string $subject): string
{
return self::makeString(preg_replace($pattern, $replacement, $subject));
return self::makeString(preg_replace($pattern, $replacement, $subject) ?? '');
}
}
32 changes: 32 additions & 0 deletions tests/PhpSpreadsheetTests/Style/NumberFormatRoundTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace PhpOffice\PhpSpreadsheetTests\Style;

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PHPUnit\Framework\TestCase;

class NumberFormatRoundTest extends TestCase
{
public static function testRound(): void
{
// Inconsistent rounding due to letting sprintf do it rather than round.
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->getStyle('A1:H2')->getNumberFormat()->setFormatCode('0');
$sheet->getStyle('A3:H3')->getNumberFormat()->setFormatCode('0.0');
$sheet->fromArray(
[
[-3.5, -2.5, -1.5, -0.5, 0.5, 1.5, 2.5, 3.5],
[-3.1, -2.9, -1.4, -0.3, 0.7, 1.6, 2.4, 3.7],
[-3.15, -2.85, -1.43, -0.87, 0.72, 1.60, 2.45, 3.75],
]
);
$expected = [
[-4, -3, -2, -1, 1, 2, 3, 4],
[-3, -3, -1, 0, 1, 2, 2, 4],
[-3.2, -2.9, -1.4, -0.9, 0.7, 1.6, 2.5, 3.8],
];
self::assertEquals($expected, $sheet->toArray());
$spreadsheet->disconnectWorksheets();
}
}

0 comments on commit 3257ae5

Please sign in to comment.