Skip to content

Commit e02eab2

Browse files
oleibmanoleibman
authored
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.
1 parent bc9234e commit e02eab2

File tree

3 files changed

+161
-31
lines changed

3 files changed

+161
-31
lines changed

‎phpstan-baseline.neon

-25
Original file line numberDiff line numberDiff line change
@@ -4825,31 +4825,6 @@ parameters:
48254825
count: 2
48264826
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
48274827

4828-
-
4829-
message: "#^Parameter \\#3 \\$subject of function preg_replace expects array\\|string, string\\|null given\\.$#"
4830-
count: 3
4831-
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
4832-
4833-
-
4834-
message: "#^Parameter \\#1 \\$coord of static method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Coordinate\\:\\:coordinateIsRange\\(\\) expects string, string\\|null given\\.$#"
4835-
count: 1
4836-
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
4837-
4838-
-
4839-
message: "#^Parameter \\#1 \\$pRange of static method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Coordinate\\:\\:splitRange\\(\\) expects string, string\\|null given\\.$#"
4840-
count: 1
4841-
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
4842-
4843-
-
4844-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\:\\:\\$activeCell \\(string\\) does not accept string\\|null\\.$#"
4845-
count: 1
4846-
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
4847-
4848-
-
4849-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\:\\:\\$selectedCells \\(string\\) does not accept string\\|null\\.$#"
4850-
count: 1
4851-
path: src/PhpSpreadsheet/Worksheet/Worksheet.php
4852-
48534828
-
48544829
message: "#^Cannot call method getValue\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#"
48554830
count: 4

‎src/PhpSpreadsheet/Worksheet/Worksheet.php

+41-6
Original file line numberDiff line numberDiff line change
@@ -2373,6 +2373,38 @@ public function setSelectedCell($pCoordinate)
23732373
return $this->setSelectedCells($pCoordinate);
23742374
}
23752375

2376+
/**
2377+
* Sigh - Phpstan thinks, correctly, that preg_replace can return null.
2378+
* But Scrutinizer doesn't. Try to satisfy both.
2379+
*
2380+
* @param mixed $str
2381+
*/
2382+
private static function ensureString($str): string
2383+
{
2384+
return is_string($str) ? $str : '';
2385+
}
2386+
2387+
private static function pregReplace(string $pattern, string $replacement, string $subject): string
2388+
{
2389+
return self::ensureString(preg_replace($pattern, $replacement, $subject));
2390+
}
2391+
2392+
private function tryDefinedName(string $pCoordinate): string
2393+
{
2394+
// Uppercase coordinate
2395+
$pCoordinate = strtoupper($pCoordinate);
2396+
// Eliminate leading equal sign
2397+
$pCoordinate = self::pregReplace('/^=/', '', $pCoordinate);
2398+
$defined = $this->parent->getDefinedName($pCoordinate, $this);
2399+
if ($defined !== null) {
2400+
if ($defined->getWorksheet() === $this && !$defined->isFormula()) {
2401+
$pCoordinate = self::pregReplace('/^=/', '', $defined->getValue());
2402+
}
2403+
}
2404+
2405+
return $pCoordinate;
2406+
}
2407+
23762408
/**
23772409
* Select a range of cells.
23782410
*
@@ -2382,20 +2414,23 @@ public function setSelectedCell($pCoordinate)
23822414
*/
23832415
public function setSelectedCells($pCoordinate)
23842416
{
2385-
// Uppercase coordinate
2386-
$pCoordinate = strtoupper($pCoordinate);
2417+
$originalCoordinate = $pCoordinate;
2418+
$pCoordinate = $this->tryDefinedName($pCoordinate);
23872419

23882420
// Convert 'A' to 'A:A'
2389-
$pCoordinate = preg_replace('/^([A-Z]+)$/', '${1}:${1}', $pCoordinate);
2421+
$pCoordinate = self::pregReplace('/^([A-Z]+)$/', '${1}:${1}', $pCoordinate);
23902422

23912423
// Convert '1' to '1:1'
2392-
$pCoordinate = preg_replace('/^(\d+)$/', '${1}:${1}', $pCoordinate);
2424+
$pCoordinate = self::pregReplace('/^(\d+)$/', '${1}:${1}', $pCoordinate);
23932425

23942426
// Convert 'A:C' to 'A1:C1048576'
2395-
$pCoordinate = preg_replace('/^([A-Z]+):([A-Z]+)$/', '${1}1:${2}1048576', $pCoordinate);
2427+
$pCoordinate = self::pregReplace('/^([A-Z]+):([A-Z]+)$/', '${1}1:${2}1048576', $pCoordinate);
23962428

23972429
// Convert '1:3' to 'A1:XFD3'
2398-
$pCoordinate = preg_replace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate);
2430+
$pCoordinate = self::pregReplace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate);
2431+
if (preg_match('/^\\$?[A-Z]{1,3}\\$?\d{1,7}(:\\$?[A-Z]{1,3}\\$?\d{1,7})?$/', $pCoordinate) !== 1) {
2432+
throw new Exception("Invalid setSelectedCells $originalCoordinate $pCoordinate");
2433+
}
23992434

