Skip to content

Commit 4bf4278

Browse files
oleibmanoleibman
authored
VLOOKUP Breaks When Array Contains Null Cells (#2939)
Fix #2934. Null is passed to StringHelper::strtolower which expects string. Same problem appears to be applicable to HLOOKUP. I noted the following problem in the code, but will document it here as well. Excel's results are not consistent when a non-numeric string is passed as the third parameter. For example, if cell Z1 contains `xyz`, Excel will return a REF error for function `VLOOKUP(whatever,whatever,Z1)`, but it returns a VALUE error for function `VLOOKUP(whatever,whatever,"xyz")`. I don't think PhpSpreadsheet can match both behaviors. For now, it will return VALUE for both, with similar results for other errors. While studying the returned errors, I realized there is something that needs to be deprecated. `ExcelError::$errorCodes` is a public static array. This means that a user can change its value, which should not be allowed. It is replaced by a constant. Since the original is public, I think it needs to stay, but with a deprecation notice; users can reference and change it, but it will be unused in the rest of the code. I suppose this might be considered a break in functionality (that should not have been allowed in the first place).
1 parent a062521 commit 4bf4278

File tree

8 files changed

+106
-31
lines changed

8 files changed

+106
-31
lines changed

‎src/PhpSpreadsheet/Calculation/Information/ErrorValue.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public static function isError($value = '')
4747
return false;
4848
}
4949

50-
return in_array($value, ExcelError::$errorCodes, true);
50+
return in_array($value, ExcelError::ERROR_CODES, true);
5151
}
5252

