Skip to content

Commit ac5299b

Browse files
oleibmanoleibman
authored
Minor Fix for AND/OR/XOR (#3287)
* Minor Fix for AND/OR/XOR These 3 fall into the set of functions where Excel treats string literals differently depending on whether they are passed to the function directly or as a cell reference. PhpSpreadsheet is updated to try to duplicate that logic. New tests are added. Some existing test results had to change as a result of this code change. * Adopt A Suggestion From Mark Baker Reduce if statements by adding functions.
1 parent b043a01 commit ac5299b

File tree

11 files changed

+184
-94
lines changed

11 files changed

+184
-94
lines changed

‎phpstan-baseline.neon

-10
Original file line numberDiff line numberDiff line change
@@ -80,16 +80,6 @@ parameters:
8080
count: 1
8181
path: src/PhpSpreadsheet/Calculation/Internal/MakeMatrix.php
8282

83-
-
84-
message: "#^Call to function is_string\\(\\) with null will always evaluate to false\\.$#"
85-
count: 3
86-
path: src/PhpSpreadsheet/Calculation/Logical/Operations.php
87-
88-
-
89-
message: "#^Result of && is always false\\.$#"
90-
count: 3
91-
path: src/PhpSpreadsheet/Calculation/Logical/Operations.php
92-
9383
-
9484
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\:\\:CHOOSE\\(\\) has parameter \\$chooseArgs with no type specified\\.$#"
9585
count: 1

‎src/PhpSpreadsheet/Calculation/Logical/Operations.php

+23-58
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,9 @@ class Operations
3333
*/
3434
public static function logicalAnd(...$args)
3535
{
36-
$args = Functions::flattenArray($args);
37-
38-
if (count($args) == 0) {
39-
return ExcelError::VALUE();
40-
}
41-
42-
$args = array_filter($args, function ($value) {
43-
return $value !== null || (is_string($value) && trim($value) == '');
36+
return self::countTrueValues($args, function (int $trueValueCount, int $count): bool {
37+
return $trueValueCount === $count;
4438
});
45-
46-
$returnValue = self::countTrueValues($args);
47-
if (is_string($returnValue)) {
48-
return $returnValue;
49-
}
50-
$argCount = count($args);
51-
52-
return ($returnValue > 0) && ($returnValue == $argCount);
5339
}
5440

5541
/**
@@ -74,22 +60,9 @@ public static function logicalAnd(...$args)
7460
*/
7561
public static function logicalOr(...$args)
7662
{
77-
$args = Functions::flattenArray($args);
78-
79-
if (count($args) == 0) {
80-
return ExcelError::VALUE();
81-
}
82-
83-
$args = array_filter($args, function ($value) {
84-
return $value !== null || (is_string($value) && trim($value) == '');
63+
return self::countTrueValues($args, function (int $trueValueCount): bool {
64+
return $trueValueCount > 0;
8565
});
86-
87-
$returnValue = self::countTrueValues($args);
88-
if (is_string($returnValue)) {
89-
return $returnValue;
90-
}
91-
92-
return $returnValue > 0;
9366
}
9467

9568
/**
@@ -116,22 +89,9 @@ public static function logicalOr(...$args)
11689
*/
11790
public static function logicalXor(...$args)
11891
{
119-
$args = Functions::flattenArray($args);
120-
121-
if (count($args) == 0) {
122-
return ExcelError::VALUE();
123-
}
124-
125-
$args = array_filter($args, function ($value) {
126-
return $value !== null || (is_string($value) && trim($value) == '');
92+
return self::countTrueValues($args, function (int $trueValueCount): bool {
93+
return $trueValueCount % 2 === 1;
12794
});
128-
129-
$returnValue = self::countTrueValues($args);
130-
if (is_string($returnValue)) {
131-
return $returnValue;
132-
}
133-
134-
return $returnValue % 2 == 1;
13595
}
13696

13797
/**
@@ -177,31 +137,36 @@ public static function NOT($logical = false)
177137
}
178138

179139
/**
180-
* @return int|string
140+
* @return bool|string
181141
*/
182-
private static function countTrueValues(array $args)
142+
private static function countTrueValues(array $args, callable $func)
183143
{
184144
$trueValueCount = 0;
145+
$count = 0;
185146

186-
foreach ($args as $arg) {
147+
$aArgs = Functions::flattenArrayIndexed($args);
148+
foreach ($aArgs as $k => $arg) {
149+
++$count;
187150
// Is it a boolean value?
188151
if (is_bool($arg)) {
189152
$trueValueCount += $arg;
190-
} elseif ((is_numeric($arg)) && (!is_string($arg))) {
191-
$trueValueCount += ((int) $arg != 0);
192153
} elseif (is_string($arg)) {
154+
$isLiteral = !Functions::isCellValue($k);
193155
$arg = mb_strtoupper($arg, 'UTF-8');
194-
if (($arg == 'TRUE') || ($arg == Calculation::getTRUE())) {
195-
$arg = true;
196-
} elseif (($arg == 'FALSE') || ($arg == Calculation::getFALSE())) {
197-
$arg = false;
156+
if ($isLiteral && ($arg == 'TRUE' || $arg == Calculation::getTRUE())) {
157+
++$trueValueCount;
158+
} elseif ($isLiteral && ($arg == 'FALSE' || $arg == Calculation::getFALSE())) {
159+
//$trueValueCount += 0;
198160
} else {
199-
return ExcelError::VALUE();
161+
--$count;
200162
}
201-
$trueValueCount += ($arg != 0);
163+
} elseif (is_int($arg) || is_float($arg)) {
164+
$trueValueCount += (int) ($arg != 0);
165+
} else {
166+
--$count;
202167
}
203168
}
204169

205-
return $trueValueCount;
170+
return ($count === 0) ? ExcelError::VALUE() : $func($trueValueCount, $count);
206171
}
207172
}

‎tests/PhpSpreadsheetTests/Calculation/Functions/Logical/AndTest.php

+18
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,22 @@ public function providerAND(): array
1818
{
1919
return require 'tests/data/Calculation/Logical/AND.php';
2020
}
21+
22+
/**
23+
* @dataProvider providerANDLiteral
24+
*
25+
* @param mixed $expectedResult
26+
* @param string $formula
27+
*/
28+
public function testANDLiteral($expectedResult, $formula): void
29+
{
30+
$sheet = $this->getSheet();
31+
$sheet->getCell('A1')->setValue("=AND($formula)");
32+
self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue());
33+
}
34+
35+
public function providerANDLiteral(): array
36+
{
37+
return require 'tests/data/Calculation/Logical/ANDLiteral.php';
38+
}
2139
}

‎tests/PhpSpreadsheetTests/Calculation/Functions/Logical/OrTest.php

+18
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,22 @@ public function providerOR(): array
1818
{
1919
return require 'tests/data/Calculation/Logical/OR.php';
2020
}
21+
22+
/**
23+
* @dataProvider providerORLiteral
24+
*
25+
* @param mixed $expectedResult
26+
* @param string $formula
27+
*/
28+
public function testORLiteral($expectedResult, $formula): void
29+
{
30+
$sheet = $this->getSheet();
31+
$sheet->getCell('A1')->setValue("=OR($formula)");
32+
self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue());
33+
}
34+
35+
public function providerORLiteral(): array
36+
{
37+
return require 'tests/data/Calculation/Logical/ORLiteral.php';
38+
}
2139
}

‎tests/PhpSpreadsheetTests/Calculation/Functions/Logical/XorTest.php

+18
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,22 @@ public function providerXOR(): array
1818
{
1919
return require 'tests/data/Calculation/Logical/XOR.php';
2020
}
21+
22+
/**
23+
* @dataProvider providerXORLiteral
24+
*
25+
* @param mixed $expectedResult
26+
* @param string $formula
27+
*/
28+
public function xtestXORLiteral($expectedResult, $formula): void
29+
{
30+
$sheet = $this->getSheet();
31+
$sheet->getCell('A1')->setValue("=XOR($formula)");
32+
self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue());
33+
}
34+
35+
public function providerXORLiteral(): array
36+
{
37+
return require 'tests/data/Calculation/Logical/XORLiteral.php';
38+
}
2139
}

‎tests/data/Calculation/Logical/AND.php

+10-14
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,8 @@
44
'no arguments' => [
55
'exception',
66
],
7-
// NULL
8-
[
9-
false,
7+
'only argument is null reference' => [
8+
'#VALUE!',
109
null,
1110
],
1211
// Boolean TRUE and NULL
@@ -87,28 +86,25 @@
8786
1,
8887
1,
8988
],
90-
[
91-
'#VALUE!',
89+
'string 1 is ignored' => [
90+
true,
9291
'1',
9392
1,
9493
],
95-
// 'TRUE' String
96-
[
94+
'true string is ignored' => [
9795
true,
9896
'TRUE',
9997
1,
10098
],
101-
// 'FALSE' String
102-
[
103-
false,
99+
'false string is ignored' => [
100+
true,
104101
'FALSE',
105102
true,
106103
],
107-
// Non-numeric String
108-
[
109-
'#VALUE!',
104+
'non-boolean string is ignored' => [
105+
false,
110106
'ABCD',
111-
1,
107+
0,
112108
],
113109
[
114110
true,

‎tests/data/Calculation/Logical/ANDLiteral.php

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
return [
4+
'both boolean true' => [true, 'true, true'],
5+
'boolean and string-true' => [true, 'true, "true"'],
6+
'true and non-boolean-string' => [true, 'true, "xyz"'],
7+
'non-boolean-string and true' => [true, '"xyz", true'],
8+
'empty-string and true' => [true, '"", true'],
9+
'non-boolean-string and false' => [false, 'false, "xyz"'],
10+
'false and non-boolean-string' => [false, '"xyz", false'],
11+
'only non-boolean-string' => ['#VALUE!', '"xyz"'],
12+
'only boolean-string' => [true, '"true"'],
13+
'numeric-true and true' => [true, '3.1, true, true'],
14+
'numeric-false and true' => [false, '0, true, true'],
15+
'mixed boolean' => [false, 'true, true, true, false'],
16+
'only true' => [true, 'true'],
17+
'only false' => [false, 0.0],
18+
'boolean in array' => [true, '{true}'],
19+
'boolean-string in array' => ['#VALUE!', '{"true"}'],
20+
];

‎tests/data/Calculation/Logical/OR.php

+11-10
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,8 @@
44
'no arguments' => [
55
'exception',
66
],
7-
// NULL
8-
[
9-
false,
7+
'only argument is null reference' => [
8+
'#VALUE!',
109
null,
1110
],
1211
// Boolean TRUE and NULL
@@ -87,21 +86,23 @@
8786
1,
8887
1,
8988
],
90-
// 'TRUE' String
91-
[
89+
'string 1 is ignored' => [
90+
false,
91+
0,
92+
'1',
93+
],
94+
'true string is ignored' => [
9295
true,
9396
'TRUE',
9497
1,
9598
],
96-
// 'FALSE' String
97-
[
99+
'false string is ignored' => [
98100
true,
99101
'FALSE',
100102
true,
101103
],
102-
// Non-numeric String
103-
[
104-
'#VALUE!',
104+
'non-boolean string is ignored' => [
105+
true,
105106
'ABCD',
106107
1,
107108
],

‎tests/data/Calculation/Logical/ORLiteral.php

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
return [
4+
'both boolean true' => [true, 'true, true'],
5+
'boolean or string-true' => [true, 'true, "true"'],
6+
'true or non-boolean-string' => [true, 'true, "xyz"'],
7+
'non-boolean-string or true' => [true, '"xyz", true'],
8+
'empty-string or true' => [true, '"", true'],
9+
'non-boolean-string or false' => [false, 'false, "xyz"'],
10+
'false or non-boolean-string' => [false, '"xyz", false'],
11+
'only non-boolean-string' => ['#VALUE!', '"xyz"'],
12+
'only boolean-string' => [true, '"true"'],
13+
'numeric-true or true' => [true, '3.1, true, true'],
14+
'numeric-false or true' => [true, '0, true, true'],
15+
'mixed boolean' => [true, 'true, true, true, false'],
16+
'only true' => [true, 'true'],
17+
'only false' => [false, 0.0],
18+
'boolean in array' => [true, '{true}'],
19+
'boolean-string in array' => ['#VALUE!', '{"true"}'],
20+
];

‎tests/data/Calculation/Logical/XOR.php

+26-2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44
'no arguments' => [
55
'exception',
66
],
7+
'only argument is null reference' => [
8+
'#VALUE!',
9+
null,
10+
],
711
[
812
false,
913
true, true,
@@ -32,7 +36,7 @@
3236
true,
3337
true, true, true, false,
3438
],
35-
[
39+
'ignore string other two should be true' => [
3640
false,
3741
'TRUE',
3842
1,
@@ -44,8 +48,28 @@
4448
1.5,
4549
0,
4650
],
47-
[
51+
'only arg is string' => [
4852
'#VALUE!',
4953
'HELLO WORLD',
5054
],
55+
'true string is ignored' => [
56+
true,
57+
'TRUE',
58+
1,
59+
],
60+
'false string is ignored' => [
61+
true,
62+
'FALSE',
63+
true,
64+
],
65+
'string 1 is ignored' => [
66+
true,
67+
'1',
68+
true,
69+
],
70+
'non-boolean string is ignored' => [
71+
true,
72+
'ABCD',
73+
1,
74+
],
5175
];

0 commit comments

Comments
 (0)