Tests Involving Decimal and Currency Separators

This was suggested by the investigation of issue #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.

src/PhpSpreadsheet/Shared/StringHelper.php

@@ -35,7 +35,7 @@ class StringHelper
     /**
      * Currency code.
      *
-     * @var string
+     * @var ?string
      */
     private static $currencyCode;
 
@@ -551,9 +551,9 @@ class StringHelper
      * 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 @@ class StringHelper
      * 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 @@ class StringHelper
      * 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,6 +2,8 @@
 
 namespace PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
+use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
+
 abstract class BaseFormatter
 {
     protected static function stripQuotes(string $format): string
@@ -9,4 +11,15 @@ abstract class BaseFormatter
         // 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\RichText\RichText;
 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 @@ class Formatter
         // 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 @@ namespace PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 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 @@ class NumberFormatter
             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 @@ class PercentageFormatter extends BaseFormatter
 
         $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 @@ use PHPUnit\Framework\TestCase;
 
 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 @@ use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
 
 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,20 +18,11 @@ 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());
@@ -39,9 +30,9 @@ class AdvancedValueBinderTest extends TestCase
 
     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 @@ class CsvNumberFormatLocaleTest extends TestCase
     {
         $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 @@ class CsvNumberFormatLocaleTest extends TestCase
 
     /**
      * @dataProvider providerNumberFormatNoConversionTest
+     *
+     * @runInSeparateProcess
      */
     public function testNumberFormatNoConversion(mixed $expectedValue, string $expectedFormat, string $cellAddress): void
     {

tests/PhpSpreadsheetTests/Shared/StringHelperTest.php

@@ -9,25 +9,11 @@ use PHPUnit\Framework\TestCase;
 
 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 @@ use PHPUnit\Framework\TestCase;
 
 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 @@ use PhpOffice\PhpSpreadsheetTests\Functional;
 
 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

tests/PhpSpreadsheetTests/Writer/Xlsx/LocaleFloatsTest.php

@@ -4,17 +4,18 @@ declare(strict_types=1);
 
 namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
+use PhpOffice\PhpSpreadsheet\Reader\Xlsx as XlsxReader;
+use PhpOffice\PhpSpreadsheet\Settings;
+use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
 
 class LocaleFloatsTest extends AbstractFunctional
 {
-    private bool $localeAdjusted;
+    private false|string $currentPhpLocale;
 
-    /**
-     * @var false|string
-     */
-    private $currentLocale;
+    private string $originalLocale;
 
     /** @var ?Spreadsheet */
     private $spreadsheet;
@@ -24,21 +25,19 @@ class LocaleFloatsTest extends AbstractFunctional
 
     protected function setUp(): void
     {
-        $this->currentLocale = setlocale(LC_ALL, '0');
-
-        if (!setlocale(LC_ALL, 'fr_FR.UTF-8', 'fra_fra')) {
-            $this->localeAdjusted = false;
-
-            return;
-        }
-
-        $this->localeAdjusted = true;
+        $this->currentPhpLocale = setlocale(LC_ALL, '0');
+        $this->originalLocale = Settings::getLocale();
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
     }
 
     protected function tearDown(): void
     {
-        if ($this->localeAdjusted && is_string($this->currentLocale)) {
-            setlocale(LC_ALL, $this->currentLocale);
+        StringHelper::setDecimalSeparator(null);
+        StringHelper::setThousandsSeparator(null);
+        Settings::setLocale($this->originalLocale);
+        if ($this->currentPhpLocale !== false) {
+            setlocale(LC_ALL, $this->currentPhpLocale);
         }
         if ($this->spreadsheet !== null) {
             $this->spreadsheet->disconnectWorksheets();
@@ -50,12 +49,17 @@ class LocaleFloatsTest extends AbstractFunctional
         }
     }
 
+    /**
+     * Use separate process because this calls native Php setlocale.
+     *
+     * @runInSeparateProcess
+     */
     public function testLocaleFloatsCorrectlyConvertedByWriter(): void
     {
-        if (!$this->localeAdjusted) {
+        if (!setlocale(LC_ALL, 'fr_FR.UTF-8', 'fra_fra.utf8')) {
+            $this->currentPhpLocale = false;
             self::markTestSkipped('Unable to set locale for testing.');
         }
-
         $this->spreadsheet = $spreadsheet = new Spreadsheet();
         $properties = $spreadsheet->getProperties();
         $properties->setCustomProperty('Version', 1.2);
@@ -68,7 +72,67 @@ class LocaleFloatsTest extends AbstractFunctional
         $prop = $reloadedSpreadsheet->getProperties()->getCustomPropertyValue('Version');
         self::assertEqualsWithDelta(1.2, $prop, 1.0E-8);
 
-        $actual = sprintf('%f', $result);
+        $actual = $reloadedSpreadsheet->getActiveSheet()->getCell('A1')->getFormattedValue();
         self::assertStringContainsString('1,1', $actual);
     }
+
+    public function testPercentageStoredAsString(): void
+    {
+        Settings::setLocale('fr_FR');
+        StringHelper::setDecimalSeparator(',');
+        StringHelper::setThousandsSeparator('.');
+        $reader = new XlsxReader();
+        $this->spreadsheet = $spreadsheet = $reader->load('tests/data/Writer/Xlsx/issue.3811b.xlsx');
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertSame('48,34%', $sheet->getCell('L2')->getValue());
+        self::assertIsString($sheet->getCell('L2')->getValue());
+        self::assertSame('=(10%+L2)/2', $sheet->getCell('L1')->getValue());
+        self::assertEqualsWithDelta(0.2917, $sheet->getCell('L1')->getCalculatedValue(), 1E-8);
+        self::assertIsFloat($sheet->getCell('L1')->getCalculatedValue());
+        self::assertEquals('29,17%', $sheet->getCell('L1')->getFormattedValue());
+
+        $sheet->getCell('A10')->setValue(3.2);
+        self::assertSame(NumberFormat::FORMAT_GENERAL, $sheet->getStyle('A10')->getNumberFormat()->getFormatCode());
+        self::assertSame('3,2', $sheet->getCell('A10')->getFormattedValue());
+        $sheet->getCell('A11')->setValue(1002.5);
+        $sheet->getStyle('A11')->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_NUMBER_COMMA_SEPARATED1);
+        self::assertSame('1.002,50', $sheet->getCell('A11')->getFormattedValue());
+        $sheet->getCell('A12')->setValue(2.5);
+        $sheet->getStyle('A12')->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_NUMBER_00);
+        self::assertSame('2,50', $sheet->getCell('A12')->getFormattedValue());
+    }
+
+    /**
+     * Use separate process because this calls native Php setlocale.
+     *
+     * @runInSeparateProcess
+     */
+    public function testPercentageStoredAsString2(): void
+    {
+        if (!setlocale(LC_ALL, 'fr_FR.UTF-8', 'fra_fra.utf8')) {
+            $this->currentPhpLocale = false;
+            self::markTestSkipped('Unable to set locale for testing.');
+        }
+        $reader = new XlsxReader();
+        $this->spreadsheet = $spreadsheet = $reader->load('tests/data/Writer/Xlsx/issue.3811b.xlsx');
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertSame('48,34%', $sheet->getCell('L2')->getValue());
+        self::assertIsString($sheet->getCell('L2')->getValue());
+        self::assertSame('=(10%+L2)/2', $sheet->getCell('L1')->getValue());
+        self::assertEqualsWithDelta(0.2917, $sheet->getCell('L1')->getCalculatedValue(), 1E-8);
+        self::assertIsFloat($sheet->getCell('L1')->getCalculatedValue());
+        self::assertEquals('29,17%', $sheet->getCell('L1')->getFormattedValue());
+
+        $sheet->getCell('A10')->setValue(3.2);
+        self::assertSame(NumberFormat::FORMAT_GENERAL, $sheet->getStyle('A10')->getNumberFormat()->getFormatCode());
+        self::assertSame('3,2', $sheet->getCell('A10')->getFormattedValue());
+        $narrowNonBreakSpace = "\u{202f}";
+        self::assertSame($narrowNonBreakSpace, localeconv()['thousands_sep']);
+        $sheet->getCell('A11')->setValue(1002.5);
+        $sheet->getStyle('A11')->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_NUMBER_COMMA_SEPARATED1);
+        self::assertSame('1' . $narrowNonBreakSpace . '002,50', $sheet->getCell('A11')->getFormattedValue());
+        $sheet->getCell('A12')->setValue(2.5);
+        $sheet->getStyle('A12')->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_NUMBER_00);
+        self::assertSame('2,50', $sheet->getCell('A12')->getFormattedValue());
+    }
 }