5353
/**

‎src/PhpSpreadsheet/Calculation/Information/ExcelError.php

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ class ExcelError
1313
*
1414
* @var array<string, string>
1515
*/
16-
public static $errorCodes = [
16+
public const ERROR_CODES = [
1717
'null' => '#NULL!', // 1
1818
'divisionbyzero' => '#DIV/0!', // 2
1919
'value' => '#VALUE!', // 3
@@ -30,12 +30,23 @@ class ExcelError
3030
'calculation' => '#CALC!', //14
3131
];
3232

33+
/**
34+
* List of error codes. Replaced by constant;
35+
* previously it was public and updateable, allowing
36+
* user to make inappropriate alterations.
37+
*
38+
* @deprecated 1.25.0 Use ERROR_CODES constant instead.
39+
*
40+
* @var array<string, string>
41+
*/
42+
public static $errorCodes = self::ERROR_CODES;
43+
3344
/**
3445
* @param mixed $value
3546
*/
3647
public static function throwError($value): string
3748
{
38-
return in_array($value, self::$errorCodes, true) ? $value : self::$errorCodes['value'];
49+
return in_array($value, self::ERROR_CODES, true) ? $value : self::ERROR_CODES['value'];
3950
}
4051

4152
/**
@@ -52,7 +63,7 @@ public static function type($value = '')
5263
}
5364

5465
$i = 1;
55-
foreach (self::$errorCodes as $errorCode) {
66+
foreach (self::ERROR_CODES as $errorCode) {
5667
if ($value === $errorCode) {
5768
return $i;
5869
}
@@ -71,7 +82,7 @@ public static function type($value = '')
7182
*/
7283
public static function null(): string
7384
{
74-
return self::$errorCodes['null'];
85+
return self::ERROR_CODES['null'];
7586
}
7687

7788
/**
@@ -83,7 +94,7 @@ public static function null(): string
8394
*/
8495
public static function NAN(): string
8596
{
86-
return self::$errorCodes['num'];
97+
return self::ERROR_CODES['num'];
8798
}
8899

89100
/**
@@ -95,7 +106,7 @@ public static function NAN(): string
95106
*/
96107
public static function REF(): string
97108
{
98-
return self::$errorCodes['reference'];
109+
return self::ERROR_CODES['reference'];
99110
}
100111

101112
/**
@@ -111,7 +122,7 @@ public static function REF(): string
111122
*/
112123
public static function NA(): string
113124
{
114-
return self::$errorCodes['na'];
125+
return self::ERROR_CODES['na'];
115126
}
116127

117128
/**
@@ -123,7 +134,7 @@ public static function NA(): string
123134
*/
124135
public static function VALUE(): string
125136
{
126-
return self::$errorCodes['value'];
137+
return self::ERROR_CODES['value'];
127138
}
128139

129140
/**
@@ -135,7 +146,7 @@ public static function VALUE(): string
135146
*/
136147
public static function NAME(): string
137148
{
138-
return self::$errorCodes['name'];
149+
return self::ERROR_CODES['name'];
139150
}
140151

141152
/**
@@ -145,7 +156,7 @@ public static function NAME(): string
145156
*/
146157
public static function DIV0(): string
147158
{
148-
return self::$errorCodes['divisionbyzero'];
159+
return self::ERROR_CODES['divisionbyzero'];
149160
}
150161

151162
/**
@@ -155,6 +166,6 @@ public static function DIV0(): string
155166
*/
156167
public static function CALC(): string
157168
{
158-
return self::$errorCodes['calculation'];
169+
return self::ERROR_CODES['calculation'];
159170
}
160171
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public static function lookup($lookupValue, $lookupArray, $indexNumber, $notExac
6666
*/
6767
private static function hLookupSearch($lookupValue, array $lookupArray, $column, bool $notExactMatch): ?int
6868
{
69-
$lookupLower = StringHelper::strToLower($lookupValue);
69+
$lookupLower = StringHelper::strToLower((string) $lookupValue);
7070

7171
$rowNumber = null;
7272
foreach ($lookupArray[$column] as $rowKey => $rowData) {

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,16 @@ protected static function validateLookupArray($lookup_array): void
1919

2020
protected static function validateIndexLookup(array $lookup_array, $index_number): int
2121
{
22-
// index_number must be a number greater than or equal to 1
23-
if (!is_numeric($index_number) || $index_number < 1) {
22+
// index_number must be a number greater than or equal to 1.
23+
// Excel results are inconsistent when index is non-numeric.
24+
// VLOOKUP(whatever, whatever, SQRT(-1)) yields NUM error, but
25+
// VLOOKUP(whatever, whatever, cellref) yields REF error
26+
// when cellref is '=SQRT(-1)'. So just try our best here.
27+
// Similar results if string (literal yields VALUE, cellRef REF).
28+
if (!is_numeric($index_number)) {
29+
throw new Exception(ExcelError::throwError($index_number));
30+
}
31+
if ($index_number < 1) {
2432
throw new Exception(ExcelError::VALUE());
2533
}
2634

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ private static function vlookupSort(array $a, array $b): int
6868
{
6969
reset($a);
7070
$firstColumn = key($a);
71-
$aLower = StringHelper::strToLower($a[$firstColumn]);
72-
$bLower = StringHelper::strToLower($b[$firstColumn]);
71+
$aLower = StringHelper::strToLower((string) $a[$firstColumn]);
72+
$bLower = StringHelper::strToLower((string) $b[$firstColumn]);
7373

7474
if ($aLower == $bLower) {
7575
return 0;
@@ -84,7 +84,7 @@ private static function vlookupSort(array $a, array $b): int
8484
*/
8585
private static function vLookupSearch($lookupValue, array $lookupArray, $column, bool $notExactMatch): ?int
8686
{
87-
$lookupLower = StringHelper::strToLower($lookupValue);
87+
$lookupLower = StringHelper::strToLower((string) $lookupValue);
8888

8989
$rowNumber = null;
9090
foreach ($lookupArray as $rowKey => $rowData) {

‎tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/VLookupTest.php

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,26 +3,42 @@
33
namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\LookupRef;
44

55
use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
6-
use PhpOffice\PhpSpreadsheet\Calculation\Functions;
7-
use PhpOffice\PhpSpreadsheet\Calculation\LookupRef;
6+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
87
use PHPUnit\Framework\TestCase;
98

109
class VLookupTest extends TestCase
1110
{
12-
protected function setUp(): void
13-
{
14-
Functions::setCompatibilityMode(Functions::COMPATIBILITY_EXCEL);
15-
}
16-
1711
/**
1812
* @dataProvider providerVLOOKUP
1913
*
2014
* @param mixed $expectedResult
15+
* @param mixed $value
16+
* @param mixed $table
17+
* @param mixed $index
2118
*/
22-
public function testVLOOKUP($expectedResult, ...$args): void
19+
public function testVLOOKUP($expectedResult, $value, $table, $index, ?bool $lookup = null): void
2320
{
24-
$result = LookupRef::VLOOKUP(...$args);
21+
$spreadsheet = new Spreadsheet();
22+
$sheet = $spreadsheet->getActiveSheet();
23+
if (is_array($table)) {
24+
$sheet->fromArray($table);
25+
$dimension = $sheet->calculateWorksheetDimension();
26+
} else {
27+
$sheet->getCell('A1')->setValue($table);
28+
$dimension = 'A1';
29+
}
30+
if ($lookup === null) {
31+
$lastarg = '';
32+
} else {
33+
$lastarg = $lookup ? ',TRUE' : ',FALSE';
34+
}
35+
$sheet->getCell('Z98')->setValue($value);
36+
$sheet->getCell('Z97')->setValue($index);
37+
38+
$sheet->getCell('Z99')->setValue("=VLOOKUP(Z98,$dimension,Z97$lastarg)");
39+
$result = $sheet->getCell('Z99')->getCalculatedValue();
2540
self::assertEquals($expectedResult, $result);
41+
$spreadsheet->disconnectWorksheets();
2642
}
2743

2844
public function providerVLOOKUP(): array

‎tests/data/Calculation/LookupRef/HLOOKUP.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,4 +186,14 @@ function partsGrid(): array
186186
3,
187187
true,
188188
],
189+
'issue2934' => [
190+
'Red',
191+
102,
192+
[
193+
[null, 102],
194+
[null, 'Red'],
195+
],
196+
2,
197+
false,
198+
],
189199
];

‎tests/data/Calculation/LookupRef/VLOOKUP.php

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ function densityGrid(): array
9898
['10y1', 7.0],
9999
['10y2', 10.0],
100100
],
101-
'NaN',
101+
-5,
102102
],
103103
[
104104
'#REF!',
@@ -111,9 +111,9 @@ function densityGrid(): array
111111
'#REF!',
112112
'10y2',
113113
[
114-
2.0,
115-
7.0,
116-
10.0,
114+
[2.0],
115+
[7.0],
116+
[10.0],
117117
],
118118
2.0,
119119
],
@@ -163,4 +163,34 @@ function densityGrid(): array
163163
3,
164164
null,
165165
],
166+
'issue2934' => [
167+
'Red',
168+
102,
169+
[
170+
[null, null],
171+
[102, 'Red'],
172+
],
173+
2,
174+
false,
175+
],
176+
'string supplied as index' => [
177+
'#VALUE!',
178+
102,
179+
[
180+
[null, null],
181+
[102, 'Red'],
182+
],
183+
'xyz',
184+
false,
185+
],
186+
'num error propagated' => [
187+
'#NUM!',
188+
102,
189+
[
190+
[null, null],
191+
[102, 'Red'],
192+
],
193+
'=SQRT(-1)',
194+
false,
195+
],
166196
];

0 commit comments

Comments
 (0)