Skip to content

Commit 7b38d8c

Browse files
oleibmanoleibman
committed
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.
1 parent 3922d9a commit 7b38d8c

File tree

3 files changed

+137
-9
lines changed

3 files changed

+137
-9
lines changed

‎src/PhpSpreadsheet/Calculation/Calculation.php‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -845,15 +845,15 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i
845845
if (($matrix2Columns < $matrix1Columns) || ($matrix2Rows < $matrix1Rows)) {
846846
if ($matrix2Columns < $matrix1Columns) {
847847
for ($i = 0; $i < $matrix2Rows; ++$i) {
848-
$x = $matrix2[$i][$matrix2Columns - 1];
848+
$x = ($matrix2Columns === 1) ? $matrix2[$i][0] : null;
849849
for ($j = $matrix2Columns; $j < $matrix1Columns; ++$j) {
850850
$matrix2[$i][$j] = $x;
851851
}
852852
}
853853
}
854854
if ($matrix2Rows < $matrix1Rows) {
855-
$x = $matrix2[$matrix2Rows - 1];
856-
for ($i = 0; $i < $matrix1Rows; ++$i) {
855+
$x = ($matrix2Rows === 1) ? $matrix2[0] : array_fill(0, $matrix2Columns, null);
856+
for ($i = $matrix2Rows; $i < $matrix1Rows; ++$i) {
857857
$matrix2[$i] = $x;
858858
}
859859
}
@@ -862,15 +862,15 @@ private static function resizeMatricesExtend(array &$matrix1, array &$matrix2, i
862862
if (($matrix1Columns < $matrix2Columns) || ($matrix1Rows < $matrix2Rows)) {
863863
if ($matrix1Columns < $matrix2Columns) {
864864
for ($i = 0; $i < $matrix1Rows; ++$i) {
865-
$x = $matrix1[$i][$matrix1Columns - 1];
865+
$x = ($matrix1Columns === 1) ? $matrix1[$i][0] : null;
866866
for ($j = $matrix1Columns; $j < $matrix2Columns; ++$j) {
867867
$matrix1[$i][$j] = $x;
868868
}
869869
}
870870
}
871871
if ($matrix1Rows < $matrix2Rows) {
872-
$x = $matrix1[$matrix1Rows - 1];
873-
for ($i = 0; $i < $matrix2Rows; ++$i) {
872+
$x = ($matrix1Rows === 1) ? $matrix1[0] : array_fill(0, $matrix1Columns, null);
873+
for ($i = $matrix1Rows; $i < $matrix2Rows; ++$i) {
874874
$matrix1[$i] = $x;
875875
}
876876
}
@@ -2334,14 +2334,14 @@ private function executeNumericBinaryOperation(mixed $operand1, mixed $operand2,
23342334

23352335
for ($row = 0; $row < $rows; ++$row) {
23362336
for ($column = 0; $column < $columns; ++$column) {
2337-
if ($operand1[$row][$column] === null) {
2337+
if (($operand1[$row][$column] ?? null) === null) {
23382338
$operand1[$row][$column] = 0;
23392339
} elseif (!self::isNumericOrBool($operand1[$row][$column])) {
23402340
$operand1[$row][$column] = self::makeError($operand1[$row][$column]);
23412341

23422342
continue;
23432343
}
2344-
if ($operand2[$row][$column] === null) {
2344+
if (($operand2[$row][$column] ?? null) === null) {
23452345
$operand2[$row][$column] = 0;
23462346
} elseif (!self::isNumericOrBool($operand2[$row][$column])) {
23472347
$operand1[$row][$column] = self::makeError($operand2[$row][$column]);

‎src/PhpSpreadsheet/Calculation/LookupRef/Filter.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ private static function filterByRow(array $lookupArray, array $matchArray): arra
4545

4646
return array_filter(
4747
array_values($lookupArray),
48-
fn ($index): bool => (bool) $matchArray[$index],
48+
fn ($index): bool => (bool) ($matchArray[$index] ?? null),
4949
ARRAY_FILTER_USE_KEY
5050
);
5151
}

‎tests/PhpSpreadsheetTests/Calculation/Issue4451Test.php‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOffice\PhpSpreadsheetTests\Calculation;
6+
7+
use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
8+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
9+
use PHPUnit\Framework\TestCase;
10+
use ReflectionMethod;
11+
12+
class Issue4451Test extends TestCase
13+
{
14+
public static function testReflect(): void
15+
{
16+
// Sample matrices to test with
17+
$matrix1 = [[1], [3]];
18+
$matrix2 = [[5], [8], [11]];
19+
20+
// Use reflection to make the protected method accessible
21+
$calculation = new Calculation();
22+
$reflectionMethod = new ReflectionMethod(Calculation::class, 'resizeMatricesExtend');
23+
$reflectionMethod->setAccessible(true);
24+
25+
// Call the method using reflection
26+
$reflectionMethod->invokeArgs($calculation, [&$matrix1, &$matrix2, count($matrix1), 1, count($matrix2), 1]);
27+
28+
self::assertSame([[1], [3], [null]], $matrix1); //* @phpstan-ignore-line
29+
}
30+
31+
/**
32+
* These 2 tests are contrived. They prove that method
33+
* works as desired, but Excel will actually return
34+
* a CALC error, a result I don't know how to duplicate.
35+
*/
36+
public static function testExtendFirstColumn(): void
37+
{
38+
$spreadsheet = new Spreadsheet();
39+
$sheet = $spreadsheet->getActiveSheet();
40+
$sheet->setTitle('Products');
41+
$calculationEngine = Calculation::getInstance($spreadsheet);
42+
$calculationEngine->setInstanceArrayReturnType(
43+
Calculation::RETURN_ARRAY_AS_ARRAY
44+
);
45+
46+
$sheet->getCell('D5')->setValue(5);
47+
$sheet->getCell('E5')->setValue(20);
48+
$sheet->fromArray(
49+
[
50+
[5, 20, 'Apples'],
51+
[10, 20, 'Bananas'],
52+
[5, 20, 'Cherries'],
53+
[5, 40, 'Grapes'],
54+
[25, 50, 'Peaches'],
55+
[30, 60, 'Pears'],
56+
[35, 70, 'Papayas'],
57+
[40, 80, 'Mangos'],
58+
[null, 20, 'Unknown'],
59+
],
60+
null,
61+
'K1',
62+
true
63+
);
64+
$kRows = $sheet->getHighestDataRow('K');
65+
self::assertSame(8, $kRows);
66+
$lRows = $sheet->getHighestDataRow('L');
67+
self::assertSame(9, $lRows);
68+
$mRows = $sheet->getHighestDataRow('M');
69+
self::assertSame(9, $mRows);
70+
$sheet->getCell('A1')
71+
->setValue(
72+
"=FILTER(Products!M1:M$mRows,"
73+
. "(Products!K1:K$kRows=D5)"
74+
. "*(Products!L1:L$lRows=E5))"
75+
);
76+
77+
$result = $sheet->getCell('A1')->getCalculatedValue();
78+
self::assertSame([['Apples'], ['Cherries']], $result);
79+
$spreadsheet->disconnectWorksheets();
80+
}
81+
82+
public static function testExtendSecondColumn(): void
83+
{
84+
$spreadsheet = new Spreadsheet();
85+
$sheet = $spreadsheet->getActiveSheet();
86+
$sheet->setTitle('Products');
87+
$calculationEngine = Calculation::getInstance($spreadsheet);
88+
$calculationEngine->setInstanceArrayReturnType(
89+
Calculation::RETURN_ARRAY_AS_ARRAY
90+
);
91+
92+
$sheet->getCell('D5')->setValue(5);
93+
$sheet->getCell('E5')->setValue(20);
94+
$sheet->fromArray(
95+
[
96+
[5, 20, 'Apples'],
97+
[10, 20, 'Bananas'],
98+
[5, 20, 'Cherries'],
99+
[5, 40, 'Grapes'],
100+
[25, 50, 'Peaches'],
101+
[30, 60, 'Pears'],
102+
[35, 70, 'Papayas'],
103+
[40, 80, 'Mangos'],
104+
[null, 20, 'Unknown'],
105+
],
106+
null,
107+
'K1',
108+
true
109+
);
110+
$kRows = $sheet->getHighestDataRow('K');
111+
self::assertSame(8, $kRows);
112+
//$lRows = $sheet->getHighestDataRow('L');
113+
//self::assertSame(9, $lRows);
114+
$lRows = 2;
115+
$mRows = $sheet->getHighestDataRow('M');
116+
self::assertSame(9, $mRows);
117+
$sheet->getCell('A1')
118+
->setValue(
119+
"=FILTER(Products!M1:M$mRows,"
120+
. "(Products!K1:K$kRows=D5)"
121+
. "*(Products!L1:L$lRows=E5))"
122+
);
123+
124+
$result = $sheet->getCell('A1')->getCalculatedValue();
125+
self::assertSame([['Apples']], $result);
126+
$spreadsheet->disconnectWorksheets();
127+
}
128+
}

0 commit comments

Comments
 (0)