From e02eab29f13ed55c4775408de74b393c519526e9 Mon Sep 17 00:00:00 2001 From: oleibman Date: Sat, 11 Sep 2021 06:55:00 -0700 Subject: [PATCH] Validate Input to SetSelectedCells (#2280) * Validate Input to SetSelectedCells See issue #2279. User requests an enhancement so that you can set a Style on a Named Range. The attempt is failing because setting the style causes a call to setSelectedCells, which does not account for Named Ranges. Although not related to the issue, it is worth noting that setSelectedCells does nothing to attempt to validate its input. The request seems reasonable, even if it is probably more than Excel itself offers. I have added code to setSelectedCells to recognize Named Ranges (if and only if they are defined on the sheet in question). It will throw an exception if the string passed as coordinates cannot be parsed as a range of cells or an appropriate Named Range, e.e.g. a Named Range on a different sheet, a non-existent named range, named formulas, formulas, use of sheet name qualifiers (even for the same sheet). Tests are, of course, added for all of those and for the original issue. The code in setSelectedCells is tested in a very large number of cases in the test suite, none of which showed any problems after this change. * Scrutinizer 2 minor (non-fatal) corrections, including 1 where Phpstan and Scrutinizer have a different idea about return values from preg_replace. --- phpstan-baseline.neon | 25 ---- src/PhpSpreadsheet/Worksheet/Worksheet.php | 47 ++++++- tests/PhpSpreadsheetTests/NamedRange2Test.php | 120 ++++++++++++++++++ 3 files changed, 161 insertions(+), 31 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/NamedRange2Test.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index c997788a8f..b55850e832 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -4825,31 +4825,6 @@ parameters: count: 2 path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - - message: "#^Parameter \\#3 \\$subject of function preg_replace expects array\\|string, string\\|null given\\.$#" - count: 3 - path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - - - message: "#^Parameter \\#1 \\$coord of static method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Coordinate\\:\\:coordinateIsRange\\(\\) expects string, string\\|null given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - - - message: "#^Parameter \\#1 \\$pRange of static method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Coordinate\\:\\:splitRange\\(\\) expects string, string\\|null given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\:\\:\\$activeCell \\(string\\) does not accept string\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\:\\:\\$selectedCells \\(string\\) does not accept string\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Worksheet/Worksheet.php - - message: "#^Cannot call method getValue\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#" count: 4 diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php index 8dd16df033..29522463aa 100644 --- a/src/PhpSpreadsheet/Worksheet/Worksheet.php +++ b/src/PhpSpreadsheet/Worksheet/Worksheet.php @@ -2373,6 +2373,38 @@ public function setSelectedCell($pCoordinate) return $this->setSelectedCells($pCoordinate); } + /** + * Sigh - Phpstan thinks, correctly, that preg_replace can return null. + * But Scrutinizer doesn't. Try to satisfy both. + * + * @param mixed $str + */ + private static function ensureString($str): string + { + return is_string($str) ? $str : ''; + } + + private static function pregReplace(string $pattern, string $replacement, string $subject): string + { + return self::ensureString(preg_replace($pattern, $replacement, $subject)); + } + + private function tryDefinedName(string $pCoordinate): string + { + // Uppercase coordinate + $pCoordinate = strtoupper($pCoordinate); + // Eliminate leading equal sign + $pCoordinate = self::pregReplace('/^=/', '', $pCoordinate); + $defined = $this->parent->getDefinedName($pCoordinate, $this); + if ($defined !== null) { + if ($defined->getWorksheet() === $this && !$defined->isFormula()) { + $pCoordinate = self::pregReplace('/^=/', '', $defined->getValue()); + } + } + + return $pCoordinate; + } + /** * Select a range of cells. * @@ -2382,20 +2414,23 @@ public function setSelectedCell($pCoordinate) */ public function setSelectedCells($pCoordinate) { - // Uppercase coordinate - $pCoordinate = strtoupper($pCoordinate); + $originalCoordinate = $pCoordinate; + $pCoordinate = $this->tryDefinedName($pCoordinate); // Convert 'A' to 'A:A' - $pCoordinate = preg_replace('/^([A-Z]+)$/', '${1}:${1}', $pCoordinate); + $pCoordinate = self::pregReplace('/^([A-Z]+)$/', '${1}:${1}', $pCoordinate); // Convert '1' to '1:1' - $pCoordinate = preg_replace('/^(\d+)$/', '${1}:${1}', $pCoordinate); + $pCoordinate = self::pregReplace('/^(\d+)$/', '${1}:${1}', $pCoordinate); // Convert 'A:C' to 'A1:C1048576' - $pCoordinate = preg_replace('/^([A-Z]+):([A-Z]+)$/', '${1}1:${2}1048576', $pCoordinate); + $pCoordinate = self::pregReplace('/^([A-Z]+):([A-Z]+)$/', '${1}1:${2}1048576', $pCoordinate); // Convert '1:3' to 'A1:XFD3' - $pCoordinate = preg_replace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate); + $pCoordinate = self::pregReplace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate); + if (preg_match('/^\\$?[A-Z]{1,3}\\$?\d{1,7}(:\\$?[A-Z]{1,3}\\$?\d{1,7})?$/', $pCoordinate) !== 1) { + throw new Exception("Invalid setSelectedCells $originalCoordinate $pCoordinate"); + } if (Coordinate::coordinateIsRange($pCoordinate)) { [$first] = Coordinate::splitRange($pCoordinate); diff --git a/tests/PhpSpreadsheetTests/NamedRange2Test.php b/tests/PhpSpreadsheetTests/NamedRange2Test.php new file mode 100644 index 0000000000..2082f479dd --- /dev/null +++ b/tests/PhpSpreadsheetTests/NamedRange2Test.php @@ -0,0 +1,120 @@ +spreadsheet = new Spreadsheet(); + + $worksheet1 = $spreadsheet->getActiveSheet(); + $worksheet1->setTitle('SheetOne'); + $spreadsheet->addNamedRange( + new NamedRange('FirstRel', $worksheet1, '=A1') + ); + $spreadsheet->addNamedRange( + new NamedRange('FirstAbs', $worksheet1, '=$B$1') + ); + $spreadsheet->addNamedRange( + new NamedRange('FirstRelMult', $worksheet1, '=C1:D2') + ); + $spreadsheet->addNamedRange( + new NamedRange('FirstAbsMult', $worksheet1, '$E$3:$F$4') + ); + + $worksheet2 = $spreadsheet->createSheet(); + $worksheet2->setTitle('SheetTwo'); + $spreadsheet->addNamedRange( + new NamedRange('SecondRel', $worksheet2, '=A1') + ); + $spreadsheet->addNamedRange( + new NamedRange('SecondAbs', $worksheet2, '=$B$1') + ); + $spreadsheet->addNamedRange( + new NamedRange('SecondRelMult', $worksheet2, '=C1:D2') + ); + $spreadsheet->addNamedRange( + new NamedRange('SecondAbsMult', $worksheet2, '$E$3:$F$4') + ); + + $spreadsheet->addDefinedName(DefinedName::createInstance('FirstFormula', $worksheet1, '=TODAY()-1')); + $spreadsheet->addDefinedName(DefinedName::createInstance('SecondFormula', $worksheet2, '=TODAY()-2')); + + $this->spreadsheet->setActiveSheetIndex(0); + } + + protected function tearDown(): void + { + if ($this->spreadsheet !== null) { + $this->spreadsheet->disconnectWorksheets(); + $this->spreadsheet = null; + } + } + + private function getSpreadsheet(): Spreadsheet + { + if ($this->spreadsheet !== null) { + return $this->spreadsheet; + } + $this->spreadsheet = new Spreadsheet(); + + return $this->spreadsheet; + } + + public function testNamedRangeSetStyle(): void + { + $spreadsheet = $this->getSpreadsheet(); + $sheet = $spreadsheet->getSheet(0); + $sheet->getStyle('FirstRel')->getNumberFormat()->setFormatCode('yyyy-mm-dd'); + self::assertSame('yyyy-mm-dd', $sheet->getStyle('A1')->getNumberFormat()->getFormatCode()); + $sheet->getStyle('FirstAbs')->getFont()->setName('Georgia'); + self::assertSame('Georgia', $sheet->getStyle('B1')->getFont()->getName()); + $sheet->getStyle('FirstRelMult')->getFont()->setItalic(true); + self::assertTrue($sheet->getStyle('D2')->getFont()->getItalic()); + $sheet->getStyle('FirstAbsMult')->getFill()->setFillType('gray125'); + self::assertSame('gray125', $sheet->getStyle('F4')->getFill()->getFillType()); + self::assertSame('none', $sheet->getStyle('A1')->getFill()->getFillType()); + $sheet = $spreadsheet->getSheet(1); + $sheet->getStyle('SecondAbsMult')->getFill()->setFillType('lightDown'); + self::assertSame('lightDown', $sheet->getStyle('E3')->getFill()->getFillType()); + } + + /** + * @dataProvider providerRangeOrFormula + */ + public function testNamedRangeSetStyleBad(string $name): void + { + $this->expectException(Except::class); + $spreadsheet = $this->getSpreadsheet(); + $sheet = $spreadsheet->getSheet(0); + $sheet->getStyle($name)->getNumberFormat()->setFormatCode('yyyy-mm-dd'); + self::assertSame('yyyy-mm-dd', $sheet->getStyle('A1')->getNumberFormat()->getFormatCode()); + } + + public function providerRangeOrFormula(): array + { + return [ + 'wrong sheet rel' => ['SecondRel'], + 'wrong sheet abs' => ['SecondAbs'], + 'wrong sheet relmult' => ['SecondRelMult'], + 'wrong sheet absmult' => ['SecondAbsMult'], + 'wrong sheet formula' => ['SecondFormula'], + 'right sheet formula' => ['FirstFormula'], + 'non-existent name' => ['NonExistentName'], + 'this sheet name' => ['SheetOne!G7'], + 'other sheet name' => ['SheetTwo!G7'], + 'non-existent sheet name' => ['SheetAbc!G7'], + 'unnamed formula' => ['=2+3'], + ]; + } +}