Handling of #REF! Errors in Subtotal, and More (#2902)

* Handling of #REF! Errors in Subtotal, and More

This PR derives from, and supersedes, PR #2870, submitted by @ndench. The problem reported in the original is that SUBTOTAL does not handle #REF! errors in its arguments properly; however, my investigation has enlarged the scope.

The main problem is in Calculation, and it has a simple fix. When the calculation engine finds a reference to an uninitialized cell, it uses `null` as the value. This is appropriate when the cell belongs to a defined sheet; however, for an undefined sheet, #REF! is more appropriate.

With that fix in place, SUBTOTAL still needs a small fix of its own. It tries to parse its cell reference arguments into an array, but, if the reference does not match the expected format (as #REF! will not), this results in referencing undefined array indexes, with attendant messages. That assignment is changed to be more flexible, eliminating the problem and the messages.

Those 2 fixes are sufficient to ensure that the original problem is resolved. It also resolves a similar problem with some other functions (e.g. SUM). However, it does not resolve it for all functions. Or, to be more particular, many functions will return #VALUE! rather than #REF! if this arises, and the same is true for other errors in the function arguments, e.g. #DIV/0!. This PR does not attempt to address all functions; I need to think of a systematic way to pursue that. However, at least for most MathTrig functions, which validate their arguments using a common method, it is relatively easy to get the function to propagate the proper error result.

* Arrange Array The Way call_user_func_array Wants

Problem with Php8.0+ - array passed to call_user_func_array must have int keys before string keys, otherwise Php thinks we are passing positional parameters after keyword parameters.

7 other functions use flattenArrayIndexed, but Subtotal is the only one which uses that result to subsequently pass arguments to call_user_func_array. So the others should not require a change. A specific test is added for SUM to validate that conclusion.

* Change Needed for Hidden Row Filter

Same as change made to Formula Args filter.

src/PhpSpreadsheet/Calculation/Calculation.php

@@ -4834,7 +4834,7 @@ class Calculation
                                     $cell->attach($pCellParent);
                                 } else {
                                     $cellRef = ($cellSheet !== null) ? "'{$matches[2]}'!{$cellRef}" : $cellRef;
-                                    $cellValue = null;
+                                    $cellValue = ($cellSheet !== null) ? null : Information\ExcelError::REF();
                                 }
                             } else {
                                 return $this->raiseFormulaError('Unable to access Cell Reference');

src/PhpSpreadsheet/Calculation/Information/ExcelError.php

@@ -25,6 +25,14 @@ class ExcelError
         'spill' => '#SPILL!',
     ];
 