24002435
if (Coordinate::coordinateIsRange($pCoordinate)) {
24012436
[$first] = Coordinate::splitRange($pCoordinate);

‎tests/PhpSpreadsheetTests/NamedRange2Test.php

+120
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
<?php
2+
3+
namespace PhpOffice\PhpSpreadsheetTests;
4+
5+
use PhpOffice\PhpSpreadsheet\DefinedName;
6+
use PhpOffice\PhpSpreadsheet\Exception as Except;
7+
use PhpOffice\PhpSpreadsheet\NamedRange;
8+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
9+
use PHPUnit\Framework\TestCase;
10+
11+
class NamedRange2Test extends TestCase
12+
{
13+
/** @var ?Spreadsheet */
14+
private $spreadsheet;
15+
16+
protected function setUp(): void
17+
{
18+
$spreadsheet = $this->spreadsheet = new Spreadsheet();
19+
20+
$worksheet1 = $spreadsheet->getActiveSheet();
21+
$worksheet1->setTitle('SheetOne');
22+
$spreadsheet->addNamedRange(
23+
new NamedRange('FirstRel', $worksheet1, '=A1')
24+
);
25+
$spreadsheet->addNamedRange(
26+
new NamedRange('FirstAbs', $worksheet1, '=$B$1')
27+
);
28+
$spreadsheet->addNamedRange(
29+
new NamedRange('FirstRelMult', $worksheet1, '=C1:D2')
30+
);
31+
$spreadsheet->addNamedRange(
32+
new NamedRange('FirstAbsMult', $worksheet1, '$E$3:$F$4')
33+
);
34+
35+
$worksheet2 = $spreadsheet->createSheet();
36+
$worksheet2->setTitle('SheetTwo');
37+
$spreadsheet->addNamedRange(
38+
new NamedRange('SecondRel', $worksheet2, '=A1')
39+
);
40+
$spreadsheet->addNamedRange(
41+
new NamedRange('SecondAbs', $worksheet2, '=$B$1')
42+
);
43+
$spreadsheet->addNamedRange(
44+
new NamedRange('SecondRelMult', $worksheet2, '=C1:D2')
45+
);
46+
$spreadsheet->addNamedRange(
47+
new NamedRange('SecondAbsMult', $worksheet2, '$E$3:$F$4')
48+
);
49+
50+
$spreadsheet->addDefinedName(DefinedName::createInstance('FirstFormula', $worksheet1, '=TODAY()-1'));
51+
$spreadsheet->addDefinedName(DefinedName::createInstance('SecondFormula', $worksheet2, '=TODAY()-2'));
52+
53+
$this->spreadsheet->setActiveSheetIndex(0);
54+
}
55+
56+
protected function tearDown(): void
57+
{
58+
if ($this->spreadsheet !== null) {
59+
$this->spreadsheet->disconnectWorksheets();
60+
$this->spreadsheet = null;
61+
}
62+
}
63+
64+
private function getSpreadsheet(): Spreadsheet
65+
{
66+
if ($this->spreadsheet !== null) {
67+
return $this->spreadsheet;
68+
}
69+
$this->spreadsheet = new Spreadsheet();
70+
71+
return $this->spreadsheet;
72+
}
73+
74+
public function testNamedRangeSetStyle(): void
75+
{
76+
$spreadsheet = $this->getSpreadsheet();
77+
$sheet = $spreadsheet->getSheet(0);
78+
$sheet->getStyle('FirstRel')->getNumberFormat()->setFormatCode('yyyy-mm-dd');
79+
self::assertSame('yyyy-mm-dd', $sheet->getStyle('A1')->getNumberFormat()->getFormatCode());
80+
$sheet->getStyle('FirstAbs')->getFont()->setName('Georgia');
81+
self::assertSame('Georgia', $sheet->getStyle('B1')->getFont()->getName());
82+
$sheet->getStyle('FirstRelMult')->getFont()->setItalic(true);
83+
self::assertTrue($sheet->getStyle('D2')->getFont()->getItalic());
84+
$sheet->getStyle('FirstAbsMult')->getFill()->setFillType('gray125');
85+
self::assertSame('gray125', $sheet->getStyle('F4')->getFill()->getFillType());
86+
self::assertSame('none', $sheet->getStyle('A1')->getFill()->getFillType());
87+
$sheet = $spreadsheet->getSheet(1);
88+
$sheet->getStyle('SecondAbsMult')->getFill()->setFillType('lightDown');
89+
self::assertSame('lightDown', $sheet->getStyle('E3')->getFill()->getFillType());
90+
}
91+
92+
/**
93+
* @dataProvider providerRangeOrFormula
94+
*/
95+
public function testNamedRangeSetStyleBad(string $name): void
96+
{
97+
$this->expectException(Except::class);
98+
$spreadsheet = $this->getSpreadsheet();
99+
$sheet = $spreadsheet->getSheet(0);
100+
$sheet->getStyle($name)->getNumberFormat()->setFormatCode('yyyy-mm-dd');
101+
self::assertSame('yyyy-mm-dd', $sheet->getStyle('A1')->getNumberFormat()->getFormatCode());
102+
}
103+
104+
public function providerRangeOrFormula(): array
105+
{
106+
return [
107+
'wrong sheet rel' => ['SecondRel'],
108+
'wrong sheet abs' => ['SecondAbs'],
109+
'wrong sheet relmult' => ['SecondRelMult'],
110+
'wrong sheet absmult' => ['SecondAbsMult'],
111+
'wrong sheet formula' => ['SecondFormula'],
112+
'right sheet formula' => ['FirstFormula'],
113+
'non-existent name' => ['NonExistentName'],
114+
'this sheet name' => ['SheetOne!G7'],
115+
'other sheet name' => ['SheetTwo!G7'],
116+
'non-existent sheet name' => ['SheetAbc!G7'],
117+
'unnamed formula' => ['=2+3'],
118+
];
119+
}
120+
}

0 commit comments

Comments
 (0)