From 3db0b2a2dead9b997eacf1230dee3f7fc55c516a Mon Sep 17 00:00:00 2001 From: oleibman Date: Sat, 23 Oct 2021 17:59:42 -0700 Subject: [PATCH] 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. --- phpstan-baseline.neon | 30 ------- .../Calculation/LookupRef/HLookup.php | 36 ++++++++- .../Functions/LookupRef/HLookupTest.php | 79 ++++++++++++++++--- tests/data/Calculation/LookupRef/HLOOKUP.php | 22 +++++- 4 files changed, 124 insertions(+), 43 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 41cd866d..e2890617 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -760,36 +760,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php - - - message: "#^Binary operation \"\\-\" between int\\|string and 1 results in an error\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has no return typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$column with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$lookupArray with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$lookupValue with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\HLookup\\:\\:hLookupSearch\\(\\) has parameter \\$notExactMatch with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\Lookup\\:\\:verifyResultVector\\(\\) has no return typehint specified\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php b/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php index 96cfd7ab..7db27804 100644 --- a/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php +++ b/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php @@ -4,6 +4,7 @@ namespace PhpOffice\PhpSpreadsheet\Calculation\LookupRef; use PhpOffice\PhpSpreadsheet\Calculation\Exception; use PhpOffice\PhpSpreadsheet\Calculation\Functions; +use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Shared\StringHelper; class HLookup extends LookupBase @@ -26,6 +27,7 @@ class HLookup extends LookupBase $lookupValue = Functions::flattenSingleValue($lookupValue); $indexNumber = Functions::flattenSingleValue($indexNumber); $notExactMatch = ($notExactMatch === null) ? true : Functions::flattenSingleValue($notExactMatch); + $lookupArray = self::convertLiteralArray($lookupArray); try { $indexNumber = self::validateIndexLookup($lookupArray, $indexNumber); @@ -46,13 +48,18 @@ class HLookup extends LookupBase if ($rowNumber !== null) { // otherwise return the appropriate value - return $lookupArray[$returnColumn][$rowNumber]; + return $lookupArray[$returnColumn][Coordinate::stringFromColumnIndex($rowNumber)]; } return Functions::NA(); } - private static function hLookupSearch($lookupValue, $lookupArray, $column, $notExactMatch) + /** + * @param mixed $lookupValue The value that you want to match in lookup_array + * @param mixed $column The column to look up + * @param mixed $notExactMatch determines if you are looking for an exact match based on lookup_value + */ + private static function hLookupSearch($lookupValue, array $lookupArray, $column, $notExactMatch): ?int { $lookupLower = StringHelper::strToLower($lookupValue); @@ -74,7 +81,7 @@ class HLookup extends LookupBase $bothNumeric, $bothNotNumeric, $notExactMatch, - $rowKey, + Coordinate::columnIndexFromString($rowKey), $cellDataLower, $lookupLower, $rowNumber @@ -83,4 +90,27 @@ class HLookup extends LookupBase return $rowNumber; } + + private static function convertLiteralArray(array $lookupArray): array + { + if (array_key_exists(0, $lookupArray)) { + $lookupArray2 = []; + $row = 0; + foreach ($lookupArray as $arrayVal) { + ++$row; + if (!is_array($arrayVal)) { + $arrayVal = [$arrayVal]; + } + $arrayVal2 = []; + foreach ($arrayVal as $key2 => $val2) { + $index = Coordinate::stringFromColumnIndex($key2 + 1); + $arrayVal2[$index] = $val2; + } + $lookupArray2[$row] = $arrayVal2; + } + $lookupArray = $lookupArray2; + } + + return $lookupArray; + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/HLookupTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/HLookupTest.php index 7484c47b..7dc5b29c 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/HLookupTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/HLookupTest.php @@ -2,30 +2,91 @@ namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\LookupRef; -use PhpOffice\PhpSpreadsheet\Calculation\Functions; use PhpOffice\PhpSpreadsheet\Calculation\LookupRef; +use PhpOffice\PhpSpreadsheet\Cell\Coordinate; +use PhpOffice\PhpSpreadsheet\Spreadsheet; use PHPUnit\Framework\TestCase; class HLookupTest extends TestCase { - protected function setUp(): void - { - Functions::setCompatibilityMode(Functions::COMPATIBILITY_EXCEL); - } - /** * @dataProvider providerHLOOKUP * * @param mixed $expectedResult + * @param mixed $lookup + * @param mixed $rowIndex */ - public function testHLOOKUP($expectedResult, ...$args): void + public function testHLOOKUP($expectedResult, $lookup, array $values, $rowIndex, ?bool $rangeLookup = null): void { - $result = LookupRef::HLOOKUP(...$args); - self::assertEquals($expectedResult, $result); + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + $maxRow = 0; + $maxCol = 0; + $maxColLetter = 'A'; + $row = 0; + foreach ($values as $rowValues) { + ++$row; + ++$maxRow; + $col = 0; + if (!is_array($rowValues)) { + $rowValues = [$rowValues]; + } + foreach ($rowValues as $cellValue) { + ++$col; + $colLetter = Coordinate::stringFromColumnIndex($col); + if ($col > $maxCol) { + $maxCol = $col; + $maxColLetter = $colLetter; + } + if ($cellValue !== null) { + $sheet->getCell("$colLetter$row")->setValue($cellValue); + } + } + } + + $boolArg = self::parseRangeLookup($rangeLookup); + $sheet->getCell('ZZ8')->setValue($lookup); + $sheet->getCell('ZZ7')->setValue($rowIndex); + $sheet->getCell('ZZ1')->setValue("=HLOOKUP(ZZ8, A1:$maxColLetter$maxRow, ZZ7$boolArg)"); + self::assertEquals($expectedResult, $sheet->getCell('ZZ1')->getCalculatedValue()); + + $spreadsheet->disconnectWorksheets(); + } + + private static function parseRangeLookup(?bool $rangeLookup): string + { + if ($rangeLookup === null) { + return ''; + } + + return $rangeLookup ? ', true' : ', false'; } public function providerHLOOKUP(): array { return require 'tests/data/Calculation/LookupRef/HLOOKUP.php'; } + + public function testGrandfathered(): void + { + // Second parameter is supposed to be array of arrays. + // Some old tests called function directly using array of strings; + // ensure these work as before. + $expectedResult = '#REF!'; + $result = LookupRef::HLOOKUP( + 'Selection column', + ['Selection column', 'Value to retrieve'], + 5, + false + ); + self::assertSame($expectedResult, $result); + $expectedResult = 'Value to retrieve'; + $result = LookupRef::HLOOKUP( + 'Selection column', + ['Selection column', 'Value to retrieve'], + 2, + false + ); + self::assertSame($expectedResult, $result); + } } diff --git a/tests/data/Calculation/LookupRef/HLOOKUP.php b/tests/data/Calculation/LookupRef/HLOOKUP.php index 26601721..078ed007 100644 --- a/tests/data/Calculation/LookupRef/HLOOKUP.php +++ b/tests/data/Calculation/LookupRef/HLOOKUP.php @@ -133,7 +133,7 @@ return [ false, ], [ - '#REF!', + '#N/A', 'B', [ 'Selection column', @@ -142,6 +142,26 @@ return [ 2, false, ], + [ + '#REF!', + 'Selection column', + [ + 'Selection column', + 'Value to retrieve', + ], + 5, + false, + ], + [ + 'Selection column', + 'Selection column', + [ + 'Selection column', + 'Value to retrieve', + ], + 1, + false, + ], [ 0.61, 'Ed',