From a89572107a655ac3fe8107034c7c1a3acb7b4740 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sat, 25 Jun 2022 22:08:32 -0700 Subject: [PATCH] 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. --- .../Calculation/Calculation.php | 2 +- .../Calculation/Information/ExcelError.php | 8 +++ .../Calculation/MathTrig/Helpers.php | 4 +- .../Calculation/MathTrig/Operations.php | 2 +- .../Calculation/MathTrig/Subtotal.php | 27 ++++++++- .../Functions/MathTrig/SubTotalTest.php | 28 +++++++++ .../Calculation/RefErrorTest.php | 58 +++++++++++++++++++ 7 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Calculation/RefErrorTest.php diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 516faa86..4e3a7c53 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/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'); diff --git a/src/PhpSpreadsheet/Calculation/Information/ExcelError.php b/src/PhpSpreadsheet/Calculation/Information/ExcelError.php index 88de7e54..6305e502 100644 --- a/src/PhpSpreadsheet/Calculation/Information/ExcelError.php +++ b/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. * diff --git a/src/PhpSpreadsheet/Calculation/MathTrig/Helpers.php b/src/PhpSpreadsheet/Calculation/MathTrig/Helpers.php index 348aa5b3..f34f159b 100644 --- a/src/PhpSpreadsheet/Calculation/MathTrig/Helpers.php +++ b/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)); } /** diff --git a/src/PhpSpreadsheet/Calculation/MathTrig/Operations.php b/src/PhpSpreadsheet/Calculation/MathTrig/Operations.php index f26da389..06258451 100644 --- a/src/PhpSpreadsheet/Calculation/MathTrig/Operations.php +++ b/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); } } diff --git a/src/PhpSpreadsheet/Calculation/MathTrig/Subtotal.php b/src/PhpSpreadsheet/Calculation/MathTrig/Subtotal.php index 336bc690..6d8f4723 100644 --- a/src/PhpSpreadsheet/Calculation/MathTrig/Subtotal.php +++ b/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); diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SubTotalTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SubTotalTest.php index cf79ac0b..43156182 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SubTotalTest.php +++ b/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()); + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/RefErrorTest.php b/tests/PhpSpreadsheetTests/Calculation/RefErrorTest.php new file mode 100644 index 00000000..ffdfc2fa --- /dev/null +++ b/tests/PhpSpreadsheetTests/Calculation/RefErrorTest.php @@ -0,0 +1,58 @@ +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)'], + ]; + } +}