+    /**
+     * @param mixed $value
+     */
+    public static function throwError($value): string
+    {
+        return in_array($value, self::$errorCodes, true) ? $value : self::$errorCodes['value'];
+    }
+
     /**
      * ERROR_TYPE.
      *

src/PhpSpreadsheet/Calculation/MathTrig/Helpers.php

@@ -38,7 +38,7 @@ class Helpers
             return 0 + $number;
         }
 
-        throw new Exception(ExcelError::VALUE());
+        throw new Exception(ExcelError::throwError($number));
     }
 
     /**
@@ -59,7 +59,7 @@ class Helpers
             return 0 + $number;
         }
 
-        throw new Exception(ExcelError::VALUE());
+        throw new Exception(ExcelError::throwError($number));
     }
 
     /**

src/PhpSpreadsheet/Calculation/MathTrig/Operations.php

@@ -118,7 +118,7 @@ class Operations
             if (is_numeric($arg)) {
                 $returnValue *= $arg;
             } else {
-                return ExcelError::VALUE();
+                return ExcelError::throwError($arg);
             }
         }
 

src/PhpSpreadsheet/Calculation/MathTrig/Subtotal.php

@@ -18,7 +18,11 @@ class Subtotal
         return array_filter(
             $args,
             function ($index) use ($cellReference) {
-                [, $row, ] = explode('.', $index);
+                $explodeArray = explode('.', $index);
+                $row = $explodeArray[1] ?? '';
+                if (!is_numeric($row)) {
+                    return true;
+                }
 
                 return $cellReference->getWorksheet()->getRowDimension($row)->getVisible();
             },
@@ -35,7 +39,9 @@ class Subtotal
         return array_filter(
             $args,
             function ($index) use ($cellReference) {
-                [, $row, $column] = explode('.', $index);
+                $explodeArray = explode('.', $index);
+                $row = $explodeArray[1] ?? '';
+                $column = $explodeArray[2] ?? '';
                 $retVal = true;
                 if ($cellReference->getWorksheet()->cellExists($column . $row)) {
                     //take this cell out if it contains the SUBTOTAL or AGGREGATE functions in a formula
@@ -87,7 +93,22 @@ class Subtotal
     public static function evaluate($functionType, ...$args)
     {
         $cellReference = array_pop($args);
-        $aArgs = Functions::flattenArrayIndexed($args);
+        $bArgs = Functions::flattenArrayIndexed($args);
+        $aArgs = [];
+        // int keys must come before string keys for PHP 8.0+
+        // Otherwise, PHP thinks positional args follow keyword
+        //    in the subsequent call to call_user_func_array.
+        // Fortunately, order of args is unimportant to Subtotal.
+        foreach ($bArgs as $key => $value) {
+            if (is_int($key)) {
+                $aArgs[$key] = $value;
+            }
+        }
+        foreach ($bArgs as $key => $value) {
+            if (!is_int($key)) {
+                $aArgs[$key] = $value;
+            }
+        }
 
         try {
             $subtotal = (int) Helpers::validateNumericNullBool($functionType);

tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SubTotalTest.php

@@ -127,4 +127,32 @@ class SubTotalTest extends AllSetupTeardown
         $sheet->getCell('H1')->setValue("=SUBTOTAL(9, A1:$maxCol$maxRow)");
         self::assertEquals(362, $sheet->getCell('H1')->getCalculatedValue());
     }
+
+    public function testRefError(): void
+    {
+        $sheet = $this->getSheet();
+        $sheet->getCell('A1')->setValue('=SUBTOTAL(9, #REF!)');
+        self::assertEquals('#REF!', $sheet->getCell('A1')->getCalculatedValue());
+    }
+
+    public function testSecondaryRefError(): void
+    {
+        $sheet = $this->getSheet();
+        $sheet->getCell('A1')->setValue('=SUBTOTAL(9, B1:B9,#REF!,C1:C9)');
+        self::assertEquals('#REF!', $sheet->getCell('A1')->getCalculatedValue());
+    }
+
+    public function testNonStringSingleCellRefError(): void
+    {
+        $sheet = $this->getSheet();
+        $sheet->getCell('A1')->setValue('=SUBTOTAL(9, 1, C1, Sheet99!A11)');
+        self::assertEquals('#REF!', $sheet->getCell('A1')->getCalculatedValue());
+    }
+
+    public function testNonStringCellRangeRefError(): void
+    {
+        $sheet = $this->getSheet();
+        $sheet->getCell('A1')->setValue('=SUBTOTAL(9, Sheet99!A1)');
+        self::assertEquals('#REF!', $sheet->getCell('A1')->getCalculatedValue());
+    }
 }

tests/PhpSpreadsheetTests/Calculation/RefErrorTest.php

@@ -0,0 +1,58 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Calculation;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PHPUnit\Framework\TestCase;
+
+class RefErrorTest extends TestCase
+{
+    /**
+     * @param mixed $expected
+     *
+     * @dataProvider providerRefError
+     */
+    public function testRefError($expected, string $formula): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $sheet1 = $spreadsheet->getActiveSheet();
+        $sheet1->setTitle('Sheet1');
+        $sheet2 = $spreadsheet->createSheet();
+        $sheet2->setTitle('Sheet2');
+        $sheet2->getCell('A1')->setValue(5);
+        $sheet1->getCell('A1')->setValue(9);
+        $sheet1->getCell('A2')->setValue(2);
+        $sheet1->getCell('A3')->setValue(4);
+        $sheet1->getCell('A4')->setValue(6);
+        $sheet1->getCell('A5')->setValue(7);
+        $sheet1->getRowDimension(5)->setVisible(false);
+        $sheet1->getCell('B1')->setValue('=1/0');
+        $sheet1->getCell('C1')->setValue('=Sheet99!A1');
+        $sheet1->getCell('C2')->setValue('=Sheet2!A1');
+        $sheet1->getCell('C3')->setValue('=Sheet2!A2');
+        $sheet1->getCell('H1')->setValue($formula);
+        self::assertSame($expected, $sheet1->getCell('H1')->getCalculatedValue());
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public function providerRefError(): array
+    {
+        return [
+            'Subtotal9 Ok' => [12, '=SUBTOTAL(A1,A2:A4)'],
+            'Subtotal9 REF' => ['#REF!', '=SUBTOTAL(A1,A2:A4,C1)'],
+            'Subtotal9 with literal and cells' => [111, '=SUBTOTAL(A1,A2:A4,99)'],
+            'Subtotal9 with literal no rows hidden' => [111, '=SUBTOTAL(109,A2:A4,99)'],
+            'Subtotal9 with literal ignoring hidden row' => [111, '=SUBTOTAL(109,A2:A5,99)'],
+            'Subtotal9 with literal using hidden row' => [118, '=SUBTOTAL(9,A2:A5,99)'],
+            'Subtotal9 with Null same sheet' => [12, '=SUBTOTAL(A1,A2:A4,A99)'],
+            'Subtotal9 with Null Different sheet' => [12, '=SUBTOTAL(A1,A2:A4,C3)'],
+            'Subtotal9 with NonNull Different sheet' => [17, '=SUBTOTAL(A1,A2:A4,C2)'],
+            'Product DIV0' => ['#DIV/0!', '=PRODUCT(2, 3, B1)'],
+            'Sqrt REF' => ['#REF!', '=SQRT(C1)'],
+            'Sum NUM' => ['#NUM!', '=SUM(SQRT(-1), A2:A4)'],
+            'Sum with literal and cells' => [111, '=SUM(A2:A4, 99)'],
+            'Sum REF' => ['#REF!', '=SUM(A2:A4, C1)'],
+            'Tan DIV0' => ['#DIV/0!', '=TAN(B1)'],
+        ];
+    }
+}