From 57b391dc97f0c18d01d9c0ecc7e6d8e4460af1d0 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 9 Jan 2023 07:54:26 -0800 Subject: [PATCH 1/2] 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. --- phpstan-baseline.neon | 10 -- .../Calculation/Logical/Operations.php | 92 +++++++------------ .../Calculation/Functions/Logical/AndTest.php | 18 ++++ .../Calculation/Functions/Logical/OrTest.php | 18 ++++ .../Calculation/Functions/Logical/XorTest.php | 18 ++++ tests/data/Calculation/Logical/AND.php | 24 ++--- tests/data/Calculation/Logical/ANDLiteral.php | 20 ++++ tests/data/Calculation/Logical/OR.php | 21 +++-- tests/data/Calculation/Logical/ORLiteral.php | 20 ++++ tests/data/Calculation/Logical/XOR.php | 28 +++++- tests/data/Calculation/Logical/XORLiteral.php | 20 ++++ 11 files changed, 192 insertions(+), 97 deletions(-) create mode 100644 tests/data/Calculation/Logical/ANDLiteral.php create mode 100644 tests/data/Calculation/Logical/ORLiteral.php create mode 100644 tests/data/Calculation/Logical/XORLiteral.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index abb4209e9f..e6a9ebc8f3 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -270,16 +270,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Calculation/Internal/MakeMatrix.php - - - message: "#^Call to function is_string\\(\\) with null will always evaluate to false\\.$#" - count: 3 - path: src/PhpSpreadsheet/Calculation/Logical/Operations.php - - - - message: "#^Result of && is always false\\.$#" - count: 3 - path: src/PhpSpreadsheet/Calculation/Logical/Operations.php - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\LookupRef\\:\\:CHOOSE\\(\\) has parameter \\$chooseArgs with no type specified\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Calculation/Logical/Operations.php b/src/PhpSpreadsheet/Calculation/Logical/Operations.php index 2e2faa136c..a4e1e7bac8 100644 --- a/src/PhpSpreadsheet/Calculation/Logical/Operations.php +++ b/src/PhpSpreadsheet/Calculation/Logical/Operations.php @@ -33,23 +33,7 @@ class Operations */ public static function logicalAnd(...$args) { - $args = Functions::flattenArray($args); - - if (count($args) == 0) { - return ExcelError::VALUE(); - } - - $args = array_filter($args, function ($value) { - return $value !== null || (is_string($value) && trim($value) == ''); - }); - - $returnValue = self::countTrueValues($args); - if (is_string($returnValue)) { - return $returnValue; - } - $argCount = count($args); - - return ($returnValue > 0) && ($returnValue == $argCount); + return self::countTrueValues($args, 'and'); } /** @@ -74,22 +58,7 @@ public static function logicalAnd(...$args) */ public static function logicalOr(...$args) { - $args = Functions::flattenArray($args); - - if (count($args) == 0) { - return ExcelError::VALUE(); - } - - $args = array_filter($args, function ($value) { - return $value !== null || (is_string($value) && trim($value) == ''); - }); - - $returnValue = self::countTrueValues($args); - if (is_string($returnValue)) { - return $returnValue; - } - - return $returnValue > 0; + return self::countTrueValues($args, 'or'); } /** @@ -116,22 +85,7 @@ public static function logicalOr(...$args) */ public static function logicalXor(...$args) { - $args = Functions::flattenArray($args); - - if (count($args) == 0) { - return ExcelError::VALUE(); - } - - $args = array_filter($args, function ($value) { - return $value !== null || (is_string($value) && trim($value) == ''); - }); - - $returnValue = self::countTrueValues($args); - if (is_string($returnValue)) { - return $returnValue; - } - - return $returnValue % 2 == 1; + return self::countTrueValues($args, 'xor'); } /** @@ -177,31 +131,47 @@ public static function NOT($logical = false) } /** - * @return int|string + * @return bool|string */ - private static function countTrueValues(array $args) + private static function countTrueValues(array $args, string $operation) { $trueValueCount = 0; + $count = 0; - foreach ($args as $arg) { + $aArgs = Functions::flattenArrayIndexed($args); + foreach ($aArgs as $k => $arg) { + ++$count; // Is it a boolean value? if (is_bool($arg)) { $trueValueCount += $arg; - } elseif ((is_numeric($arg)) && (!is_string($arg))) { - $trueValueCount += ((int) $arg != 0); } elseif (is_string($arg)) { + $isLiteral = !Functions::isCellValue($k); $arg = mb_strtoupper($arg, 'UTF-8'); - if (($arg == 'TRUE') || ($arg == Calculation::getTRUE())) { - $arg = true; - } elseif (($arg == 'FALSE') || ($arg == Calculation::getFALSE())) { - $arg = false; + if ($isLiteral && ($arg == 'TRUE' || $arg == Calculation::getTRUE())) { + ++$trueValueCount; + } elseif ($isLiteral && ($arg == 'FALSE' || $arg == Calculation::getFALSE())) { + //$trueValueCount += 0; } else { - return ExcelError::VALUE(); + --$count; } - $trueValueCount += ($arg != 0); + } elseif (is_int($arg) || is_float($arg)) { + $trueValueCount += (int) ($arg != 0); + } else { + --$count; } } - return $trueValueCount; + if ($count === 0) { + return ExcelError::VALUE(); + } + if ($operation === 'and') { + return $trueValueCount === $count; + } + if ($operation === 'or') { + return $trueValueCount > 0; + } + + // xor + return $trueValueCount % 2 === 1; } } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/AndTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/AndTest.php index e5f14d7762..7c49d987be 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/AndTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/AndTest.php @@ -18,4 +18,22 @@ public function providerAND(): array { return require 'tests/data/Calculation/Logical/AND.php'; } + + /** + * @dataProvider providerANDLiteral + * + * @param mixed $expectedResult + * @param string $formula + */ + public function testANDLiteral($expectedResult, $formula): void + { + $sheet = $this->getSheet(); + $sheet->getCell('A1')->setValue("=AND($formula)"); + self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue()); + } + + public function providerANDLiteral(): array + { + return require 'tests/data/Calculation/Logical/ANDLiteral.php'; + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/OrTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/OrTest.php index 2338a46f85..4859e81161 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/OrTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/OrTest.php @@ -18,4 +18,22 @@ public function providerOR(): array { return require 'tests/data/Calculation/Logical/OR.php'; } + + /** + * @dataProvider providerORLiteral + * + * @param mixed $expectedResult + * @param string $formula + */ + public function testORLiteral($expectedResult, $formula): void + { + $sheet = $this->getSheet(); + $sheet->getCell('A1')->setValue("=OR($formula)"); + self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue()); + } + + public function providerORLiteral(): array + { + return require 'tests/data/Calculation/Logical/ORLiteral.php'; + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/XorTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/XorTest.php index 086d59807d..200a82be33 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/XorTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/Logical/XorTest.php @@ -18,4 +18,22 @@ public function providerXOR(): array { return require 'tests/data/Calculation/Logical/XOR.php'; } + + /** + * @dataProvider providerXORLiteral + * + * @param mixed $expectedResult + * @param string $formula + */ + public function xtestXORLiteral($expectedResult, $formula): void + { + $sheet = $this->getSheet(); + $sheet->getCell('A1')->setValue("=XOR($formula)"); + self::assertSame($expectedResult, $sheet->getCell('A1')->getCalculatedValue()); + } + + public function providerXORLiteral(): array + { + return require 'tests/data/Calculation/Logical/XORLiteral.php'; + } } diff --git a/tests/data/Calculation/Logical/AND.php b/tests/data/Calculation/Logical/AND.php index 221a4cb2d9..3d08a3a6a3 100644 --- a/tests/data/Calculation/Logical/AND.php +++ b/tests/data/Calculation/Logical/AND.php @@ -4,9 +4,8 @@ 'no arguments' => [ 'exception', ], - // NULL - [ - false, + 'only argument is null reference' => [ + '#VALUE!', null, ], // Boolean TRUE and NULL @@ -87,28 +86,25 @@ 1, 1, ], - [ - '#VALUE!', + 'string 1 is ignored' => [ + true, '1', 1, ], - // 'TRUE' String - [ + 'true string is ignored' => [ true, 'TRUE', 1, ], - // 'FALSE' String - [ - false, + 'false string is ignored' => [ + true, 'FALSE', true, ], - // Non-numeric String - [ - '#VALUE!', + 'non-boolean string is ignored' => [ + false, 'ABCD', - 1, + 0, ], [ true, diff --git a/tests/data/Calculation/Logical/ANDLiteral.php b/tests/data/Calculation/Logical/ANDLiteral.php new file mode 100644 index 0000000000..0dff483bb7 --- /dev/null +++ b/tests/data/Calculation/Logical/ANDLiteral.php @@ -0,0 +1,20 @@ + [true, 'true, true'], + 'boolean and string-true' => [true, 'true, "true"'], + 'true and non-boolean-string' => [true, 'true, "xyz"'], + 'non-boolean-string and true' => [true, '"xyz", true'], + 'empty-string and true' => [true, '"", true'], + 'non-boolean-string and false' => [false, 'false, "xyz"'], + 'false and non-boolean-string' => [false, '"xyz", false'], + 'only non-boolean-string' => ['#VALUE!', '"xyz"'], + 'only boolean-string' => [true, '"true"'], + 'numeric-true and true' => [true, '3.1, true, true'], + 'numeric-false and true' => [false, '0, true, true'], + 'mixed boolean' => [false, 'true, true, true, false'], + 'only true' => [true, 'true'], + 'only false' => [false, 0.0], + 'boolean in array' => [true, '{true}'], + 'boolean-string in array' => ['#VALUE!', '{"true"}'], +]; diff --git a/tests/data/Calculation/Logical/OR.php b/tests/data/Calculation/Logical/OR.php index 148147fb3f..c5317b3bcb 100644 --- a/tests/data/Calculation/Logical/OR.php +++ b/tests/data/Calculation/Logical/OR.php @@ -4,9 +4,8 @@ 'no arguments' => [ 'exception', ], - // NULL - [ - false, + 'only argument is null reference' => [ + '#VALUE!', null, ], // Boolean TRUE and NULL @@ -87,21 +86,23 @@ 1, 1, ], - // 'TRUE' String - [ + 'string 1 is ignored' => [ + false, + 0, + '1', + ], + 'true string is ignored' => [ true, 'TRUE', 1, ], - // 'FALSE' String - [ + 'false string is ignored' => [ true, 'FALSE', true, ], - // Non-numeric String - [ - '#VALUE!', + 'non-boolean string is ignored' => [ + true, 'ABCD', 1, ], diff --git a/tests/data/Calculation/Logical/ORLiteral.php b/tests/data/Calculation/Logical/ORLiteral.php new file mode 100644 index 0000000000..17f4988226 --- /dev/null +++ b/tests/data/Calculation/Logical/ORLiteral.php @@ -0,0 +1,20 @@ + [true, 'true, true'], + 'boolean or string-true' => [true, 'true, "true"'], + 'true or non-boolean-string' => [true, 'true, "xyz"'], + 'non-boolean-string or true' => [true, '"xyz", true'], + 'empty-string or true' => [true, '"", true'], + 'non-boolean-string or false' => [false, 'false, "xyz"'], + 'false or non-boolean-string' => [false, '"xyz", false'], + 'only non-boolean-string' => ['#VALUE!', '"xyz"'], + 'only boolean-string' => [true, '"true"'], + 'numeric-true or true' => [true, '3.1, true, true'], + 'numeric-false or true' => [true, '0, true, true'], + 'mixed boolean' => [true, 'true, true, true, false'], + 'only true' => [true, 'true'], + 'only false' => [false, 0.0], + 'boolean in array' => [true, '{true}'], + 'boolean-string in array' => ['#VALUE!', '{"true"}'], +]; diff --git a/tests/data/Calculation/Logical/XOR.php b/tests/data/Calculation/Logical/XOR.php index 51c041b560..57a170a48c 100644 --- a/tests/data/Calculation/Logical/XOR.php +++ b/tests/data/Calculation/Logical/XOR.php @@ -4,6 +4,10 @@ 'no arguments' => [ 'exception', ], + 'only argument is null reference' => [ + '#VALUE!', + null, + ], [ false, true, true, @@ -32,7 +36,7 @@ true, true, true, true, false, ], - [ + 'ignore string other two should be true' => [ false, 'TRUE', 1, @@ -44,8 +48,28 @@ 1.5, 0, ], - [ + 'only arg is string' => [ '#VALUE!', 'HELLO WORLD', ], + 'true string is ignored' => [ + true, + 'TRUE', + 1, + ], + 'false string is ignored' => [ + true, + 'FALSE', + true, + ], + 'string 1 is ignored' => [ + true, + '1', + true, + ], + 'non-boolean string is ignored' => [ + true, + 'ABCD', + 1, + ], ]; diff --git a/tests/data/Calculation/Logical/XORLiteral.php b/tests/data/Calculation/Logical/XORLiteral.php new file mode 100644 index 0000000000..a0c0be7073 --- /dev/null +++ b/tests/data/Calculation/Logical/XORLiteral.php @@ -0,0 +1,20 @@ + [false, 'true, true'], + 'boolean xor string-true' => [false, 'true, "true"'], + 'true xor non-boolean-string' => [true, 'true, "xyz"'], + 'non-boolean-string xor true' => [true, '"xyz", true'], + 'empty-string xor true' => [true, '"", true'], + 'non-boolean-string xor false' => [false, 'false, "xyz"'], + 'false xor non-boolean-string' => [false, '"xyz", false'], + 'only non-boolean-string' => ['#VALUE!', '"xyz"'], + 'only boolean-string' => [true, '"true"'], + 'numeric-true xor true xor true' => [true, '3.1, true, true'], + 'numeric-false xor true xor true' => [false, '0, true, true'], + 'mixed boolean' => [true, 'true, true, true, false'], + 'only true' => [true, 'true'], + 'only false' => [false, 0.0], + 'boolean in array' => [true, '{true}'], + 'boolean-string in array' => ['#VALUE!', '{"true"}'], +]; From d692c1e077a5c1b4ee95f5885389cf9226ab87a3 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Tue, 10 Jan 2023 18:54:53 -0800 Subject: [PATCH 2/2] Adopt A Suggestion From Mark Baker Reduce if statements by adding functions. --- .../Calculation/Logical/Operations.php | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Logical/Operations.php b/src/PhpSpreadsheet/Calculation/Logical/Operations.php index a4e1e7bac8..f09785919c 100644 --- a/src/PhpSpreadsheet/Calculation/Logical/Operations.php +++ b/src/PhpSpreadsheet/Calculation/Logical/Operations.php @@ -33,7 +33,9 @@ class Operations */ public static function logicalAnd(...$args) { - return self::countTrueValues($args, 'and'); + return self::countTrueValues($args, function (int $trueValueCount, int $count): bool { + return $trueValueCount === $count; + }); } /** @@ -58,7 +60,9 @@ public static function logicalAnd(...$args) */ public static function logicalOr(...$args) { - return self::countTrueValues($args, 'or'); + return self::countTrueValues($args, function (int $trueValueCount): bool { + return $trueValueCount > 0; + }); } /** @@ -85,7 +89,9 @@ public static function logicalOr(...$args) */ public static function logicalXor(...$args) { - return self::countTrueValues($args, 'xor'); + return self::countTrueValues($args, function (int $trueValueCount): bool { + return $trueValueCount % 2 === 1; + }); } /** @@ -133,7 +139,7 @@ public static function NOT($logical = false) /** * @return bool|string */ - private static function countTrueValues(array $args, string $operation) + private static function countTrueValues(array $args, callable $func) { $trueValueCount = 0; $count = 0; @@ -161,17 +167,6 @@ private static function countTrueValues(array $args, string $operation) } } - if ($count === 0) { - return ExcelError::VALUE(); - } - if ($operation === 'and') { - return $trueValueCount === $count; - } - if ($operation === 'or') { - return $trueValueCount > 0; - } - - // xor - return $trueValueCount % 2 === 1; + return ($count === 0) ? ExcelError::VALUE() : $func($trueValueCount, $count); } }