Calculation suppressFormulaErrors - Minor Break and Deprecation (#3092)

Fix #1531. This is a replacement for PR #3081 (see last paragraph below), which I will close.

Calculation has a property `suppressFormulaErrors`, which really doesn't work as one might expect. If a calculation throws an exception, the setting of this property might prevent the Exception from being thrown, but it will still trigger an Error. I do not think this makes sense, and will change it so the calculation will return `false`, which is part of the original design but which would essentially never happen. This allows the user to save a corrupt spreadsheet, but this was already possible through the use of `setPreCalculateFormulas(false)` on the Writer, so this doesn't really open any new exposures. It nevertheless might be considered a breaking change because of the difference in behavior.

Deprecation - the visibility of the existing property is public, which means it can be changed directly. A new private property is added with a public setter/getter. The new property will be used when the existing property is null (default), which will allow the existing property to be deprecated.

Function getFunctions is changed to static - the array which it returns is static. Existing callers using it as non-static will still function correctly.

Although I am enabling this ability, I don't necessarily think it's a good idea to make use of it. See the original issue for a discussion of why. It is not mentioned in the official documentation, and I will not be adding documentation for it. The originator discovered it by reading the code, and I think that is sufficient for what will often be an ill-advised choice.

Many of the large number of problems with Calculation.php in phpstan baseline are addressed. PR 3081 ran afoul of something in phpstan. The changes in this ticket are more limited, adding a number of doc blocks but leaving executable code unchanged.

phpstan-baseline.neon

@@ -25,71 +25,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Calculation/Calculation.php
 
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:_translateFormulaToEnglish\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:_translateFormulaToEnglish\\(\\) has parameter \\$formula with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:_translateFormulaToLocale\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:_translateFormulaToLocale\\(\\) has parameter \\$formula with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:dataTestReference\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:dataTestReference\\(\\) has parameter \\$operandData with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:getTokensAsString\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:getTokensAsString\\(\\) has parameter \\$tokens with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:localeFunc\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:localeFunc\\(\\) has parameter \\$function with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:validateBinaryOperand\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:validateBinaryOperand\\(\\) has parameter \\$operand with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:validateBinaryOperand\\(\\) has parameter \\$stack with no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
 		-
 			message: "#^Offset 'value' does not exist on array\\|null\\.$#"
 			count: 5
@@ -105,81 +40,11 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Calculation/Calculation.php
 
-		-
-			message: "#^Parameter \\#1 \\$str of function preg_quote expects string, int\\|string given\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
 		-
 			message: "#^Parameter \\#2 \\$worksheet of static method PhpOffice\\\\PhpSpreadsheet\\\\DefinedName\\:\\:resolveName\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet, PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\|null given\\.$#"
 			count: 1
 			path: src/PhpSpreadsheet/Calculation/Calculation.php
 
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$cellStack has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$comparisonOperators has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$controlFunctions has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$cyclicFormulaCell has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$functionReplaceFromExcel has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$functionReplaceFromLocale has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$functionReplaceToExcel has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$functionReplaceToLocale has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$localeFunctions has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$operatorAssociativity has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$operatorPrecedence has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$phpSpreadsheetFunctions has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$returnArrayAsType has no type specified\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Calculation/Calculation.php
-
 		-
 			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:\\$spreadsheet \\(PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\) does not accept PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|null\\.$#"
 			count: 1

src/PhpSpreadsheet/Calculation/Calculation.php

@@ -58,6 +58,7 @@ class Calculation
     const FORMULA_CLOSE_MATRIX_BRACE = '}';
     const FORMULA_STRING_QUOTE = '"';
 
+    /** @var string */
     private static $returnArrayAsType = self::RETURN_ARRAY_AS_VALUE;
 
     /**
@@ -136,9 +137,14 @@ class Calculation
      *        If true, then a user error will be triggered
      *        If false, then an exception will be thrown.
      *
-     * @var bool
+     * @var ?bool
+     *
+     * @deprecated 1.25.2 use setSuppressFormulaErrors() instead
      */
-    public $suppressFormulaErrors = false;
+    public $suppressFormulaErrors;
+
+    /** @var bool */
+    private $suppressFormulaErrorsNew = false;
 
     /**
      * Error message for any error that was raised/thrown by the calculation engine.
@@ -161,6 +167,7 @@ class Calculation
      */
     private $cyclicReferenceStack;
 
+    /** @var array */
     private $cellStack = [];
 
     /**
@@ -172,6 +179,7 @@ class Calculation
      */
     private $cyclicFormulaCounter = 1;
 
+    /** @var string */
     private $cyclicFormulaCell = '';
 
     /**
@@ -205,6 +213,7 @@ class Calculation
      */
     private static $localeArgumentSeparator = ',';
 
+    /** @var array */
     private static $localeFunctions = [];
 
     /**
@@ -230,7 +239,7 @@ class Calculation
         'NULL' => null,
     ];
 
-    // PhpSpreadsheet functions
+    /** @var array */
     private static $phpSpreadsheetFunctions = [
         'ABS' => [
             'category' => Category::CATEGORY_MATH_AND_TRIG,
@@ -2814,7 +2823,11 @@ class Calculation
         ],
     ];
 
-    //    Internal functions used for special control purposes
+    /**
+     *    Internal functions used for special control purposes.
+     *
+     * @var array
+     */
     private static $controlFunctions = [
         'MKMATRIX' => [
             'argumentCount' => '*',
@@ -3243,10 +3256,17 @@ class Calculation
         return $formula;
     }
 
+    /** @var ?array */
     private static $functionReplaceFromExcel;
 
+    /** @var ?array */
     private static $functionReplaceToLocale;
 
+    /**
+     * @param string $formula
+     *
+     * @return string
+     */
     public function _translateFormulaToLocale($formula)
     {
         // Build list of function names and constants for translation
@@ -3279,10 +3299,17 @@ class Calculation
         );
     }
 
+    /** @var ?array */
     private static $functionReplaceFromLocale;
 
+    /** @var ?array */
     private static $functionReplaceToExcel;
 
+    /**
+     * @param string $formula
+     *
+     * @return string
+     */
     public function _translateFormulaToEnglish($formula)
     {
         if (self::$functionReplaceFromLocale === null) {
@@ -3298,7 +3325,6 @@ class Calculation
         if (self::$functionReplaceToExcel === null) {
             self::$functionReplaceToExcel = [];
             foreach (array_keys(self::$localeFunctions) as $excelFunctionName) {
-                // @phpstan-ignore-next-line
                 self::$functionReplaceToExcel[] = '$1' . trim($excelFunctionName) . '$2';
             }
             foreach (array_keys(self::$localeBoolean) as $excelBoolean) {
@@ -3309,6 +3335,11 @@ class Calculation
         return self::translateFormula(self::$functionReplaceFromLocale, self::$functionReplaceToExcel, $formula, self::$localeArgumentSeparator, ',');
     }
 
+    /**
+     * @param string $function
+     *
+     * @return string
+     */
     public static function localeFunc($function)
     {
         if (self::$localeLanguage !== 'en_us') {
@@ -3937,9 +3968,13 @@ class Calculation
         return $formula;
     }
 
-    //    Binary Operators
+    /**
-    //    These operators always work on two values
+     *    Binary Operators.
-    //    Array key is the operator, the value indicates whether this is a left or right associative operator
+     *    These operators always work on two values.
+     *    Array key is the operator, the value indicates whether this is a left or right associative operator.
+     *
+     * @var array
+     */
     private static $operatorAssociativity = [
         '^' => 0, //    Exponentiation
         '*' => 0, '/' => 0, //    Multiplication and Division
@@ -3949,13 +3984,21 @@ class Calculation
         '>' => 0, '<' => 0, '=' => 0, '>=' => 0, '<=' => 0, '<>' => 0, //    Comparison
     ];
 
-    //    Comparison (Boolean) Operators
+    /**
-    //    These operators work on two values, but always return a boolean result
+     *    Comparison (Boolean) Operators.
+     *    These operators work on two values, but always return a boolean result.
+     *
+     * @var array
+     */
     private static $comparisonOperators = ['>' => true, '<' => true, '=' => true, '>=' => true, '<=' => true, '<>' => true];
 
-    //    Operator Precedence
+    /**
-    //    This list includes all valid operators, whether binary (including boolean) or unary (such as %)
+     *    Operator Precedence.
-    //    Array key is the operator, the value is its precedence
+     *    This list includes all valid operators, whether binary (including boolean) or unary (such as %).
+     *    Array key is the operator, the value is its precedence.
+     *
+     * @var array
+     */
     private static $operatorPrecedence = [
         ':' => 9, //    Range
         '∩' => 8, //    Intersect
@@ -4441,6 +4484,11 @@ class Calculation
         return $output;
     }
 
+    /**
+     * @param array $operandData
+     *
+     * @return mixed
+     */
     private static function dataTestReference(&$operandData)
     {
         $operand = $operandData['value'];
@@ -5007,6 +5055,12 @@ class Calculation
         return $output;
     }
 
+    /**
+     * @param mixed $operand
+     * @param mixed $stack
+     *
+     * @return bool
+     */
     private function validateBinaryOperand(&$operand, &$stack)
     {
         if (is_array($operand)) {
@@ -5218,14 +5272,11 @@ class Calculation
     {
         $this->formulaError = $errorMessage;
         $this->cyclicReferenceStack->clear();
-        if (!$this->suppressFormulaErrors) {
+        $suppress = $this->suppressFormulaErrors ?? $this->suppressFormulaErrorsNew;
+        if (!$suppress) {
             throw new Exception($errorMessage);
         }
 
-        if (strlen($errorMessage) > 0) {
-            trigger_error($errorMessage, E_USER_ERROR);
-        }
-
         return false;
     }
 
@@ -5360,7 +5411,7 @@ class Calculation
     /**
      * Get a list of all implemented functions as an array of function objects.
      */
-    public function getFunctions(): array
+    public static function getFunctions(): array
     {
         return self::$phpSpreadsheetFunctions;
     }
@@ -5461,6 +5512,11 @@ class Calculation
         return $args;
     }
 
+    /**
+     * @param array $tokens
+     *
+     * @return string
+     */
     private function getTokensAsString($tokens)
     {
         $tokensStr = array_map(function ($token) {
@@ -5527,4 +5583,14 @@ class Calculation
 
         return $result;
     }
+
+    public function setSuppressFormulaErrors(bool $suppressFormulaErrors): void
+    {
+        $this->suppressFormulaErrorsNew = $suppressFormulaErrors;
+    }
+
+    public function getSuppressFormulaErrors(): bool
+    {
+        return $this->suppressFormulaErrorsNew;
+    }
 }

tests/PhpSpreadsheetTests/Calculation/CalculationErrorTest.php

@@ -5,50 +5,26 @@ namespace PhpOffice\PhpSpreadsheetTests\Calculation;
 use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
 use PhpOffice\PhpSpreadsheet\Calculation\Exception as CalcException;
 use PHPUnit\Framework\TestCase;
-use Throwable;
 
 class CalculationErrorTest extends TestCase
 {
+    public function testCalculationExceptionSuppressed(): void
+    {
+        $calculation = Calculation::getInstance();
+        self::assertFalse($calculation->getSuppressFormulaErrors());
+        $calculation->setSuppressFormulaErrors(true);
+        $result = $calculation->_calculateFormulaValue('=SUM(');
+        $calculation->setSuppressFormulaErrors(false);
+        self::assertFalse($result);
+    }
+
     public function testCalculationException(): void
     {
+        $calculation = Calculation::getInstance();
+        self::assertFalse($calculation->getSuppressFormulaErrors());
         $this->expectException(CalcException::class);
-        $this->expectExceptionMessage('Formula Error:');
+        $this->expectExceptionMessage("Formula Error: Expecting ')'");
-        $calculation = Calculation::getInstance();
         $result = $calculation->_calculateFormulaValue('=SUM(');
         self::assertFalse($result);
     }
-
-    public function testCalculationError(): void
-    {
-        $calculation = Calculation::getInstance();
-        $calculation->suppressFormulaErrors = true;
-        $error = false;
-
-        try {
-            $calculation->_calculateFormulaValue('=SUM(');
-        } catch (Throwable $e) {
-            self::assertSame("Formula Error: Expecting ')'", $e->getMessage());
-            self::assertSame('PHPUnit\\Framework\\Error\\Error', get_class($e));
-            $error = true;
-        }
-        self::assertTrue($error);
-    }
-
-    /**
-     * @param mixed $args
-     */
-    public static function errhandler2(...$args): bool
-    {
-        return $args[0] === E_USER_ERROR;
-    }
-
-    public function testCalculationErrorTrulySuppressed(): void
-    {
-        $calculation = Calculation::getInstance();
-        $calculation->suppressFormulaErrors = true;
-        set_error_handler([self::class, 'errhandler2']);
-        $result = $calculation->_calculateFormulaValue('=SUM(');
-        restore_error_handler();
-        self::assertFalse($result);
-    }
 }

tests/PhpSpreadsheetTests/LocaleGeneratorTest.php

@@ -5,7 +5,6 @@ namespace PhpOffice\PhpSpreadsheetTests;
 use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
 use PhpOffice\PhpSpreadsheetInfra\LocaleGenerator;
 use PHPUnit\Framework\TestCase;
-use ReflectionClass;
 
 class LocaleGeneratorTest extends TestCase
 {
@@ -13,10 +12,7 @@ class LocaleGeneratorTest extends TestCase
     {
         $directory = realpath(__DIR__ . '/../../src/PhpSpreadsheet/Calculation/locale/') ?: '';
         self::assertNotEquals('', $directory);
-        $phpSpreadsheetFunctionsProperty = (new ReflectionClass(Calculation::class))
+        $phpSpreadsheetFunctions = Calculation::getFunctions();
-            ->getProperty('phpSpreadsheetFunctions');
-        $phpSpreadsheetFunctionsProperty->setAccessible(true);
-        $phpSpreadsheetFunctions = $phpSpreadsheetFunctionsProperty->getValue();
 
         $localeGenerator = new LocaleGenerator(
             $directory . DIRECTORY_SEPARATOR,

tests/PhpSpreadsheetTests/Writer/Xlsx/CalculationErrorTest.php

@@ -0,0 +1,64 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
+
+use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
+use PhpOffice\PhpSpreadsheet\Calculation\Exception as CalcException;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
+
+class CalculationErrorTest extends AbstractFunctional
+{
+    /** @var ?Spreadsheet */
+    private $spreadsheet;
+
+    /** @var ?Spreadsheet */
+    private $reloadedSpreadsheet;
+
+    protected function tearDown(): void
+    {
+        if ($this->spreadsheet !== null) {
+            $this->spreadsheet->disconnectWorksheets();
+            $this->spreadsheet = null;
+        }
+        if ($this->reloadedSpreadsheet !== null) {
+            $this->reloadedSpreadsheet->disconnectWorksheets();
+            $this->reloadedSpreadsheet = null;
+        }
+    }
+
+    public function testCalculationExceptionSuppressed(): void
+    {
+        $this->spreadsheet = new Spreadsheet();
+        $sheet = $this->spreadsheet->getActiveSheet();
+        $calculation = Calculation::getInstance($this->spreadsheet);
+        self::assertFalse($calculation->getSuppressFormulaErrors());
+        $calculation->setSuppressFormulaErrors(true);
+        $sheet->getCell('A1')->setValue('=SUM(');
+        $sheet->getCell('A2')->setValue('=2+3');
+        $this->reloadedSpreadsheet = $this->writeAndReload($this->spreadsheet, 'Xlsx');
+        $rcalculation = Calculation::getInstance($this->reloadedSpreadsheet);
+        self::assertFalse($rcalculation->getSuppressFormulaErrors());
+        $rcalculation->setSuppressFormulaErrors(true);
+        $rsheet = $this->reloadedSpreadsheet->getActiveSheet();
+        self::assertSame('=SUM(', $rsheet->getCell('A1')->getValue());
+        self::assertFalse($rsheet->getCell('A1')->getCalculatedValue());
+        self::assertSame('=2+3', $rsheet->getCell('A2')->getValue());
+        self::assertSame(5, $rsheet->getCell('A2')->getCalculatedValue());
+        $calculation->setSuppressFormulaErrors(false);
+        $rcalculation->setSuppressFormulaErrors(false);
+    }
+
+    public function testCalculationException(): void
+    {
+        $this->expectException(CalcException::class);
+        $this->expectExceptionMessage("Formula Error: Expecting ')'");
+        $this->spreadsheet = new Spreadsheet();
+        $sheet = $this->spreadsheet->getActiveSheet();
+        $calculation = Calculation::getInstance($this->spreadsheet);
+        self::assertFalse($calculation->getSuppressFormulaErrors());
+        $sheet->getCell('A1')->setValue('=SUM(');
+        $sheet->getCell('A2')->setValue('=2+3');
+        $this->reloadedSpreadsheet = $this->writeAndReload($this->spreadsheet, 'Xlsx');
+    }
+}