Breaking Change to toFormattedString and 3 toArray Methods (#3304)

* Breaking Change to toFormattedString and 3 toArray Methods

DocBlock says `toFormattedString` returns string, but code can return int, float, bool, null, or RichText. I think the DocBlock is correct (function name clearly indicates "to string"), and the code needs to change to match it. This is not that big a deal on its own. However, some methods in `Worksheet` call this function by default - `rangeToArray`, `toArray`, and `namedRangeToArray`. All 3 can be called with a parameter so that they don't call `toFormattedString`, but, by default, they do call, and so are affected by this change. Some unit test results are changed as a result of this code change. In all those cases, an additional test is added which would match the previous result.

* Update Change Log

I often skip this till later, but, since it's a breaking change ...

* Scrutinizer, Plus Strange Cast to -0

Avoid Scrutinizer complaint by casting string to float, but ... Php decides resulted should sometimes be `-0`? That caused test failures. Very odd.

CHANGELOG.md

@@ -39,7 +39,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 
 ### Changed
 
-- Nothing
+- `toFormattedString` will now always return a string. This can affect the results of `toArray`, `namedRangeToArray`, and `rangeToArray`. [PR #3304](https://github.com/PHPOffice/PhpSpreadsheet/pull/3304)
 
 ### Deprecated
 

phpstan-baseline.neon

@@ -790,56 +790,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Shared/Trend/Trend.php
 
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormat\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormat\\(\\) has parameter \\$sections with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormat\\(\\) has parameter \\$value with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has parameter \\$cond with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has parameter \\$dfcond with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has parameter \\$dfval with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has parameter \\$val with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:splitFormatCompare\\(\\) has parameter \\$value with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\Formatter\\:\\:toFormattedString\\(\\) should return string but returns float\\|int\\|string\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat/Formatter.php
-
 		-
 			message: "#^Negated boolean expression is always false\\.$#"
 			count: 1

src/PhpSpreadsheet/Style/NumberFormat/Formatter.php

@@ -2,12 +2,19 @@
 
 namespace PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
+use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
+use PhpOffice\PhpSpreadsheet\RichText\RichText;
 use PhpOffice\PhpSpreadsheet\Style\Color;
 use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
 
 class Formatter
 {
-    private static function splitFormatCompare($value, $cond, $val, $dfcond, $dfval)
+    /**
+     * @param mixed $value
+     * @param mixed $val
+     * @param mixed $dfval
+     */
+    private static function splitFormatCompare($value, ?string $cond, $val, string $dfcond, $dfval): bool
     {
         if (!$cond) {
             $cond = $dfcond;
@@ -33,7 +40,8 @@ private static function splitFormatCompare($value, $cond, $val, $dfcond, $dfval)
         return $value >= $val;
     }
 
-    private static function splitFormat($sections, $value)
+    /** @param mixed $value */
+    private static function splitFormat(array $sections, $value): array
     {
         // Extract the relevant section depending on whether number is positive, negative, or zero?
         // Text not supported yet.
@@ -93,23 +101,26 @@ private static function splitFormat($sections, $value)
     /**
      * Convert a value in a pre-defined format to a PHP string.
      *
-     * @param mixed $value Value to format
+     * @param null|bool|float|int|RichText|string $value Value to format
      * @param string $format Format code, see = NumberFormat::FORMAT_*
      * @param array $callBack Callback function for additional formatting of string
      *
      * @return string Formatted string
      */
     public static function toFormattedString($value, $format, $callBack = null)
     {
+        if (is_bool($value)) {
+            return $value ? Calculation::getTRUE() : Calculation::getFALSE();
+        }
         // For now we do not treat strings although section 4 of a format code affects strings
         if (!is_numeric($value)) {
-            return $value;
+            return (string) $value;
         }
 
         // 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 $value;
+            return (string) $value;
         }
 
         // Ignore square-$-brackets prefix in format string, like "[$-411]ge.m.d", "[$-010419]0%", etc
@@ -127,7 +138,7 @@ function ($matches) {
         $format = (string) preg_replace('/(\\\(((.)(?!((AM\/PM)|(A\/P))))|([^ ])))(?=(?:[^"]|"[^"]*")*$)/ui', '"${2}"', $format);
 
         // Get the sections, there can be up to four sections, separated with a semi-colon (but only if not a quoted literal)
-        $sections = preg_split('/(;)(?=(?:[^"]|"[^"]*")*$)/u', $format);
+        $sections = preg_split('/(;)(?=(?:[^"]|"[^"]*")*$)/u', $format) ?: [];
 
         [$colors, $format, $value] = self::splitFormat($sections, $value);
 
@@ -145,8 +156,8 @@ function ($matches) {
             if (substr($format, 0, 1) === '"' && substr($format, -1, 1) === '"' && substr_count($format, '"') === 2) {
                 $value = substr($format, 1, -1);
             } elseif (preg_match('/[0#, ]%/', $format)) {
-                // % number format
-                $value = PercentageFormatter::format($value, $format);
+                // % number format - avoid weird '-0' problem
+                $value = PercentageFormatter::format(0 + (float) $value, $format);
             } else {
                 $value = NumberFormatter::format($value, $format);
             }

tests/PhpSpreadsheetTests/Reader/Ods/HiddenMergeCellsTest.php

@@ -3,46 +3,43 @@
 namespace PhpOffice\PhpSpreadsheetTests\Reader\Ods;
 
 use PhpOffice\PhpSpreadsheet\Reader\Ods;
-use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PHPUnit\Framework\TestCase;
 
 class HiddenMergeCellsTest extends TestCase
 {
-    /**
-     * @var Spreadsheet
-     */
-    private $spreadsheet;
-
-    protected function setup(): void
-    {
-        $filename = 'tests/data/Reader/Ods/HiddenMergeCellsTest.ods';
-        $reader = new Ods();
-        $this->spreadsheet = $reader->load($filename);
-    }
+    private const FILENAME = 'tests/data/Reader/Ods/HiddenMergeCellsTest.ods';
 
     public function testHiddenMergeCells(): void
     {
-        $c2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
+        $reader = new Ods();
+        $spreadsheet = $reader->load(self::FILENAME);
+        $c2InMergeRange = $spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
         self::assertTrue($c2InMergeRange);
-        $a2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
+        $a2InMergeRange = $spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
         self::assertTrue($a2InMergeRange);
-        $a2MergeRangeValue = $this->spreadsheet->getActiveSheet()->getCell('A2')->isMergeRangeValueCell();
+        $a2MergeRangeValue = $spreadsheet->getActiveSheet()->getCell('A2')->isMergeRangeValueCell();
         self::assertTrue($a2MergeRangeValue);
 
-        $cellArray = $this->spreadsheet->getActiveSheet()->rangeToArray('A2:C2');
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2');
+        self::assertSame([['12', '4', '3']], $cellArray);
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, true, false);
         self::assertSame([[12, 4, 3]], $cellArray);
+        $spreadsheet->disconnectWorksheets();
     }
 
     public function testUnmergeHiddenMergeCells(): void
     {
-        $this->spreadsheet->getActiveSheet()->unmergeCells('A2:C2');
+        $reader = new Ods();
+        $spreadsheet = $reader->load(self::FILENAME);
+        $spreadsheet->getActiveSheet()->unmergeCells('A2:C2');
 
-        $c2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
+        $c2InMergeRange = $spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
         self::assertFalse($c2InMergeRange);
-        $a2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
+        $a2InMergeRange = $spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
         self::assertFalse($a2InMergeRange);
 
-        $cellArray = $this->spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, false, false, false);
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, false, false, false);
         self::assertSame([[12, '=6-B1', '=A2/B2']], $cellArray);
+        $spreadsheet->disconnectWorksheets();
     }
 }

tests/PhpSpreadsheetTests/Reader/Ods/HiddenWorksheetTest.php

@@ -3,29 +3,19 @@
 namespace PhpOffice\PhpSpreadsheetTests\Reader\Ods;
 
 use PhpOffice\PhpSpreadsheet\Reader\Ods;
-use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
 use PHPUnit\Framework\TestCase;
 
 class HiddenWorksheetTest extends TestCase
 {
-    /**
-     * @var Spreadsheet
-     */
-    private $spreadsheet;
-
-    protected function setup(): void
+    public function testPageSetup(): void
     {
         $filename = 'tests/data/Reader/Ods/HiddenSheet.ods';
         $reader = new Ods();
-        $this->spreadsheet = $reader->load($filename);
-    }
-
-    public function testPageSetup(): void
-    {
+        $spreadsheet = $reader->load($filename);
         $assertions = $this->worksheetAssertions();
 
-        foreach ($this->spreadsheet->getAllSheets() as $worksheet) {
+        foreach ($spreadsheet->getAllSheets() as $worksheet) {
             if (!array_key_exists($worksheet->getTitle(), $assertions)) {
                 continue;
             }
@@ -40,6 +30,7 @@ public function testPageSetup(): void
                 );
             }
         }
+        $spreadsheet->disconnectWorksheets();
     }
 
     private function worksheetAssertions(): array

tests/PhpSpreadsheetTests/Reader/Xls/HiddenMergeCellsTest.php

@@ -3,46 +3,43 @@
 namespace PhpOffice\PhpSpreadsheetTests\Reader\Xls;
 
 use PhpOffice\PhpSpreadsheet\Reader\Xls;
-use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PHPUnit\Framework\TestCase;
 
 class HiddenMergeCellsTest extends TestCase
 {
-    /**
-     * @var Spreadsheet
-     */
-    private $spreadsheet;
-
-    protected function setup(): void
-    {
-        $filename = 'tests/data/Reader/XLS/HiddenMergeCellsTest.xls';
-        $reader = new Xls();
-        $this->spreadsheet = $reader->load($filename);
-    }
+    private const FILENAME = 'tests/data/Reader/XLS/HiddenMergeCellsTest.xls';
 
     public function testHiddenMergeCells(): void
     {
-        $c2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
+        $reader = new Xls();
+        $spreadsheet = $reader->load(self::FILENAME);
+        $c2InMergeRange = $spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
         self::assertTrue($c2InMergeRange);
-        $a2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
+        $a2InMergeRange = $spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
         self::assertTrue($a2InMergeRange);
-        $a2MergeRangeValue = $this->spreadsheet->getActiveSheet()->getCell('A2')->isMergeRangeValueCell();
+        $a2MergeRangeValue = $spreadsheet->getActiveSheet()->getCell('A2')->isMergeRangeValueCell();
         self::assertTrue($a2MergeRangeValue);
 
-        $cellArray = $this->spreadsheet->getActiveSheet()->rangeToArray('A2:C2');
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2');
+        self::assertSame([['12', '4', '3']], $cellArray);
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, true, false);
         self::assertSame([[12, 4, 3]], $cellArray);
+        $spreadsheet->disconnectWorksheets();
     }
 
     public function testUnmergeHiddenMergeCells(): void
     {
-        $this->spreadsheet->getActiveSheet()->unmergeCells('A2:C2');
+        $reader = new Xls();
+        $spreadsheet = $reader->load(self::FILENAME);
+        $spreadsheet->getActiveSheet()->unmergeCells('A2:C2');
 
-        $c2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
+        $c2InMergeRange = $spreadsheet->getActiveSheet()->getCell('C2')->isInMergeRange();
         self::assertFalse($c2InMergeRange);
-        $a2InMergeRange = $this->spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
+        $a2InMergeRange = $spreadsheet->getActiveSheet()->getCell('A2')->isInMergeRange();
         self::assertFalse($a2InMergeRange);
 
-        $cellArray = $this->spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, false, false, false);
+        $cellArray = $spreadsheet->getActiveSheet()->rangeToArray('A2:C2', null, false, false, false);
         self::assertSame([[12, '=6-B1', '=A2/B2']], $cellArray);
+        $spreadsheet->disconnectWorksheets();
     }
 }

tests/PhpSpreadsheetTests/Reader/Xls/HiddenWorksheetTest.php

@@ -3,29 +3,19 @@
 namespace PhpOffice\PhpSpreadsheetTests\Reader\Xls;
 
 use PhpOffice\PhpSpreadsheet\Reader\Xls;
-use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
 use PHPUnit\Framework\TestCase;
 
 class HiddenWorksheetTest extends TestCase
 {
-    /**
-     * @var Spreadsheet
-     */
-    private $spreadsheet;
-
-    protected function setup(): void
+    public function testPageSetup(): void
     {
         $filename = 'tests/data/Reader/XLS/HiddenSheet.xls';
         $reader = new Xls();
-        $this->spreadsheet = $reader->load($filename);
-    }
-
-    public function testPageSetup(): void
-    {
+        $spreadsheet = $reader->load($filename);
         $assertions = $this->worksheetAssertions();
 
-        foreach ($this->spreadsheet->getAllSheets() as $worksheet) {
+        foreach ($spreadsheet->getAllSheets() as $worksheet) {
             if (!array_key_exists($worksheet->getTitle(), $assertions)) {
                 continue;
             }
@@ -40,6 +30,7 @@ public function testPageSetup(): void
                 );
             }
         }
+        $spreadsheet->disconnectWorksheets();
     }
 
     private function worksheetAssertions(): array

tests/PhpSpreadsheetTests/Reader/Xls/NumberFormatGeneralTest.php

@@ -21,7 +21,7 @@ public function testGeneral(): void
         self::assertSame(2.95, $sheet->getCell('D2')->getValue());
         self::assertSame(2.95, $sheet->getCell('D2')->getCalculatedValue());
         self::assertSame('€ 2.95', $sheet->getCell('D2')->getFormattedValue());
-        self::assertSame(21, $array[1][4]);
+        self::assertSame('21', $array[1][4]);
         self::assertSame(21, $sheet->getCell('E2')->getValue());
         self::assertSame(21, $sheet->getCell('E2')->getCalculatedValue());
         self::assertSame('21', $sheet->getCell('E2')->getFormattedValue());