Tests Involving Decimal and Currency Separators

This was suggested by the investigation of issue PHPOffice#3811. No fix is necessary for the issue. However, two possible code solutions (Php setlocale, which comes with certain design flaws, and StringHelper set(Decimal/Thousands)Separator were suggested, and neither is adequately tested. This PR adds such tests.

Unusually, getting StringHelper Decimal Separator, Thousands Separator, and Currency Code can result in a change to those properties. So, the existing design in several tests where those properties are captured in Setup and restored in Teardown do not work quite as designed. Instead, the ability to set those properties to their default value (null) is added, and the tests re-done to restore the default in Teardown.

The two methods yield the same results when parsing input. However, they diverge when examining output fields through `getFormattedValue`. Such output is currently correct (usually) when using setlocale, but not when using StringHelper. The former works through the 'trick' of using `sprintf(%f)`, which generates a locale-aware string. However, using non-locale-aware `sprintf(%F)` followed by `str_replace` will produce the correct result for both setlocale and StringHelper. One place in the code uses a cast to string, which is incorrect for both methods. Following that up with the same str_replace makes it correct for both. These changes permit, but do not require, the user to avoid setlocale altogether.

It remains an open question whether Settings/Calculation::setLocale should set DecimalSeparator, CurrencySeparator, and CurrencyCode. That makes logical sense, but it would be a breaking change, and having to explicitly set those values when using setLocale does not seem especially burdensome. For now, such a change will not be made.

Author: oleibman

src/PhpSpreadsheet/Shared/StringHelper.php

@@ -35,7 +35,7 @@ class StringHelper
     /**
      * Currency code.
      *
-     * @var string
+     * @var ?string
      */
     private static $currencyCode;
 
@@ -551,9 +551,9 @@ public static function getDecimalSeparator(): string
      * Set the decimal separator. Only used by NumberFormat::toFormattedString()
      * to format output by \PhpOffice\PhpSpreadsheet\Writer\Html and \PhpOffice\PhpSpreadsheet\Writer\Pdf.
      *
-     * @param string $separator Character for decimal separator
+     * @param ?string $separator Character for decimal separator
      */
-    public static function setDecimalSeparator(string $separator): void
+    public static function setDecimalSeparator(?string $separator): void
     {
         self::$decimalSeparator = $separator;
     }
@@ -582,9 +582,9 @@ public static function getThousandsSeparator(): string
      * Set the thousands separator. Only used by NumberFormat::toFormattedString()
      * to format output by \PhpOffice\PhpSpreadsheet\Writer\Html and \PhpOffice\PhpSpreadsheet\Writer\Pdf.
      *
-     * @param string $separator Character for thousands separator
+     * @param ?string $separator Character for thousands separator
      */
-    public static function setThousandsSeparator(string $separator): void
+    public static function setThousandsSeparator(?string $separator): void
     {
         self::$thousandsSeparator = $separator;
     }
@@ -618,9 +618,9 @@ public static function getCurrencyCode(): string
      * Set the currency code. Only used by NumberFormat::toFormattedString()
      *        to format output by \PhpOffice\PhpSpreadsheet\Writer\Html and \PhpOffice\PhpSpreadsheet\Writer\Pdf.
      *
-     * @param string $currencyCode Character for currency code
+     * @param ?string $currencyCode Character for currency code
      */
-    public static function setCurrencyCode(string $currencyCode): void
+    public static function setCurrencyCode(?string $currencyCode): void
     {
         self::$currencyCode = $currencyCode;
     }

src/PhpSpreadsheet/Style/NumberFormat/BaseFormatter.php

@@ -2,11 +2,24 @@
 
 namespace PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
+use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
+
 abstract class BaseFormatter
 {
     protected static function stripQuotes(string $format): string
     {
         // Some non-number strings are quoted, so we'll get rid of the quotes, likewise any positional * symbols
         return str_replace(['"', '*'], '', $format);
     }
+
+    protected static function adjustSeparators(string $value): string
+    {
+        $thousandsSeparator = StringHelper::getThousandsSeparator();
+        $decimalSeparator = StringHelper::getDecimalSeparator();
+        if ($thousandsSeparator !== ',' || $decimalSeparator !== '.') {
+            $value = str_replace(['.', ',', "\u{fffd}"], ["\u{fffd}", '.', ','], $value);
+        }
+
+        return $value;
+    }
 }

src/PhpSpreadsheet/Style/NumberFormat/Formatter.php

@@ -8,7 +8,7 @@
 use PhpOffice\PhpSpreadsheet\Style\Color;
 use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
