From 7b38d8c07f5d2ad7d07a455258ba4fdc40e647ba Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 16 May 2025 20:46:09 -0700 Subject: [PATCH] WIP Bug in resizeMatricesExtend Fix #4451. The reporter's analysis is correct - the method has a bug. The solution is relatively easy. Nevertheless, proving that the fix works as desired is tricky. The submitter's Reflection test is good, but it would be better to find one within Excel. My additional tests are contrived, for reasons explained in the test member. So, I will leave this as a work in progress while I search for something better. If that search doesn't succeed, I will nevertheless merge this in about a month. --- .../Calculation/Calculation.php | 16 +-- .../Calculation/LookupRef/Filter.php | 2 +- .../Calculation/Issue4451Test.php | 128 ++++++++++++++++++ 3 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Calculation/Issue4451Test.php diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index cca6ca446..05ebe0c1b 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -845,15 +845,15 @@ class Calculation extends CalculationLocale if (($matrix2Columns < $matrix1Columns) || ($matrix2Rows < $matrix1Rows)) { if ($matrix2Columns < $matrix1Columns) { for ($i = 0; $i < $matrix2Rows; ++$i) { - $x = $matrix2[$i][$matrix2Columns - 1]; + $x = ($matrix2Columns === 1) ? $matrix2[$i][0] : null; for ($j = $matrix2Columns; $j < $matrix1Columns; ++$j) { $matrix2[$i][$j] = $x; } } } if ($matrix2Rows < $matrix1Rows) { - $x = $matrix2[$matrix2Rows - 1]; - for ($i = 0; $i < $matrix1Rows; ++$i) { + $x = ($matrix2Rows === 1) ? $matrix2[0] : array_fill(0, $matrix2Columns, null); + for ($i = $matrix2Rows; $i < $matrix1Rows; ++$i) { $matrix2[$i] = $x; } } @@ -862,15 +862,15 @@ class Calculation extends CalculationLocale if (($matrix1Columns < $matrix2Columns) || ($matrix1Rows < $matrix2Rows)) { if ($matrix1Columns < $matrix2Columns) { for ($i = 0; $i < $matrix1Rows; ++$i) { - $x = $matrix1[$i][$matrix1Columns - 1]; + $x = ($matrix1Columns === 1) ? $matrix1[$i][0] : null; for ($j = $matrix1Columns; $j < $matrix2Columns; ++$j) { $matrix1[$i][$j] = $x; } } } if ($matrix1Rows < $matrix2Rows) { - $x = $matrix1[$matrix1Rows - 1]; - for ($i = 0; $i < $matrix2Rows; ++$i) { + $x = ($matrix1Rows === 1) ? $matrix1[0] : array_fill(0, $matrix1Columns, null); + for ($i = $matrix1Rows; $i < $matrix2Rows; ++$i) { $matrix1[$i] = $x; } } @@ -2334,14 +2334,14 @@ class Calculation extends CalculationLocale for ($row = 0; $row < $rows; ++$row) { for ($column = 0; $column < $columns; ++$column) { - if ($operand1[$row][$column] === null) { + if (($operand1[$row][$column] ?? null) === null) { $operand1[$row][$column] = 0; } elseif (!self::isNumericOrBool($operand1[$row][$column])) { $operand1[$row][$column] = self::makeError($operand1[$row][$column]); continue; } - if ($operand2[$row][$column] === null) { + if (($operand2[$row][$column] ?? null) === null) { $operand2[$row][$column] = 0; } elseif (!self::isNumericOrBool($operand2[$row][$column])) { $operand1[$row][$column] = self::makeError($operand2[$row][$column]); diff --git a/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php b/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php index daedcd0b9..1a2553beb 100644 --- a/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php +++ b/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php @@ -45,7 +45,7 @@ class Filter return array_filter( array_values($lookupArray), - fn ($index): bool => (bool) $matchArray[$index], + fn ($index): bool => (bool) ($matchArray[$index] ?? null), ARRAY_FILTER_USE_KEY ); } diff --git a/tests/PhpSpreadsheetTests/Calculation/Issue4451Test.php b/tests/PhpSpreadsheetTests/Calculation/Issue4451Test.php new file mode 100644 index 000000000..3b9107d18 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Calculation/Issue4451Test.php @@ -0,0 +1,128 @@ +setAccessible(true); + + // Call the method using reflection + $reflectionMethod->invokeArgs($calculation, [&$matrix1, &$matrix2, count($matrix1), 1, count($matrix2), 1]); + + self::assertSame([[1], [3], [null]], $matrix1); //* @phpstan-ignore-line + } + + /** + * These 2 tests are contrived. They prove that method + * works as desired, but Excel will actually return + * a CALC error, a result I don't know how to duplicate. + */ + public static function testExtendFirstColumn(): void + { + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + $sheet->setTitle('Products'); + $calculationEngine = Calculation::getInstance($spreadsheet); + $calculationEngine->setInstanceArrayReturnType( + Calculation::RETURN_ARRAY_AS_ARRAY + ); + + $sheet->getCell('D5')->setValue(5); + $sheet->getCell('E5')->setValue(20); + $sheet->fromArray( + [ + [5, 20, 'Apples'], + [10, 20, 'Bananas'], + [5, 20, 'Cherries'], + [5, 40, 'Grapes'], + [25, 50, 'Peaches'], + [30, 60, 'Pears'], + [35, 70, 'Papayas'], + [40, 80, 'Mangos'], + [null, 20, 'Unknown'], + ], + null, + 'K1', + true + ); + $kRows = $sheet->getHighestDataRow('K'); + self::assertSame(8, $kRows); + $lRows = $sheet->getHighestDataRow('L'); + self::assertSame(9, $lRows); + $mRows = $sheet->getHighestDataRow('M'); + self::assertSame(9, $mRows); + $sheet->getCell('A1') + ->setValue( + "=FILTER(Products!M1:M$mRows," + . "(Products!K1:K$kRows=D5)" + . "*(Products!L1:L$lRows=E5))" + ); + + $result = $sheet->getCell('A1')->getCalculatedValue(); + self::assertSame([['Apples'], ['Cherries']], $result); + $spreadsheet->disconnectWorksheets(); + } + + public static function testExtendSecondColumn(): void + { + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + $sheet->setTitle('Products'); + $calculationEngine = Calculation::getInstance($spreadsheet); + $calculationEngine->setInstanceArrayReturnType( + Calculation::RETURN_ARRAY_AS_ARRAY + ); + + $sheet->getCell('D5')->setValue(5); + $sheet->getCell('E5')->setValue(20); + $sheet->fromArray( + [ + [5, 20, 'Apples'], + [10, 20, 'Bananas'], + [5, 20, 'Cherries'], + [5, 40, 'Grapes'], + [25, 50, 'Peaches'], + [30, 60, 'Pears'], + [35, 70, 'Papayas'], + [40, 80, 'Mangos'], + [null, 20, 'Unknown'], + ], + null, + 'K1', + true + ); + $kRows = $sheet->getHighestDataRow('K'); + self::assertSame(8, $kRows); + //$lRows = $sheet->getHighestDataRow('L'); + //self::assertSame(9, $lRows); + $lRows = 2; + $mRows = $sheet->getHighestDataRow('M'); + self::assertSame(9, $mRows); + $sheet->getCell('A1') + ->setValue( + "=FILTER(Products!M1:M$mRows," + . "(Products!K1:K$kRows=D5)" + . "*(Products!L1:L$lRows=E5))" + ); + + $result = $sheet->getCell('A1')->getCalculatedValue(); + self::assertSame([['Apples']], $result); + $spreadsheet->disconnectWorksheets(); + } +}