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 1/2] 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 cca6ca4464..05ebe0c1bb 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -845,15 +845,15 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i 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 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i 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 @@ private function executeNumericBinaryOperation(mixed $operand1, mixed $operand2, 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 daedcd0b9f..1a2553beb6 100644 --- a/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php +++ b/src/PhpSpreadsheet/Calculation/LookupRef/Filter.php @@ -45,7 +45,7 @@ private static function filterByRow(array $lookupArray, array $matchArray): arra 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 0000000000..3b9107d18f --- /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(); + } +} From 7efb118cbd73edb8a1d4b5608dc3532266741a90 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 4 Jul 2025 10:41:20 -0700 Subject: [PATCH 2/2] Phpstan Catch-up --- CHANGELOG.md | 3 ++- src/PhpSpreadsheet/Calculation/Calculation.php | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b8534ef61..41e375f64a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,8 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Fixed -- Nothing yet. +- Micro-optimization in getSheetByName. [PR #4499](https://github.com/PHPOffice/PhpSpreadsheet/pull/4499) +- Bug in resizeMatricesExtend. [Issue #4451](https://github.com/PHPOffice/PhpSpreadsheet/issues/4451) [PR #4474](https://github.com/PHPOffice/PhpSpreadsheet/pull/4474) ## 2025-06-22 - 4.4.0 diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 32f323817a..72105814a3 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -851,6 +851,7 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i if (($matrix2Columns < $matrix1Columns) || ($matrix2Rows < $matrix1Rows)) { if ($matrix2Columns < $matrix1Columns) { for ($i = 0; $i < $matrix2Rows; ++$i) { + /** @var mixed[][] $matrix2 */ $x = ($matrix2Columns === 1) ? $matrix2[$i][0] : null; for ($j = $matrix2Columns; $j < $matrix1Columns; ++$j) { $matrix2[$i][$j] = $x; @@ -868,6 +869,7 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i if (($matrix1Columns < $matrix2Columns) || ($matrix1Rows < $matrix2Rows)) { if ($matrix1Columns < $matrix2Columns) { for ($i = 0; $i < $matrix1Rows; ++$i) { + /** @var mixed[][] $matrix1 */ $x = ($matrix1Columns === 1) ? $matrix1[$i][0] : null; for ($j = $matrix1Columns; $j < $matrix2Columns; ++$j) { $matrix1[$i][$j] = $x; @@ -2372,6 +2374,7 @@ private function executeNumericBinaryOperation(mixed $operand1, mixed $operand2, for ($row = 0; $row < $rows; ++$row) { for ($column = 0; $column < $columns; ++$column) { + /** @var mixed[][] $operand1 */ if (($operand1[$row][$column] ?? null) === null) { $operand1[$row][$column] = 0; } elseif (!self::isNumericOrBool($operand1[$row][$column])) { @@ -2379,6 +2382,7 @@ private function executeNumericBinaryOperation(mixed $operand1, mixed $operand2, continue; } + /** @var mixed[][] $operand2 */ if (($operand2[$row][$column] ?? null) === null) { $operand2[$row][$column] = 0; } elseif (!self::isNumericOrBool($operand2[$row][$column])) {