-class Formatter
+class Formatter extends BaseFormatter
 {
     /**
      * Matches any @ symbol that isn't enclosed in quotes.
@@ -133,7 +133,7 @@ public static function toFormattedString($value, $format, $callBack = null)
         // For 'General' format code, we just pass the value although this is not entirely the way Excel does it,
         // it seems to round numbers to a total of 10 digits.
         if (($format === NumberFormat::FORMAT_GENERAL) || ($format === NumberFormat::FORMAT_TEXT)) {
-            return (string) $value;
+            return self::adjustSeparators((string) $value);
         }
 
         // Ignore square-$-brackets prefix in format string, like "[$-411]ge.m.d", "[$-010419]0%", etc

src/PhpSpreadsheet/Style/NumberFormat/NumberFormatter.php

@@ -5,7 +5,7 @@
 use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
 use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
-class NumberFormatter
+class NumberFormatter extends BaseFormatter
 {
     private const NUMBER_REGEX = '/(0+)(\\.?)(0*)/';
 
@@ -176,11 +176,11 @@ private static function formatStraightNumericValue(mixed $value, string $format,
             return $result;
         }
 
-        $sprintf_pattern = "%0$minWidth." . strlen($right) . 'f';
+        $sprintf_pattern = "%0$minWidth." . strlen($right) . 'F';
 
         /** @var float */
         $valueFloat = $value;
-        $value = sprintf($sprintf_pattern, round($valueFloat, strlen($right)));
+        $value = self::adjustSeparators(sprintf($sprintf_pattern, round($valueFloat, strlen($right))));
 
         return self::pregReplace(self::NUMBER_REGEX, $value, $format);
     }

src/PhpSpreadsheet/Style/NumberFormat/PercentageFormatter.php

@@ -38,11 +38,11 @@ public static function format($value, string $format): string
 
         $wholePartSize += $decimalPartSize + (int) ($decimalPartSize > 0);
         $replacement = "0{$wholePartSize}.{$decimalPartSize}";
-        $mask = (string) preg_replace('/[#0,]+\.?[?#0,]*/ui', "%{$replacement}f{$placeHolders}", $format);
+        $mask = (string) preg_replace('/[#0,]+\.?[?#0,]*/ui', "%{$replacement}F{$placeHolders}", $format);
 
         /** @var float */
         $valueFloat = $value;
 
-        return sprintf($mask, round($valueFloat, $decimalPartSize));
+        return self::adjustSeparators(sprintf($mask, round($valueFloat, $decimalPartSize)));
     }
 }

tests/PhpSpreadsheetTests/Calculation/Engine/FormattedNumberSlashTest.php

