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).

src/PhpSpreadsheet/Calculation/Information/ErrorValue.php

@@ -47,7 +47,7 @@ class ErrorValue
             return false;
         }
 
-        return in_array($value, ExcelError::$errorCodes, true);
+        return in_array($value, ExcelError::ERROR_CODES, true);
     }
 
     /**

src/PhpSpreadsheet/Calculation/Information/ExcelError.php

@@ -13,7 +13,7 @@ class ExcelError
      *
      * @var array<string, string>
      */
-    public static $errorCodes = [
+    public const ERROR_CODES = [
         'null' => '#NULL!', // 1
         'divisionbyzero' => '#DIV/0!', // 2
         'value' => '#VALUE!', // 3
@@ -30,12 +30,23 @@ class ExcelError
         'calculation' => '#CALC!', //14
     ];
 
+    /**
+     * List of error codes. Replaced by constant;
+     * previously it was public and updateable, allowing
+     * user to make inappropriate alterations.
+     *
+     * @deprecated 1.25.0 Use ERROR_CODES constant instead.
+     *
+     * @var array<string, string>
+     */
+    public static $errorCodes = self::ERROR_CODES;
+
     /**
      * @param mixed $value
      */
     public static function throwError($value): string
     {
-        return in_array($value, self::$errorCodes, true) ? $value : self::$errorCodes['value'];
+        return in_array($value, self::ERROR_CODES, true) ? $value : self::ERROR_CODES['value'];
     }
 
     /**
@@ -52,7 +63,7 @@ class ExcelError
         }
 
         $i = 1;
-        foreach (self::$errorCodes as $errorCode) {
+        foreach (self::ERROR_CODES as $errorCode) {
             if ($value === $errorCode) {
                 return $i;
             }
@@ -71,7 +82,7 @@ class ExcelError
      */
     public static function null(): string
     {
-        return self::$errorCodes['null'];
+        return self::ERROR_CODES['null'];
     }
 
     /**
@@ -83,7 +94,7 @@ class ExcelError
      */
     public static function NAN(): string
     {
-        return self::$errorCodes['num'];
+        return self::ERROR_CODES['num'];
     }
 
     /**
@@ -95,7 +106,7 @@ class ExcelError
      */
     public static function REF(): string
     {
-        return self::$errorCodes['reference'];
+        return self::ERROR_CODES['reference'];
     }
 
     /**
@@ -111,7 +122,7 @@ class ExcelError
      */
     public static function NA(): string
     {
-        return self::$errorCodes['na'];
+        return self::ERROR_CODES['na'];
     }
 
     /**
@@ -123,7 +134,7 @@ class ExcelError
      */
     public static function VALUE(): string
     {
-        return self::$errorCodes['value'];
+        return self::ERROR_CODES['value'];
     }
 
     /**
@@ -135,7 +146,7 @@ class ExcelError
      */
     public static function NAME(): string
     {
-        return self::$errorCodes['name'];
+        return self::ERROR_CODES['name'];
     }
 
     /**
@@ -145,7 +156,7 @@ class ExcelError
      */
     public static function DIV0(): string
     {
-        return self::$errorCodes['divisionbyzero'];
+        return self::ERROR_CODES['divisionbyzero'];
     }
 
     /**
@@ -155,6 +166,6 @@ class ExcelError
      */
     public static function CALC(): string
     {
-        return self::$errorCodes['calculation'];
+        return self::ERROR_CODES['calculation'];
     }
 }

src/PhpSpreadsheet/Calculation/LookupRef/HLookup.php

@@ -66,7 +66,7 @@ class HLookup extends LookupBase
      */
     private static function hLookupSearch($lookupValue, array $lookupArray, $column, bool $notExactMatch): ?int
     {
-        $lookupLower = StringHelper::strToLower($lookupValue);
+        $lookupLower = StringHelper::strToLower((string) $lookupValue);
 
         $rowNumber = null;
         foreach ($lookupArray[$column] as $rowKey => $rowData) {

src/PhpSpreadsheet/Calculation/LookupRef/LookupBase.php

@@ -19,8 +19,16 @@ abstract class LookupBase
 
     protected static function validateIndexLookup(array $lookup_array, $index_number): int
     {
-        // index_number must be a number greater than or equal to 1
+        // index_number must be a number greater than or equal to 1.
-        if (!is_numeric($index_number) || $index_number < 1) {
+        // Excel results are inconsistent when index is non-numeric.
+        // VLOOKUP(whatever, whatever, SQRT(-1)) yields NUM error, but
+        // VLOOKUP(whatever, whatever, cellref) yields REF error
+        //   when cellref is '=SQRT(-1)'. So just try our best here.
+        // Similar results if string (literal yields VALUE, cellRef REF).
+        if (!is_numeric($index_number)) {
+            throw new Exception(ExcelError::throwError($index_number));
+        }
+        if ($index_number < 1) {
             throw new Exception(ExcelError::VALUE());
         }
 

src/PhpSpreadsheet/Calculation/LookupRef/VLookup.php

@@ -68,8 +68,8 @@ class VLookup extends LookupBase
     {
         reset($a);
         $firstColumn = key($a);
-        $aLower = StringHelper::strToLower($a[$firstColumn]);
+        $aLower = StringHelper::strToLower((string) $a[$firstColumn]);
-        $bLower = StringHelper::strToLower($b[$firstColumn]);
+        $bLower = StringHelper::strToLower((string) $b[$firstColumn]);
 
         if ($aLower == $bLower) {
             return 0;
@@ -84,7 +84,7 @@ class VLookup extends LookupBase
      */
     private static function vLookupSearch($lookupValue, array $lookupArray, $column, bool $notExactMatch): ?int
     {
-        $lookupLower = StringHelper::strToLower($lookupValue);
+        $lookupLower = StringHelper::strToLower((string) $lookupValue);
 
         $rowNumber = null;
         foreach ($lookupArray as $rowKey => $rowData) {

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

@@ -3,26 +3,42 @@
 namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\LookupRef;
 
 use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
-use PhpOffice\PhpSpreadsheet\Calculation\Functions;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
-use PhpOffice\PhpSpreadsheet\Calculation\LookupRef;
 use PHPUnit\Framework\TestCase;
 
 class VLookupTest extends TestCase
 {
-    protected function setUp(): void
-    {
-        Functions::setCompatibilityMode(Functions::COMPATIBILITY_EXCEL);
-    }
-
     /**
      * @dataProvider providerVLOOKUP
      *
      * @param mixed $expectedResult
+     * @param mixed $value
+     * @param mixed $table
+     * @param mixed $index
      */
-    public function testVLOOKUP($expectedResult, ...$args): void
+    public function testVLOOKUP($expectedResult, $value, $table, $index, ?bool $lookup = null): void
     {
-        $result = LookupRef::VLOOKUP(...$args);
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        if (is_array($table)) {
+            $sheet->fromArray($table);
+            $dimension = $sheet->calculateWorksheetDimension();
+        } else {
+            $sheet->getCell('A1')->setValue($table);
+            $dimension = 'A1';
+        }
+        if ($lookup === null) {
+            $lastarg = '';
+        } else {
+            $lastarg = $lookup ? ',TRUE' : ',FALSE';
+        }
+        $sheet->getCell('Z98')->setValue($value);
+        $sheet->getCell('Z97')->setValue($index);
+
+        $sheet->getCell('Z99')->setValue("=VLOOKUP(Z98,$dimension,Z97$lastarg)");
+        $result = $sheet->getCell('Z99')->getCalculatedValue();
         self::assertEquals($expectedResult, $result);
+        $spreadsheet->disconnectWorksheets();
     }
 
     public function providerVLOOKUP(): array

tests/data/Calculation/LookupRef/HLOOKUP.php

@@ -186,4 +186,14 @@ return [
         3,
         true,
     ],
+    'issue2934' => [
+        'Red',
+        102,
+        [
+            [null, 102],
+            [null, 'Red'],
+        ],
+        2,
+        false,
+    ],
 ];

tests/data/Calculation/LookupRef/VLOOKUP.php

@@ -98,7 +98,7 @@ return [
             ['10y1', 7.0],
             ['10y2', 10.0],
         ],
-        'NaN',
+        -5,
     ],
     [
         '#REF!',
@@ -111,9 +111,9 @@ return [
         '#REF!',
         '10y2',
         [
-            2.0,
+            [2.0],
-            7.0,
+            [7.0],
-            10.0,
+            [10.0],
         ],
         2.0,
     ],
@@ -163,4 +163,34 @@ return [
         3,
         null,
     ],
+    'issue2934' => [
+        'Red',
+        102,
+        [
+            [null, null],
+            [102, 'Red'],
+        ],
+        2,
+        false,
+    ],
+    'string supplied as index' => [
+        '#VALUE!',
+        102,
+        [
+            [null, null],
+            [102, 'Red'],
+        ],
+        'xyz',
+        false,
+    ],
+    'num error propagated' => [
+        '#NUM!',
+        102,
+        [
+            [null, null],
+            [102, 'Red'],
+        ],
+        '=SQRT(-1)',
+        false,
+    ],
 ];