Skip to content

Commit fac0e46

Browse files
oleibmanoleibman
authored
MATCH Problems with Int/Float Compare and Wildcards (#3142)
* MATCH Problems with Int/Float Compare and Wildcards Fix #3141. Function matchSmallestValue did not recognize that an integer could match a float. Adding test cases, it seems that matchFirstValue had the same problem. However, matchLargestValue seemed to handle things correctly - but see below. In addition, the wildcard logic in matchFirstValue is faulty. It ignored tilde as a wildcard character. Although it would have been easy to just add that, I think it was wrong to determine on its own if a wildcard was in use. Just using the already available wildcard functions whenever comparing two strings is sufficient. I note that Excel doesn't seem to follow its own rules for MATCH (https://support.microsoft.com/en-us/office/match-function-e8dffd45-c762-47d6-bf89-533f4a37673a?ns=excel&version=90&syslcid=1033&uilcid=1033&appver=zxl900&helpid=xlmain11.chm60112&ui=en-us&rs=en-us&ad=us). PhpSpreadsheet's results match Excel's, so no problem. However, when match_type is not zero, the match array is supposed to be sorted, so I would expect `#N/A` when it isn't; but that's not how Excel operates. I have no idea what Excel is doing. If `MATCH(2,{2,0,4,3},1)` isn't `#N/A` because of the unsorted array, then surely it should be `1` (item 1 of the array is the largest number less than or equal to the lookup value); but Excel and PhpSpreadsheet (before and after changes) return `2`. I have moved this example to be the first of the test cases. One would think strings would behave similarly. But, no - see the second test case. This time Excel does look for an exact match. But the existing logic doesn't get the matching result in PhpSpreadsheet. It requires a whole new block of code, one which doesn't work correctly for numeric lookup value. Ugh. LibreOffice doesn't always agree with Excel. It seems that it will use wildcard matching even when the match type is not zero (Excel documentation says wildcards are only for type zero, which is just as well because I don't really know what greater/less mean when wildcards are involved). I have not attempted to duplicate this behavior. For the record, Gnumeric agrees with Excel here. * More Changes - LibreOffice Add support for LibreOffice matching wildcard strings when type is not zero. Add support for type to be specified as integer other than 0/1/-1, or as float, or as numeric string; non-numeric string should case `#VALUE!` error. I have found an example of undefined behavior (unsorted array where type is non-zero) where PhpSpreadsheet does not produce the same result as Excel. It is present as a new `incomplete` test case. I can fix it, but not without breaking other tests where the proper behavior is undefined. IMO, this is not a problem we should be concerned about. Many test cases are added. Chances are I will add some more before merging this change.
1 parent 47c00d0 commit fac0e46

File tree

4 files changed

+354
-126
lines changed

4 files changed

+354
-126
lines changed

‎phpstan-baseline.neon

Lines changed: 0 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -312,70 +312,6 @@ parameters:
312312
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\Address\\:\\:sheetName\\(\\) has no return type specified\\.$#"
313313
count: 1
314314
path: src/PhpSpreadsheet/Calculation/LookupRef/Address.php
315-
-
316-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchFirstValue\\(\\) has no return type specified\\.$#"
317-
count: 1
318-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
319-
-
320-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchFirstValue\\(\\) has parameter \\$lookupArray with no type specified\\.$#"
321-
count: 1
322-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
323-
-
324-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchFirstValue\\(\\) has parameter \\$lookupValue with no type specified\\.$#"
325-
count: 1
326-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
327-
-
328-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchLargestValue\\(\\) has no return type specified\\.$#"
329-
count: 1
330-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
331-
-
332-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchLargestValue\\(\\) has parameter \\$keySet with no type specified\\.$#"
333-
count: 1
334-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
335-
-
336-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchLargestValue\\(\\) has parameter \\$lookupArray with no type specified\\.$#"
337-
count: 1
338-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
339-
-
340-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchLargestValue\\(\\) has parameter \\$lookupValue with no type specified\\.$#"
341-
count: 1
342-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
343-
-
344-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchSmallestValue\\(\\) has no return type specified\\.$#"
345-
count: 1
346-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
347-
-
348-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchSmallestValue\\(\\) has parameter \\$lookupArray with no type specified\\.$#"
349-
count: 1
350-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
351-
-
352-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:matchSmallestValue\\(\\) has parameter \\$lookupValue with no type specified\\.$#"
353-
count: 1
354-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
355-
-
356-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:prepareLookupArray\\(\\) has no return type specified\\.$#"
357-
count: 1
358-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
359-
-
360-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:prepareLookupArray\\(\\) has parameter \\$lookupArray with no type specified\\.$#"
361-
count: 1
362-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
363-
-
364-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:prepareLookupArray\\(\\) has parameter \\$matchType with no type specified\\.$#"
365-
count: 1
366-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
367-
-
368-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:validateLookupArray\\(\\) has parameter \\$lookupArray with no type specified\\.$#"
369-
count: 1
370-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
371-
-
372-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:validateLookupValue\\(\\) has parameter \\$lookupValue with no type specified\\.$#"
373-
count: 1
374-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
375-
-
376-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\ExcelMatch\\:\\:validateMatchType\\(\\) has parameter \\$matchType with no type specified\\.$#"
377-
count: 1
378-
path: src/PhpSpreadsheet/Calculation/LookupRef/ExcelMatch.php
379315
-
380316
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\\\Lookup\\:\\:verifyResultVector\\(\\) has no return type specified\\.$#"
381317
count: 1

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

Lines changed: 108 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,11 @@ public static function MATCH($lookupValue, $lookupArray, $matchType = self::MATC
3939
}
4040

4141
$lookupArray = Functions::flattenArray($lookupArray);
42-
$matchType = (int) ($matchType ?? self::MATCHTYPE_LARGEST_VALUE);
4342

4443
try {
4544
// Input validation
4645
self::validateLookupValue($lookupValue);
47-
self::validateMatchType($matchType);
46+
$matchType = self::validateMatchType($matchType);
4847
self::validateLookupArray($lookupArray);
4948

5049
$keySet = array_keys($lookupArray);
@@ -87,36 +86,78 @@ public static function MATCH($lookupValue, $lookupArray, $matchType = self::MATC
8786
return ExcelError::NA();
8887
}
8988

90-
private static function matchFirstValue($lookupArray, $lookupValue)
89+
/**
90+
* @param mixed $lookupValue
91+
*
92+
* @return mixed
93+
*/
94+
private static function matchFirstValue(array $lookupArray, $lookupValue)
9195
{
92-
$wildcardLookup = ((bool) preg_match('/([\?\*])/', $lookupValue));
93-
$wildcard = WildcardMatch::wildcard($lookupValue);
96+
if (is_string($lookupValue)) {
97+
$valueIsString = true;
98+
$wildcard = WildcardMatch::wildcard($lookupValue);
99+
} else {
100+
$valueIsString = false;
101+
$wildcard = '';
102+
}
94103

104+
$valueIsNumeric = is_int($lookupValue) || is_float($lookupValue);
95105
foreach ($lookupArray as $i => $lookupArrayValue) {
96-
$typeMatch = ((gettype($lookupValue) === gettype($lookupArrayValue)) ||
97-
(is_numeric($lookupValue) && is_numeric($lookupArrayValue)));
98-
99106
if (
100-
$typeMatch && is_string($lookupValue) &&
101-
$wildcardLookup && WildcardMatch::compare($lookupArrayValue, $wildcard)
107+
$valueIsString
108+
&& is_string($lookupArrayValue)
102109
) {
103-
// wildcard match
104-
return $i;
105-
} elseif ($lookupArrayValue === $lookupValue) {
106-
// exact match
107-
return $i;
110+
if (WildcardMatch::compare($lookupArrayValue, $wildcard)) {
111+
return $i; // wildcard match
112+
}
113+
} else {
114+
if ($lookupArrayValue === $lookupValue) {
115+
return $i; // exact match
116+
}
117+
if (
118+
$valueIsNumeric
119+
&& (is_float($lookupArrayValue) || is_int($lookupArrayValue))
120+
&& $lookupArrayValue == $lookupValue
121+
) {
122+
return $i; // exact match
123+
}
108124
}
109125
}
110126

111127
return null;
112128
}
113129

114-
private static function matchLargestValue($lookupArray, $lookupValue, $keySet)
130+
/**
131+
* @param mixed $lookupValue
132+
*
133+
* @return mixed
134+
*/
135+
private static function matchLargestValue(array $lookupArray, $lookupValue, array $keySet)
115136
{
137+
if (is_string($lookupValue)) {
138+
if (Functions::getCompatibilityMode() === Functions::COMPATIBILITY_OPENOFFICE) {
139+
$wildcard = WildcardMatch::wildcard($lookupValue);
140+
foreach (array_reverse($lookupArray) as $i => $lookupArrayValue) {
141+
if (is_string($lookupArrayValue) && WildcardMatch::compare($lookupArrayValue, $wildcard)) {
142+
return $i;
143+
}
144+
}
145+
} else {
146+
foreach ($lookupArray as $i => $lookupArrayValue) {
147+
if ($lookupArrayValue === $lookupValue) {
148+
return $keySet[$i];
149+
}
150+
}
151+
}
152+
}
153+
$valueIsNumeric = is_int($lookupValue) || is_float($lookupValue);
116154
foreach ($lookupArray as $i => $lookupArrayValue) {
117-
$typeMatch = ((gettype($lookupValue) === gettype($lookupArrayValue)) ||
118-
(is_numeric($lookupValue) && is_numeric($lookupArrayValue)));
119-
155+
if ($valueIsNumeric && (is_int($lookupArrayValue) || is_float($lookupArrayValue))) {
156+
if ($lookupArrayValue <= $lookupValue) {
157+
return array_search($i, $keySet);
158+
}
159+
}
160+
$typeMatch = gettype($lookupValue) === gettype($lookupArrayValue);
120161
if ($typeMatch && ($lookupArrayValue <= $lookupValue)) {
121162
return array_search($i, $keySet);
122163
}
@@ -125,21 +166,42 @@ private static function matchLargestValue($lookupArray, $lookupValue, $keySet)
125166
return null;
126167
}
127168

128-
private static function matchSmallestValue($lookupArray, $lookupValue)
169+
/**
170+
* @param mixed $lookupValue
171+
*
172+
* @return mixed
173+
*/
174+
private static function matchSmallestValue(array $lookupArray, $lookupValue)
129175
{
130176
$valueKey = null;
177+
if (is_string($lookupValue)) {
178+
if (Functions::getCompatibilityMode() === Functions::COMPATIBILITY_OPENOFFICE) {
179+
$wildcard = WildcardMatch::wildcard($lookupValue);
180+
foreach ($lookupArray as $i => $lookupArrayValue) {
181+
if (is_string($lookupArrayValue) && WildcardMatch::compare($lookupArrayValue, $wildcard)) {
182+
return $i;
183+
}
184+
}
185+
}
186+
}
131187

188+
$valueIsNumeric = is_int($lookupValue) || is_float($lookupValue);
132189
// The basic algorithm is:
133190
// Iterate and keep the highest match until the next element is smaller than the searched value.
134191
// Return immediately if perfect match is found
135192
foreach ($lookupArray as $i => $lookupArrayValue) {
136193
$typeMatch = gettype($lookupValue) === gettype($lookupArrayValue);
194+
$bothNumeric = $valueIsNumeric && (is_int($lookupArrayValue) || is_float($lookupArrayValue));
137195

138196
if ($lookupArrayValue === $lookupValue) {
139197
// Another "special" case. If a perfect match is found,
140198
// the algorithm gives up immediately
141199
return $i;
142-
} elseif ($typeMatch && $lookupArrayValue >= $lookupValue) {
200+
}
201+
if ($bothNumeric && $lookupValue == $lookupArrayValue) {
202+
return $i; // exact match, as above
203+
}
204+
if (($typeMatch || $bothNumeric) && $lookupArrayValue >= $lookupValue) {
143205
$valueKey = $i;
144206
} elseif ($typeMatch && $lookupArrayValue < $lookupValue) {
145207
//Excel algorithm gives up immediately if the first element is smaller than the searched value
@@ -150,6 +212,9 @@ private static function matchSmallestValue($lookupArray, $lookupValue)
150212
return $valueKey;
151213
}
152214

215+
/**
216+
* @param mixed $lookupValue
217+
*/
153218
private static function validateLookupValue($lookupValue): void
154219
{
155220
// Lookup_value type has to be number, text, or logical values
@@ -158,18 +223,29 @@ private static function validateLookupValue($lookupValue): void
158223
}
159224
}
160225

161-
private static function validateMatchType($matchType): void
226+
/**
227+
* @param mixed $matchType
228+
*/
229+
private static function validateMatchType($matchType): int
162230
{
163231
// Match_type is 0, 1 or -1
164-
if (
165-
($matchType !== self::MATCHTYPE_FIRST_VALUE) &&
166-
($matchType !== self::MATCHTYPE_LARGEST_VALUE) && ($matchType !== self::MATCHTYPE_SMALLEST_VALUE)
167-
) {
168-
throw new Exception(ExcelError::NA());
232+
// However Excel accepts other numeric values,
233+
// including numeric strings and floats.
234+
// It seems to just be interested in the sign.
235+
if (!is_numeric($matchType)) {
236+
throw new Exception(ExcelError::Value());
237+
}
238+
if ($matchType > 0) {
239+
return self::MATCHTYPE_LARGEST_VALUE;
169240
}
241+
if ($matchType < 0) {
242+
return self::MATCHTYPE_SMALLEST_VALUE;
243+
}
244+
245+
return self::MATCHTYPE_FIRST_VALUE;
170246
}
171247

172-
private static function validateLookupArray($lookupArray): void
248+
private static function validateLookupArray(array $lookupArray): void
173249
{
174250
// Lookup_array should not be empty
175251
$lookupArraySize = count($lookupArray);
@@ -178,7 +254,10 @@ private static function validateLookupArray($lookupArray): void
178254
}
179255
}
180256

181-
private static function prepareLookupArray($lookupArray, $matchType)
257+
/**
258+
* @param mixed $matchType
259+
*/
260+
private static function prepareLookupArray(array $lookupArray, $matchType): array
182261
{
183262
// Lookup_array should contain only number, text, or logical values, or empty (null) cells
184263
foreach ($lookupArray as $i => $value) {

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

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,25 +3,77 @@
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;
8-
use PHPUnit\Framework\TestCase;
96

10-
class MatchTest extends TestCase
7+
class MatchTest extends AllSetupTeardown
118
{
12-
protected function setUp(): void
9+
/**
10+
* @dataProvider providerMATCH
11+
*
12+
* @param mixed $expectedResult
13+
* @param mixed $input
14+
* @param mixed $type
15+
*/
16+
public function testMATCH($expectedResult, $input, array $array, $type = null): void
1317
{
14-
Functions::setCompatibilityMode(Functions::COMPATIBILITY_EXCEL);
18+
if (is_array($expectedResult)) {
19+
$expectedResult = $expectedResult[0];
20+
}
21+
if ($expectedResult === 'incomplete') {
22+
self::markTestIncomplete('Undefined behavior with different results in Excel and PhpSpreadsheet');
23+
}
24+
$this->mightHaveException($expectedResult);
25+
$sheet = $this->getSheet();
26+
$maxRow = count($array);
27+
$formulaArray = '';
28+
for ($row = 1; $row <= $maxRow; ++$row) {
29+
$this->setCell("A$row", $array[$row - 1]);
30+
$formulaArray = "A1:A$row";
31+
}
32+
$this->setCell('B1', $input);
33+
if ($type === null) {
34+
$formula = "=MATCH(B1,$formulaArray)";
35+
} else {
36+
$formula = "=MATCH(B1, $formulaArray, $type)";
37+
}
38+
$sheet->getCell('D1')->setValue($formula);
39+
40+
$result = $sheet->getCell('D1')->getCalculatedValue();
41+
self::assertEquals($expectedResult, $result);
1542
}
1643

1744
/**
1845
* @dataProvider providerMATCH
1946
*
2047
* @param mixed $expectedResult
48+
* @param mixed $input
49+
* @param mixed $type
2150
*/
22-
public function testMATCH($expectedResult, ...$args): void
51+
public function testMATCHLibre($expectedResult, $input, array $array, $type = null): void
2352
{
24-
$result = LookupRef::MATCH(...$args);
53+
$this->setOpenOffice();
54+
if (is_array($expectedResult)) {
55+
$expectedResult = $expectedResult[1];
56+
}
57+
if ($expectedResult === 'incomplete') {
58+
self::markTestIncomplete('Undefined behavior with different results in Excel and PhpSpreadsheet');
59+
}
60+
$this->mightHaveException($expectedResult);
61+
$sheet = $this->getSheet();
62+
$maxRow = count($array);
63+
$formulaArray = '';
64+
for ($row = 1; $row <= $maxRow; ++$row) {
65+
$this->setCell("A$row", $array[$row - 1]);
66+
$formulaArray = "A1:A$row";
67+
}
68+
$this->setCell('B1', $input);
69+
if ($type === null) {
70+
$formula = "=MATCH(B1,$formulaArray)";
71+
} else {
72+
$formula = "=MATCH(B1, $formulaArray, $type)";
73+
}
74+
$sheet->getCell('D1')->setValue($formula);
75+
76+
$result = $sheet->getCell('D1')->getCalculatedValue();
2577
self::assertEquals($expectedResult, $result);
2678
}
2779

0 commit comments

Comments
 (0)