Skip to content

Commit 3db0b2a

Browse files
oleibmanoleibman
authored
Corrections for HLOOKUP Function (#2330)
See issue #2123. HLOOKUP needs to do some conversions between column numbers and letters which it had not been doing. HLOOKUP tests were performed using direct calls to the function in question rather than in the context of a spreadsheet. This contributed to keeping this error obscured even though there were, in theory, sufficient test cases. The tests are changed to perform in spreadsheet context. For the most part, the test cases are unchanged. One of the expected results was wrong; it has been changed, and a new case added to cover the case it was supposed to be testing. After getting the HLOOKUP tests in order, it turned out that a test using literal arrays which had been succeeding now failed. The array constructed by the literals are considerably different than those constructed using spreadsheet cells; additional code was added to handle this situation.
1 parent 9512f54 commit 3db0b2a

File tree

4 files changed

+124
-43
lines changed

4 files changed

+124
-43
lines changed

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -760,36 +760,6 @@ parameters:
760760
count: 1
761761
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
762762

763-
-
764-
message: "#^Binary operation \"\\-\" between int\\|string and 1 results in an error\\.$#"
765-
count: 1
766-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
767-
768-
-
769-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has no return typehint specified\\.$#"
770-
count: 1
771-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
772-
773-
-
774-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$column with no typehint specified\\.$#"
775-
count: 1
776-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
777-
778-
-
779-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$lookupArray with no typehint specified\\.$#"
780-
count: 1
781-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
782-
783-
-
784-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$lookupValue with no typehint specified\\.$#"
785-
count: 1
786-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
787-
788-
-
789-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$notExactMatch with no typehint specified\\.$#"
790-
count: 1
791-
path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php
792-
793763
-
794764
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\Lookup\\:\\:verifyResultVector\\(\\) has no return typehint specified\\.$#"
795765
count: 1

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

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use PhpOffice\PhpSpreadsheet\Calculation\Exception;
66
use PhpOffice\PhpSpreadsheet\Calculation\Functions;
7+
use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
78
use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
89

910
class HLookup extends LookupBase
@@ -26,6 +27,7 @@ public static function lookup($lookupValue, $lookupArray, $indexNumber, $notExac
2627
$lookupValue = Functions::flattenSingleValue($lookupValue);
2728
$indexNumber = Functions::flattenSingleValue($indexNumber);
2829
$notExactMatch = ($notExactMatch === null) ? true : Functions::flattenSingleValue($notExactMatch);
30+
$lookupArray = self::convertLiteralArray($lookupArray);
2931

3032
try {
3133
$indexNumber = self::validateIndexLookup($lookupArray, $indexNumber);
@@ -46,13 +48,18 @@ public static function lookup($lookupValue, $lookupArray, $indexNumber, $notExac
4648

4749
if ($rowNumber !== null) {
4850
// otherwise return the appropriate value
49-
return $lookupArray[$returnColumn][$rowNumber];
51+
return $lookupArray[$returnColumn][Coordinate::stringFromColumnIndex($rowNumber)];
5052
}
5153

5254
return Functions::NA();
5355
}
5456

55-
private static function hLookupSearch($lookupValue, $lookupArray, $column, $notExactMatch)
57+
/**
58+
* @param mixed $lookupValue The value that you want to match in lookup_array
59+
* @param mixed $column The column to look up
60+
* @param mixed $notExactMatch determines if you are looking for an exact match based on lookup_value
61+
*/
62+
private static function hLookupSearch($lookupValue, array $lookupArray, $column, $notExactMatch): ?int
5663
{
5764
$lookupLower = StringHelper::strToLower($lookupValue);
5865

@@ -74,7 +81,7 @@ private static function hLookupSearch($lookupValue, $lookupArray, $column, $notE
7481
$bothNumeric,
7582
$bothNotNumeric,
7683
$notExactMatch,
77-
$rowKey,
84+
Coordinate::columnIndexFromString($rowKey),
7885
$cellDataLower,
7986
$lookupLower,
8087
$rowNumber
@@ -83,4 +90,27 @@ private static function hLookupSearch($lookupValue, $lookupArray, $column, $notE
8390

8491
return $rowNumber;
8592
}
93+
94+
private static function convertLiteralArray(array $lookupArray): array
95+
{
96+
if (array_key_exists(0, $lookupArray)) {
97+
$lookupArray2 = [];
98+
$row = 0;
99+
foreach ($lookupArray as $arrayVal) {
100+
++$row;
101+
if (!is_array($arrayVal)) {
102+
$arrayVal = [$arrayVal];
103+
}
104+
$arrayVal2 = [];
105+
foreach ($arrayVal as $key2 => $val2) {
106+
$index = Coordinate::stringFromColumnIndex($key2 + 1);
107+
$arrayVal2[$index] = $val2;
108+
}
109+
$lookupArray2[$row] = $arrayVal2;
110+
}
111+
$lookupArray = $lookupArray2;
112+
}
113+
114+
return $lookupArray;
115+
}
86116
}

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

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,91 @@
22

33
namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\LookupRef;
44

5-
use PhpOffice\PhpSpreadsheet\Calculation\Functions;
65
use PhpOffice\PhpSpreadsheet\Calculation\LookupRef;
6+
use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
7+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
78
use PHPUnit\Framework\TestCase;
89

910
class HLookupTest extends TestCase
1011
{
11-
protected function setUp(): void
12-
{
13-
Functions::setCompatibilityMode(Functions::COMPATIBILITY_EXCEL);
14-
}
15-
1612
/**
1713
* @dataProvider providerHLOOKUP
1814
*
1915
* @param mixed $expectedResult
16+
* @param mixed $lookup
17+
* @param mixed $rowIndex
2018
*/
21-
public function testHLOOKUP($expectedResult, ...$args): void
19+
public function testHLOOKUP($expectedResult, $lookup, array $values, $rowIndex, ?bool $rangeLookup = null): void
20+
{
21+
$spreadsheet = new Spreadsheet();
22+
$sheet = $spreadsheet->getActiveSheet();
23+
$maxRow = 0;
24+
$maxCol = 0;
25+
$maxColLetter = 'A';
26+
$row = 0;
27+
foreach ($values as $rowValues) {
28+
++$row;
29+
++$maxRow;
30+
$col = 0;
31+
if (!is_array($rowValues)) {
32+
$rowValues = [$rowValues];
33+
}
34+
foreach ($rowValues as $cellValue) {
35+
++$col;
36+
$colLetter = Coordinate::stringFromColumnIndex($col);
37+
if ($col > $maxCol) {
38+
$maxCol = $col;
39+
$maxColLetter = $colLetter;
40+
}
41+
if ($cellValue !== null) {
42+
$sheet->getCell("$colLetter$row")->setValue($cellValue);
43+
}
44+
}
45+
}
46+
47+
$boolArg = self::parseRangeLookup($rangeLookup);
48+
$sheet->getCell('ZZ8')->setValue($lookup);
49+
$sheet->getCell('ZZ7')->setValue($rowIndex);
50+
$sheet->getCell('ZZ1')->setValue("=HLOOKUP(ZZ8, A1:$maxColLetter$maxRow, ZZ7$boolArg)");
51+
self::assertEquals($expectedResult, $sheet->getCell('ZZ1')->getCalculatedValue());
52+
53+
$spreadsheet->disconnectWorksheets();
54+
}
55+
56+
private static function parseRangeLookup(?bool $rangeLookup): string
2257
{
23-
$result = LookupRef::HLOOKUP(...$args);
24-
self::assertEquals($expectedResult, $result);
58+
if ($rangeLookup === null) {
59+
return '';
60+
}
61+
62+
return $rangeLookup ? ', true' : ', false';
2563
}
2664

2765
public function providerHLOOKUP(): array
2866
{
2967
return require 'tests/data/Calculation/LookupRef/HLOOKUP.php';
3068
}
69+
70+
public function testGrandfathered(): void
71+
{
72+
// Second parameter is supposed to be array of arrays.
73+
// Some old tests called function directly using array of strings;
74+
// ensure these work as before.
75+
$expectedResult = '#REF!';
76+
$result = LookupRef::HLOOKUP(
77+
'Selection column',
78+
['Selection column', 'Value to retrieve'],
79+
5,
80+
false
81+
);
82+
self::assertSame($expectedResult, $result);
83+
$expectedResult = 'Value to retrieve';
84+
$result = LookupRef::HLOOKUP(
85+
'Selection column',
86+
['Selection column', 'Value to retrieve'],
87+
2,
88+
false
89+
);
90+
self::assertSame($expectedResult, $result);
91+
}
3192
}

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ function partsGrid(): array
133133
false,
134134
],
135135
[
136-
'#REF!',
136+
'#N/A',
137137
'B',
138138
[
139139
'Selection column',
@@ -142,6 +142,26 @@ function partsGrid(): array
142142
2,
143143
false,
144144
],
145+
[
146+
'#REF!',
147+
'Selection column',
148+
[
149+
'Selection column',
150+
'Value to retrieve',
151+
],
152+
5,
153+
false,
154+
],
155+
[
156+
'Selection column',
157+
'Selection column',
158+
[
159+
'Selection column',
160+
'Value to retrieve',
161+
],
162+
1,
163+
false,
164+
],
145165
[
146166
0.61,
147167
'Ed',

0 commit comments

Comments
 (0)