Fix for Issue 2158 (AverageIf Calculation Problem) (#2160)
* Improve Identification of Samples in Coverage Report The Phpunit coverage report currently contains bullet items like `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "49"`. This extremely simple change takes advantage of Phpunit's ability to accept an array with keys which are either strings or integers, by using the sample filenames as the array keys rather than sequential but otherwise meaningless integers (e.g. `49` in the earlier cited item). The bullet item will now read `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "Basic/38_Clone_worksheet.php"`. * 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.
This commit is contained in:
parent
4f06d84248
commit
1e74282259
|
|
@ -95,7 +95,7 @@ abstract class DatabaseAbstract
|
||||||
foreach ($criteria as $key => $criterion) {
|
foreach ($criteria as $key => $criterion) {
|
||||||
foreach ($criterion as $field => $value) {
|
foreach ($criterion as $field => $value) {
|
||||||
$criterionName = $criteriaNames[$field];
|
$criterionName = $criteriaNames[$field];
|
||||||
if ($value !== null && $value !== '') {
|
if ($value !== null) {
|
||||||
$condition = self::buildCondition($value, $criterionName);
|
$condition = self::buildCondition($value, $criterionName);
|
||||||
$baseQuery[$key][] = $condition;
|
$baseQuery[$key][] = $condition;
|
||||||
}
|
}
|
||||||
|
|
@ -104,12 +104,12 @@ abstract class DatabaseAbstract
|
||||||
|
|
||||||
$rowQuery = array_map(
|
$rowQuery = array_map(
|
||||||
function ($rowValue) {
|
function ($rowValue) {
|
||||||
return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : $rowValue[0];
|
return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : ($rowValue[0] ?? '');
|
||||||
},
|
},
|
||||||
$baseQuery
|
$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
|
private static function buildCondition($criterion, string $criterionName): string
|
||||||
|
|
|
||||||
|
|
@ -251,7 +251,7 @@ class Functions
|
||||||
$condition = self::flattenSingleValue($condition);
|
$condition = self::flattenSingleValue($condition);
|
||||||
|
|
||||||
if ($condition === '') {
|
if ($condition === '') {
|
||||||
$condition = '=""';
|
return '=""';
|
||||||
}
|
}
|
||||||
if (!is_string($condition) || !in_array($condition[0], ['>', '<', '='])) {
|
if (!is_string($condition) || !in_array($condition[0], ['>', '<', '='])) {
|
||||||
$condition = self::operandSpecialHandling($condition);
|
$condition = self::operandSpecialHandling($condition);
|
||||||
|
|
|
||||||
|
|
@ -20,8 +20,7 @@ class SumIfTest extends AllSetupTeardown
|
||||||
$sheet->fromArray($array1, null, 'A1', true);
|
$sheet->fromArray($array1, null, 'A1', true);
|
||||||
$maxARow = count($array1);
|
$maxARow = count($array1);
|
||||||
$firstArg = "A1:A$maxARow";
|
$firstArg = "A1:A$maxARow";
|
||||||
//$secondArg = is_string($condition) ? "\"$condition\"" : $condition;
|
$this->setCell('B1', $condition);
|
||||||
$sheet->getCell('B1')->setValue($condition);
|
|
||||||
$secondArg = 'B1';
|
$secondArg = 'B1';
|
||||||
if (empty($array2)) {
|
if (empty($array2)) {
|
||||||
$sheet->getCell('D1')->setValue("=SUMIF($firstArg, $secondArg)");
|
$sheet->getCell('D1')->setValue("=SUMIF($firstArg, $secondArg)");
|
||||||
|
|
|
||||||
|
|
@ -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();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -150,4 +150,22 @@ return [
|
||||||
'North ?',
|
'North ?',
|
||||||
[[36693], [22100], ['=3/0'], [34440], [29889], [50090], [32080], [45500]],
|
[[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]],
|
||||||
|
],
|
||||||
];
|
];
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue