From e40438916f5c29d13f8f98d175a0595b5b7b0422 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Tue, 25 Jun 2024 22:06:36 -0700 Subject: [PATCH 1/3] Change Style Without Affecting Current Cell/Sheet, and Invalid Formulas Fix #1310, which was closed as stale in 2020, but which I will now reopen. Supersedes PR #1311 (@jaiminmoslake7020), from which I will remove the stale label but leave closed. The issue and the PR were too limited - they detected that the use of two equal signs at the start of a string made for an invalid formula, but there are variations, trivial and otherwise, which might also be detected. Using `setValue` with a string which starts with an equal sign will now attempt to parse (not evaluate) the formula; for certain situations in which the parser throws an exception, the string will be treated as a string rather than a formula. An example where it will still be treated as a formula is a 3D range reference, where the problem is not that it can't be parsed, but rather that the formula isn't supported (see unit test Calculation/Engine/RangeTest::test3dRangeEvaluation). Allowing such a formula might cause problems later on, but that is already what happens. A string beginning with an equal sign but which isn't treated as a formula will automatically set the `quotePrefix` attribute to `true`; all other `setValue` attempts will set it to `false`. This avoids the problem of a lingering value causing problems later on. It has long been a matter of discontent that setting a style can change the selected cells. A new method is added to `Worksheet`: ```php applyStylesFromArray(string $coordinate, array $styleArray) ``` This will attempt to guarantee that the active sheet in the current spreadsheet, and the selected cells in the current worksheet, remain undisturbed after the call. The setting of `quotePrefix` above is the first use of the new method. --- .../Calculation/Calculation.php | 2 +- src/PhpSpreadsheet/Cell/Cell.php | 6 +++ .../Cell/DefaultValueBinder.php | 40 +++++++++++++++---- src/PhpSpreadsheet/Cell/StringValueBinder.php | 7 +--- src/PhpSpreadsheet/Worksheet/Worksheet.php | 15 +++++++ .../Calculation/Engine/RangeTest.php | 35 ++++++++++++++-- .../Cell/AdvancedValueBinderTest.php | 28 +++++++++++++ .../Cell/StringValueBinderTest.php | 8 +++- tests/data/Cell/DefaultValueBinder.php | 3 ++ 9 files changed, 126 insertions(+), 18 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 6028a9d72f..f445717d06 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -4065,7 +4065,7 @@ private function internalParseFormula(string $formula, ?Cell $cell = null): bool $opCharacter = $formula[$index]; // Get the first character of the value at the current index position // Check for two-character operators (e.g. >=, <=, <>) - if ((isset(self::$comparisonOperators[$opCharacter])) && (strlen($formula) > $index) && (isset(self::$comparisonOperators[$formula[$index + 1]]))) { + if ((isset(self::$comparisonOperators[$opCharacter])) && (strlen($formula) > $index) && isset($formula[$index + 1], self::$comparisonOperators[$formula[$index + 1]])) { $opCharacter .= $formula[++$index]; } // Find out if we're currently at the beginning of a number, variable, cell/row/column reference, diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php index 3dc6c2eab6..bf0b681ac1 100644 --- a/src/PhpSpreadsheet/Cell/Cell.php +++ b/src/PhpSpreadsheet/Cell/Cell.php @@ -248,6 +248,7 @@ public function setValue(mixed $value, ?IValueBinder $binder = null): self public function setValueExplicit(mixed $value, string $dataType = DataType::TYPE_STRING): self { $oldValue = $this->value; + $quotePrefix = false; // set the value according to data type switch ($dataType) { @@ -260,6 +261,10 @@ public function setValueExplicit(mixed $value, string $dataType = DataType::TYPE // no break case DataType::TYPE_STRING: // Synonym for string + if (is_string($value) && strlen($value) > 1 && $value[0] === '=') { + $quotePrefix = true; + } + // no break case DataType::TYPE_INLINE: // Rich text $this->value = DataType::checkString($value); @@ -299,6 +304,7 @@ public function setValueExplicit(mixed $value, string $dataType = DataType::TYPE $this->updateInCollection(); $cellCoordinate = $this->getCoordinate(); self::updateIfCellIsTableHeader($this->getParent()?->getParent(), $this, $oldValue, $value); + $this->getWorksheet()->applyStylesFromArray($cellCoordinate, ['quotePrefix' => $quotePrefix]); return $this->getParent()?->get($cellCoordinate) ?? $this; } diff --git a/src/PhpSpreadsheet/Cell/DefaultValueBinder.php b/src/PhpSpreadsheet/Cell/DefaultValueBinder.php index 2710ead434..da5138e7d2 100644 --- a/src/PhpSpreadsheet/Cell/DefaultValueBinder.php +++ b/src/PhpSpreadsheet/Cell/DefaultValueBinder.php @@ -3,6 +3,8 @@ namespace PhpOffice\PhpSpreadsheet\Cell; use DateTimeInterface; +use PhpOffice\PhpSpreadsheet\Calculation\Calculation; +use PhpOffice\PhpSpreadsheet\Calculation\Exception as CalculationException; use PhpOffice\PhpSpreadsheet\Exception as SpreadsheetException; use PhpOffice\PhpSpreadsheet\RichText\RichText; use PhpOffice\PhpSpreadsheet\Shared\StringHelper; @@ -46,17 +48,40 @@ public static function dataTypeForValue(mixed $value): string // Match the value against a few data types if ($value === null) { return DataType::TYPE_NULL; - } elseif (is_float($value) || is_int($value)) { + } + if (is_float($value) || is_int($value)) { return DataType::TYPE_NUMERIC; - } elseif (is_bool($value)) { + } + if (is_bool($value)) { return DataType::TYPE_BOOL; - } elseif ($value === '') { + } + if ($value === '') { return DataType::TYPE_STRING; - } elseif ($value instanceof RichText) { + } + if ($value instanceof RichText) { return DataType::TYPE_INLINE; - } elseif (is_string($value) && strlen($value) > 1 && $value[0] === '=') { + } + if (is_string($value) && strlen($value) > 1 && $value[0] === '=') { + $calculation = new Calculation(); + $calculation->disableBranchPruning(); + + try { + if (empty($calculation->parseFormula($value))) { + return DataType::TYPE_STRING; + } + } catch (CalculationException $e) { + $message = $e->getMessage(); + if ( + $message === 'Formula Error: An unexpected error occurred' + || str_contains($message, 'has no operands') + ) { + return DataType::TYPE_STRING; + } + } + return DataType::TYPE_FORMULA; - } elseif (preg_match('/^[\+\-]?(\d+\\.?\d*|\d*\\.?\d+)([Ee][\-\+]?[0-2]?\d{1,3})?$/', $value)) { + } + if (preg_match('/^[\+\-]?(\d+\\.?\d*|\d*\\.?\d+)([Ee][\-\+]?[0-2]?\d{1,3})?$/', $value)) { $tValue = ltrim($value, '+-'); if (is_string($value) && strlen($tValue) > 1 && $tValue[0] === '0' && $tValue[1] !== '.') { return DataType::TYPE_STRING; @@ -67,7 +92,8 @@ public static function dataTypeForValue(mixed $value): string } return DataType::TYPE_NUMERIC; - } elseif (is_string($value)) { + } + if (is_string($value)) { $errorCodes = DataType::getErrorCodes(); if (isset($errorCodes[$value])) { return DataType::TYPE_ERROR; diff --git a/src/PhpSpreadsheet/Cell/StringValueBinder.php b/src/PhpSpreadsheet/Cell/StringValueBinder.php index 6ff258d93c..d86cdabd33 100644 --- a/src/PhpSpreadsheet/Cell/StringValueBinder.php +++ b/src/PhpSpreadsheet/Cell/StringValueBinder.php @@ -8,7 +8,7 @@ use PhpOffice\PhpSpreadsheet\Shared\StringHelper; use Stringable; -class StringValueBinder implements IValueBinder +class StringValueBinder extends DefaultValueBinder implements IValueBinder { protected bool $convertNull = true; @@ -87,12 +87,9 @@ public function bindValue(Cell $cell, mixed $value): bool $cell->setValueExplicit($value, DataType::TYPE_BOOL); } elseif ((is_int($value) || is_float($value)) && $this->convertNumeric === false) { $cell->setValueExplicit($value, DataType::TYPE_NUMERIC); - } elseif (is_string($value) && strlen($value) > 1 && $value[0] === '=' && $this->convertFormula === false) { + } elseif (is_string($value) && strlen($value) > 1 && $value[0] === '=' && $this->convertFormula === false && parent::dataTypeForValue($value) === DataType::TYPE_FORMULA) { $cell->setValueExplicit($value, DataType::TYPE_FORMULA); } else { - if (is_string($value) && strlen($value) > 1 && $value[0] === '=') { - $cell->getStyle()->setQuotePrefix(true); - } $cell->setValueExplicit((string) $value, DataType::TYPE_STRING); } diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php index 8adada4c11..0bb64ba594 100644 --- a/src/PhpSpreadsheet/Worksheet/Worksheet.php +++ b/src/PhpSpreadsheet/Worksheet/Worksheet.php @@ -3673,4 +3673,19 @@ public function copyCells(string $fromCell, string $toCells, bool $copyStyle = t } } } + + public function applyStylesFromArray(string $coordinate, array $styleArray): bool + { + $spreadsheet = $this->parent; + if ($spreadsheet === null) { + return false; + } + $activeSheetIndex = $spreadsheet->getActiveSheetIndex(); + $originalSelected = $this->selectedCells; + $this->getStyle($coordinate)->applyFromArray($styleArray); + $this->selectedCells = $originalSelected; + $spreadsheet->setActiveSheetIndex($activeSheetIndex); + + return true; + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php index fbac0ff91e..aa7bc529d9 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php @@ -4,6 +4,7 @@ namespace PhpOffice\PhpSpreadsheetTests\Calculation\Engine; +use PhpOffice\PhpSpreadsheet\Calculation\Calculation; use PhpOffice\PhpSpreadsheet\Calculation\Information\ExcelError; use PhpOffice\PhpSpreadsheet\NamedRange; use PhpOffice\PhpSpreadsheet\Spreadsheet; @@ -13,13 +14,23 @@ class RangeTest extends TestCase { private string $incompleteMessage = 'Must be revisited'; - private Spreadsheet $spreadSheet; + private ?Spreadsheet $spreadSheet = null; - protected function setUp(): void + protected function getSpreadsheet(): Spreadsheet { - $this->spreadSheet = new Spreadsheet(); - $this->spreadSheet->getActiveSheet() + $spreadsheet = new Spreadsheet(); + $spreadsheet->getActiveSheet() ->fromArray(array_chunk(range(1, 240), 6), null, 'A1', true); + + return $spreadsheet; + } + + protected function tearDown(): void + { + if ($this->spreadSheet !== null) { + $this->spreadSheet->disconnectWorksheets(); + $this->spreadSheet = null; + } } /** @@ -27,6 +38,7 @@ protected function setUp(): void */ public function testRangeEvaluation(string $formula, int|string $expectedResult): void { + $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); $workSheet->setCellValue('H1', $formula); @@ -64,8 +76,20 @@ public static function providerRangeEvaluation(): array ]; } + public function test3dRangeParsing(): void + { + // This test shows that parsing throws exception. + // Next test shows that formula is still treated as a formula + // despite the parse failure. + $this->expectExceptionMessage('3D Range references are not yet supported'); + $calculation = new Calculation(); + $calculation->disableBranchPruning(); + $calculation->parseFormula('=SUM(Worksheet!A1:Worksheet2!B3'); + } + public function test3dRangeEvaluation(): void { + $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); $workSheet->setCellValue('E1', '=SUM(Worksheet!A1:Worksheet2!B3)'); @@ -78,6 +102,7 @@ public function test3dRangeEvaluation(): void */ public function testNamedRangeEvaluation(array $ranges, string $formula, int $expectedResult): void { + $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); foreach ($ranges as $id => $range) { $this->spreadSheet->addNamedRange(new NamedRange('GROUP' . ++$id, $workSheet, $range)); @@ -116,6 +141,7 @@ public static function providerNamedRangeEvaluation(): array */ public function testUTF8NamedRangeEvaluation(array $names, array $ranges, string $formula, int $expectedResult): void { + $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); foreach ($names as $index => $name) { $range = $ranges[$index]; @@ -144,6 +170,7 @@ public function testCompositeNamedRangeEvaluation(string $composite, int $expect if ($this->incompleteMessage !== '') { self::markTestIncomplete($this->incompleteMessage); } + $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); $this->spreadSheet->addNamedRange(new NamedRange('COMPOSITE', $workSheet, $composite)); diff --git a/tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php b/tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php index d3f0ad6643..d8de97a332 100644 --- a/tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php +++ b/tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php @@ -6,6 +6,7 @@ use PhpOffice\PhpSpreadsheet\Cell\AdvancedValueBinder; use PhpOffice\PhpSpreadsheet\Cell\Cell; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Cell\IValueBinder; use PhpOffice\PhpSpreadsheet\Settings; use PhpOffice\PhpSpreadsheet\Shared\StringHelper; @@ -232,4 +233,31 @@ public static function stringProvider(): array ["Hello\nWorld", true], ]; } + + /** + * @dataProvider formulaProvider + */ + public function testFormula(string $value, string $dataType): void + { + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + + $sheet->getCell('A1')->setValue($value); + self::assertSame($dataType, $sheet->getCell('A1')->getDataType()); + if ($dataType === DataType::TYPE_FORMULA) { + self::assertFalse($sheet->getStyle('A1')->getQuotePrefix()); + } else { + self::assertTrue($sheet->getStyle('A1')->getQuotePrefix()); + } + + $spreadsheet->disconnectWorksheets(); + } + + public static function formulaProvider(): array + { + return [ + 'normal formula' => ['=SUM(A1:C3)', DataType::TYPE_FORMULA], + 'issue 1310' => ['======', DataType::TYPE_STRING], + ]; + } } diff --git a/tests/PhpSpreadsheetTests/Cell/StringValueBinderTest.php b/tests/PhpSpreadsheetTests/Cell/StringValueBinderTest.php index 43ac2fbd1e..71006fff8a 100644 --- a/tests/PhpSpreadsheetTests/Cell/StringValueBinderTest.php +++ b/tests/PhpSpreadsheetTests/Cell/StringValueBinderTest.php @@ -211,13 +211,19 @@ public function testStringValueBinderSuppressFormulaConversion( $cell->setValue($value); self::assertSame($expectedValue, $cell->getValue()); self::assertSame($expectedDataType, $cell->getDataType()); + if ($expectedDataType === DataType::TYPE_FORMULA) { + self::assertFalse($sheet->getStyle('A1')->getQuotePrefix()); + } else { + self::assertTrue($sheet->getStyle('A1')->getQuotePrefix()); + } $spreadsheet->disconnectWorksheets(); } public static function providerDataValuesSuppressFormulaConversion(): array { return [ - ['=SUM(A1:C3)', '=SUM(A1:C3)', DataType::TYPE_FORMULA, false], + 'normal formula' => ['=SUM(A1:C3)', '=SUM(A1:C3)', DataType::TYPE_FORMULA], + 'issue 1310' => ['======', '======', DataType::TYPE_STRING], ]; } diff --git a/tests/data/Cell/DefaultValueBinder.php b/tests/data/Cell/DefaultValueBinder.php index b9f3d51e11..bd4295c0b2 100644 --- a/tests/data/Cell/DefaultValueBinder.php +++ b/tests/data/Cell/DefaultValueBinder.php @@ -83,4 +83,7 @@ 's', '1234567890123459012345689012345690', ], + 'Issue 1310 Multiple = at start' => ['s', '======'], + 'Issue 1310 Variant 1' => ['s', '= ====='], + 'Issue 1310 Variant 2' => ['s', '=2*3='], ]; From 352872048be062f5a0b78cb179df23928a0b4a9c Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Tue, 25 Jun 2024 23:16:10 -0700 Subject: [PATCH 2/3] Resolve Merge Conflicts --- .../Cell/DefaultValueBinder.php | 18 ++++++++++----- .../Cell/DefaultValueBinderTest.php | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/PhpSpreadsheet/Cell/DefaultValueBinder.php b/src/PhpSpreadsheet/Cell/DefaultValueBinder.php index da5138e7d2..e6f97317e4 100644 --- a/src/PhpSpreadsheet/Cell/DefaultValueBinder.php +++ b/src/PhpSpreadsheet/Cell/DefaultValueBinder.php @@ -61,7 +61,15 @@ public static function dataTypeForValue(mixed $value): string if ($value instanceof RichText) { return DataType::TYPE_INLINE; } - if (is_string($value) && strlen($value) > 1 && $value[0] === '=') { + if ($value instanceof Stringable) { + $value = (string) $value; + } + if (!is_string($value)) { + $gettype = is_object($value) ? get_class($value) : gettype($value); + + throw new SpreadsheetException("unusable type $gettype"); + } + if (strlen($value) > 1 && $value[0] === '=') { $calculation = new Calculation(); $calculation->disableBranchPruning(); @@ -93,11 +101,9 @@ public static function dataTypeForValue(mixed $value): string return DataType::TYPE_NUMERIC; } - if (is_string($value)) { - $errorCodes = DataType::getErrorCodes(); - if (isset($errorCodes[$value])) { - return DataType::TYPE_ERROR; - } + $errorCodes = DataType::getErrorCodes(); + if (isset($errorCodes[$value])) { + return DataType::TYPE_ERROR; } return DataType::TYPE_STRING; diff --git a/tests/PhpSpreadsheetTests/Cell/DefaultValueBinderTest.php b/tests/PhpSpreadsheetTests/Cell/DefaultValueBinderTest.php index fb9b42dbe4..8cb5d8876a 100644 --- a/tests/PhpSpreadsheetTests/Cell/DefaultValueBinderTest.php +++ b/tests/PhpSpreadsheetTests/Cell/DefaultValueBinderTest.php @@ -108,4 +108,27 @@ public function testCanOverrideStaticMethodWithoutOverridingBindValue(): void self::assertTrue($binder::$called); $spreadsheet->disconnectWorksheets(); } + + public function testDataTypeForValueExceptions(): void + { + try { + self::assertSame('s', DefaultValueBinder::dataTypeForValue(new SpreadsheetException())); + } catch (SpreadsheetException $e) { + self::fail('Should not have failed for stringable'); + } + + try { + DefaultValueBinder::dataTypeForValue([]); + self::fail('Should have failed for array'); + } catch (SpreadsheetException $e) { + self::assertStringContainsString('unusable type array', $e->getMessage()); + } + + try { + DefaultValueBinder::dataTypeForValue(new DateTime()); + self::fail('Should have failed for DateTime'); + } catch (SpreadsheetException $e) { + self::assertStringContainsString('unusable type DateTime', $e->getMessage()); + } + } } From b8715a5d8a175a52d24d67c1c33fc661afb10cd4 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 5 Jul 2024 22:20:57 -0700 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 229a320227..bffebad017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Added - Xlsx Reader Optionally Ignore Rows With No Cells. [Issue #3982](https://github.com/PHPOffice/PhpSpreadsheet/issues/3982) [PR #4035](https://github.com/PHPOffice/PhpSpreadsheet/pull/4035) +- Means to change style without affecting current cell/sheet. [PR #4073](https://github.com/PHPOffice/PhpSpreadsheet/pull/4073) ### Changed @@ -36,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). - Csv Reader allow use of html mimetype. [Issue #4036](https://github.com/PHPOffice/PhpSpreadsheet/issues/4036) [PR #4049](https://github.com/PHPOffice/PhpSpreadsheet/pull/4040) - More RTL in Xlsx/Html Comments [Issue #4004](https://github.com/PHPOffice/PhpSpreadsheet/issues/4004) [PR #4065](https://github.com/PHPOffice/PhpSpreadsheet/pull/4065) - Empty String in sharedStrings. [Issue #4063](https://github.com/PHPOffice/PhpSpreadsheet/issues/4063) [PR #4064](https://github.com/PHPOffice/PhpSpreadsheet/pull/4064) +- Treat invalid formulas as strings. [Issue #1310](https://github.com/PHPOffice/PhpSpreadsheet/issues/1310) [PR #4073](https://github.com/PHPOffice/PhpSpreadsheet/pull/4073) ## 2024-05-11 - 2.1.0