From b088cf6e9f4d99da996ea02d3430cbf8f5b260eb Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Mon, 11 Oct 2021 19:00:23 -0700 Subject: [PATCH] Corrections for HLOOKUP Function 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 41cd866dac..e28906177e 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 96cfd7ab7b..7db2780440 100644 --- a/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php +++ b/src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php @@ -4,6 +4,7 @@ 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 @@ public static function lookup($lookupValue, $lookupArray, $indexNumber, $notExac $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 @@ public static function lookup($lookupValue, $lookupArray, $indexNumber, $notExac 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 @@ private static function hLookupSearch($lookupValue, $lookupArray, $column, $notE $bothNumeric, $bothNotNumeric, $notExactMatch, - $rowKey, + Coordinate::columnIndexFromString($rowKey), $cellDataLower, $lookupLower, $rowNumber @@ -83,4 +90,27 @@ private static function hLookupSearch($lookupValue, $lookupArray, $column, $notE 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 7484c47ba2..7dc5b29c58 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 + { + $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 { - $result = LookupRef::HLOOKUP(...$args); - self::assertEquals($expectedResult, $result); + 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 266017210a..078ed00709 100644 --- a/tests/data/Calculation/LookupRef/HLOOKUP.php +++ b/tests/data/Calculation/LookupRef/HLOOKUP.php @@ -133,7 +133,7 @@ function partsGrid(): array false, ], [ - '#REF!', + '#N/A', 'B', [ 'Selection column', @@ -142,6 +142,26 @@ function partsGrid(): array 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',