Skip to content

Commit

Permalink
Fix for Issue 2158 (AverageIf Calculation Problem)
Browse files Browse the repository at this point in the history
Issue PHPOffice#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.
  • Loading branch information
oleibman committed Jun 14, 2021
1 parent fea56e1 commit 7614467
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 6 deletions.
6 changes: 3 additions & 3 deletions src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/PhpSpreadsheet/Calculation/Functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\Statistical;

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PHPUnit\Framework\TestCase;

class AverageIf2Test extends TestCase
{
public function testAVERAGEIF(): void
{
$table = [
['n', 1],
['', 2],
['y', 3],
['', 4],
['n', 5],
['n', 6],
['n', 7],
['', 8],
[null, 9],
];

$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->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();
}
}
18 changes: 18 additions & 0 deletions tests/data/Calculation/MathTrig/SUMIF.php
Original file line number Diff line number Diff line change
Expand Up @@ -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]],
],
];

0 comments on commit 7614467

Please sign in to comment.