From 76144671ce0aa5ba1c30608d5750d3076bacadcf Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Mon, 14 Jun 2021 10:55:54 -0700 Subject: [PATCH] Fix for Issue 2158 (AverageIf Calculation Problem) Issue #2158 reports an error calculating AverageIf because a function returns null rather than a string. There turn out to be several components to this problem: - The nominal fix to the problem is to add some null-to-nullstring coercion in DatabaseAbstract. - This fixes the error, but does not necessarily lead to the correct result because buildQuery treats values of null and null-string identically, whereas Excel does not. So change that to treat null-string as any other string. - But that doesn't lead to the correct result either. That's because Functions/ifCondition recognizes a null string, but then continues to (over-)process it until it returns the wrong result. Fix this problem in conjunction with the other two, and we finally get the correct result. A new unit test is added for AVERAGEIF, and new test cases are added for SUMIF. In each case, there are complementary tests for conditions of null and null-string, and the results agree with Excel. There may or may not be value in adding new tests to other functions, and I will be glad to do so for any functions which you care to identify, but no existing tests broke as a result of these changes. --- .../Calculation/Database/DatabaseAbstract.php | 6 +-- src/PhpSpreadsheet/Calculation/Functions.php | 2 +- .../Functions/MathTrig/SumIfTest.php | 3 +- .../Functions/Statistical/AverageIf2Test.php | 40 +++++++++++++++++++ tests/data/Calculation/MathTrig/SUMIF.php | 18 +++++++++ 5 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Calculation/Functions/Statistical/AverageIf2Test.php diff --git a/src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php b/src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php index b2b0e6691e..3c8b176fba 100644 --- a/src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php +++ b/src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php @@ -95,7 +95,7 @@ private static function buildQuery(array $criteriaNames, array $criteria): strin foreach ($criteria as $key => $criterion) { foreach ($criterion as $field => $value) { $criterionName = $criteriaNames[$field]; - if ($value !== null && $value !== '') { + if ($value !== null) { $condition = self::buildCondition($value, $criterionName); $baseQuery[$key][] = $condition; } @@ -104,12 +104,12 @@ private static function buildQuery(array $criteriaNames, array $criteria): strin $rowQuery = array_map( function ($rowValue) { - return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : $rowValue[0]; + return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : ($rowValue[0] ?? ''); }, $baseQuery ); - return (count($rowQuery) > 1) ? 'OR(' . implode(',', $rowQuery) . ')' : $rowQuery[0]; + return (count($rowQuery) > 1) ? 'OR(' . implode(',', $rowQuery) . ')' : ($rowQuery[0] ?? ''); } private static function buildCondition($criterion, string $criterionName): string diff --git a/src/PhpSpreadsheet/Calculation/Functions.php b/src/PhpSpreadsheet/Calculation/Functions.php index aea2323e37..9b56aad4cb 100644 --- a/src/PhpSpreadsheet/Calculation/Functions.php +++ b/src/PhpSpreadsheet/Calculation/Functions.php @@ -251,7 +251,7 @@ public static function ifCondition($condition) $condition = self::flattenSingleValue($condition); if ($condition === '') { - $condition = '=""'; + return '=""'; } if (!is_string($condition) || !in_array($condition[0], ['>', '<', '='])) { $condition = self::operandSpecialHandling($condition); diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumIfTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumIfTest.php index 39ac9a66ab..93c2ae32f8 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumIfTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/MathTrig/SumIfTest.php @@ -20,8 +20,7 @@ public function testSUMIF2($expectedResult, array $array1, $condition, ?array $a $sheet->fromArray($array1, null, 'A1', true); $maxARow = count($array1); $firstArg = "A1:A$maxARow"; - //$secondArg = is_string($condition) ? "\"$condition\"" : $condition; - $sheet->getCell('B1')->setValue($condition); + $this->setCell('B1', $condition); $secondArg = 'B1'; if (empty($array2)) { $sheet->getCell('D1')->setValue("=SUMIF($firstArg, $secondArg)"); diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/Statistical/AverageIf2Test.php b/tests/PhpSpreadsheetTests/Calculation/Functions/Statistical/AverageIf2Test.php new file mode 100644 index 0000000000..1899239f82 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/Statistical/AverageIf2Test.php @@ -0,0 +1,40 @@ +getActiveSheet(); + $sheet->fromArray($table, null, 'A1', true); + $sheet->getCell('C1')->setValue('=AVERAGEIF(A:A,"",B:B)'); + $sheet->getCell('C2')->setValue('=AVERAGEIF(A:A,,B:B)'); + $sheet->getCell('C3')->setValue('=AVERAGEIF(A:A,"n",B:B)'); + $sheet->getCell('C4')->setValue('=AVERAGEIF(A:A,"y",B:B)'); + $sheet->getCell('C5')->setValue('=AVERAGEIF(A:A,"x",B:B)'); + + self::assertEqualsWithDelta(5.75, $sheet->getCell('C1')->getCalculatedValue(), 1E-12); + self::assertEquals('#DIV/0!', $sheet->getCell('C2')->getCalculatedValue()); + self::assertEqualsWithDelta(4.75, $sheet->getCell('C3')->getCalculatedValue(), 1E-12); + self::assertEqualsWithDelta(3, $sheet->getCell('C4')->getCalculatedValue(), 1E-12); + self::assertEquals('#DIV/0!', $sheet->getCell('C5')->getCalculatedValue()); + $spreadsheet->disconnectWorksheets(); + } +} diff --git a/tests/data/Calculation/MathTrig/SUMIF.php b/tests/data/Calculation/MathTrig/SUMIF.php index 0a6cfbdc80..a427624f30 100644 --- a/tests/data/Calculation/MathTrig/SUMIF.php +++ b/tests/data/Calculation/MathTrig/SUMIF.php @@ -150,4 +150,22 @@ 'North ?', [[36693], [22100], ['=3/0'], [34440], [29889], [50090], [32080], [45500]], ], + [ + 23, + [['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []], + '', + [[1], [2], [3], [4], [5], [6], [7], [8], [9]], + ], + [ + 0, + [['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []], + null, + [[1], [2], [3], [4], [5], [6], [7], [8], [9]], + ], + [ + 19, + [['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []], + 'n', + [[1], [2], [3], [4], [5], [6], [7], [8], [9]], + ], ];