@@ -10,24 +10,11 @@
 
 class FormattedNumberSlashTest extends TestCase
 {
-    private string $originalCurrencyCode;
-
-    private string $originalDecimalSeparator;
-
-    private string $originalThousandsSeparator;
-
-    protected function setUp(): void
-    {
-        $this->originalCurrencyCode = StringHelper::getCurrencyCode();
-        $this->originalDecimalSeparator = StringHelper::getDecimalSeparator();
-        $this->originalThousandsSeparator = StringHelper::getThousandsSeparator();
-    }
-
     protected function tearDown(): void
     {
-        StringHelper::setCurrencyCode($this->originalCurrencyCode);
-        StringHelper::setDecimalSeparator($this->originalDecimalSeparator);
-        StringHelper::setThousandsSeparator($this->originalThousandsSeparator);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     /**

tests/PhpSpreadsheetTests/Calculation/Functions/TextData/ValueTest.php

@@ -9,26 +9,12 @@
 
 class ValueTest extends AllSetupTeardown
 {
-    private string $currencyCode;
-
-    private string $decimalSeparator;
-
-    private string $thousandsSeparator;
-
-    protected function setUp(): void
-    {
-        parent::setUp();
-        $this->currencyCode = StringHelper::getCurrencyCode();
-        $this->decimalSeparator = StringHelper::getDecimalSeparator();
-        $this->thousandsSeparator = StringHelper::getThousandsSeparator();
-    }
-
     protected function tearDown(): void
     {
         parent::tearDown();
-        StringHelper::setCurrencyCode($this->currencyCode);
-        StringHelper::setDecimalSeparator($this->decimalSeparator);
-        StringHelper::setThousandsSeparator($this->thousandsSeparator);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     /**

tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php

@@ -18,30 +18,21 @@ class AdvancedValueBinderTest extends TestCase
 
     private string $originalLocale;
 
-    private string $originalCurrencyCode;
-
-    private string $originalDecimalSeparator;
-
-    private string $originalThousandsSeparator;
-
     private IValueBinder $valueBinder;
 
     protected function setUp(): void
     {
         $this->originalLocale = Settings::getLocale();
-        $this->originalCurrencyCode = StringHelper::getCurrencyCode();
-        $this->originalDecimalSeparator = StringHelper::getDecimalSeparator();
-        $this->originalThousandsSeparator = StringHelper::getThousandsSeparator();
 
         $this->valueBinder = Cell::getValueBinder();
         Cell::setValueBinder(new AdvancedValueBinder());
     }
 
     protected function tearDown(): void
     {
-        StringHelper::setCurrencyCode($this->originalCurrencyCode);
-        StringHelper::setDecimalSeparator($this->originalDecimalSeparator);
-        StringHelper::setThousandsSeparator($this->originalThousandsSeparator);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
         Settings::setLocale($this->originalLocale);
         Cell::setValueBinder($this->valueBinder);
     }

tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php

@@ -31,7 +31,7 @@ protected function setUp(): void
     {
         $this->currentLocale = setlocale(LC_ALL, '0');
 
-        if (!setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu')) {
+        if (!setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu.utf8')) {
             $this->localeAdjusted = false;
 
             return;
@@ -52,6 +52,8 @@ protected function tearDown(): void
 
     /**
      * @dataProvider providerNumberFormatNoConversionTest
+     *
+     * @runInSeparateProcess
      */
     public function testNumberFormatNoConversion(mixed $expectedValue, string $expectedFormat, string $cellAddress): void
     {

tests/PhpSpreadsheetTests/Shared/StringHelperTest.php

@@ -9,25 +9,11 @@
 
 class StringHelperTest extends TestCase
 {
-    private string $currencyCode;
-
-    private string $decimalSeparator;
-
-    private string $thousandsSeparator;
-
-    protected function setUp(): void
-    {
-        parent::setUp();
-        $this->currencyCode = StringHelper::getCurrencyCode();
-        $this->decimalSeparator = StringHelper::getDecimalSeparator();
-        $this->thousandsSeparator = StringHelper::getThousandsSeparator();
-    }
-
     protected function tearDown(): void
     {
-        StringHelper::setCurrencyCode($this->currencyCode);
-        StringHelper::setDecimalSeparator($this->decimalSeparator);
-        StringHelper::setThousandsSeparator($this->thousandsSeparator);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     public function testGetIsIconvEnabled(): void

tests/PhpSpreadsheetTests/Style/NumberFormatTest.php

@@ -11,26 +11,17 @@
 
 class NumberFormatTest extends TestCase
 {
-    private string $currencyCode;
-
-    private string $decimalSeparator;
-
-    private string $thousandsSeparator;
-
     protected function setUp(): void
     {
-        $this->currencyCode = StringHelper::getCurrencyCode();
-        $this->decimalSeparator = StringHelper::getDecimalSeparator();
-        $this->thousandsSeparator = StringHelper::getThousandsSeparator();
         StringHelper::setDecimalSeparator('.');
         StringHelper::setThousandsSeparator(',');
     }
 
     protected function tearDown(): void
     {
-        StringHelper::setCurrencyCode($this->currencyCode);
-        StringHelper::setDecimalSeparator($this->decimalSeparator);
-        StringHelper::setThousandsSeparator($this->thousandsSeparator);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     /**

tests/PhpSpreadsheetTests/Writer/Html/HtmlNumberFormatTest.php

@@ -13,27 +13,18 @@
 
 class HtmlNumberFormatTest extends Functional\AbstractFunctional
 {
-    private string $currency;
-
-    private string $decsep;
-
-    private string $thosep;
-
     protected function setUp(): void
     {
-        $this->currency = StringHelper::getCurrencyCode();
         StringHelper::setCurrencyCode('$');
-        $this->decsep = StringHelper::getDecimalSeparator();
         StringHelper::setDecimalSeparator('.');
-        $this->thosep = StringHelper::getThousandsSeparator();
         StringHelper::setThousandsSeparator(',');
     }
 
     protected function tearDown(): void
     {
-        StringHelper::setCurrencyCode($this->currency);
-        StringHelper::setDecimalSeparator($this->decsep);
-        StringHelper::setThousandsSeparator($this->thosep);
+        StringHelper::setCurrencyCode(null);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     public function testColorNumberFormat(): void