Skip to content

Commit 5c3ae52

Browse files
oleibmanoleibman
committed
Fix Minor Break Handling Drawings
Fix #4241. Some security batches caused a minor break in Drawings, forcing `setWorksheet` to come after `setPath`. Although the problem is easily fixed in user code, this was not an intended change. Some slight recoding restores the earlier functionality where the order of calls was not important, without sacrificing the security gains. This change will be back-ported to the other active branches to which the security patch had been applied.
1 parent f37b119 commit 5c3ae52

File tree

4 files changed

+107
-12
lines changed

4 files changed

+107
-12
lines changed

‎samples/Basic4/53_ImageOpacity.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,6 @@
55
use PhpOffice\PhpSpreadsheet\Spreadsheet;
66
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
77

8-
//var_dump(realpath(__DIR__ . '/../images/blue_square.png'));
9-
//exit();
10-
118
$path = __DIR__ . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR . 'images/blue_square.png';
129
$spreadsheet = new Spreadsheet();
1310
$spreadsheet->getProperties()->setTitle('53_ImageOpacity');
@@ -33,12 +30,12 @@
3330
$drawing->setWorksheet($sheet);
3431

3532
$drawing = new Drawing();
33+
$drawing->setWorksheet($sheet);
3634
$drawing->setName('Blue Square opacity 60%');
3735
$drawing->setPath($path);
3836
$drawing->setCoordinates('E1');
3937
$drawing->setCoordinates2('F5');
4038
$drawing->setOpacity(60000);
41-
$drawing->setWorksheet($sheet);
4239

4340
$drawing = new Drawing();
4441
$drawing->setName('Blue Square opacity 40%');
@@ -57,12 +54,12 @@
5754
$drawing->setWorksheet($sheet);
5855

5956
$drawing = new Drawing();
57+
$drawing->setWorksheet($sheet);
6058
$drawing->setName('Blue Square opacity 0%');
6159
$drawing->setPath($path);
6260
$drawing->setCoordinates('E8');
6361
$drawing->setCoordinates2('F12');
6462
$drawing->setOpacity(0);
65-
$drawing->setWorksheet($sheet);
6663

6764
// Save
6865
$helper->write($spreadsheet, __FILE__, ['Xlsx', 'Html', 'Dompdf', 'Mpdf']);

‎src/PhpSpreadsheet/Worksheet/BaseDrawing.php‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -197,15 +197,18 @@ public function getWorksheet(): ?Worksheet
197197
public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = false): self
198198
{
199199
if ($this->worksheet === null) {
200-
// Add drawing to \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
201-
if ($worksheet !== null && !($this instanceof Drawing && $this->getPath() === '')) {
200+
// Add drawing to Worksheet
201+
if ($worksheet !== null) {
202202
$this->worksheet = $worksheet;
203-
$this->worksheet->getCell($this->coordinates);
204-
$this->worksheet->getDrawingCollection()->append($this);
203+
if (!($this instanceof Drawing && $this->getPath() === '')) {
204+
$this->worksheet->getCell($this->coordinates);
205+
}
206+
$this->worksheet->getDrawingCollection()
207+
->append($this);
205208
}
206209
} else {
207210
if ($overrideOld) {
208-
// Remove drawing from old \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
211+
// Remove drawing from old Worksheet
209212
$iterator = $this->worksheet->getDrawingCollection()->getIterator();
210213

211214
while ($iterator->valid()) {
@@ -217,10 +220,10 @@ public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = f
217220
}
218221
}
219222

220-
// Set new \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
223+
// Set new Worksheet
221224
$this->setWorksheet($worksheet);
222225
} else {
223-
throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one \\PhpOffice\\PhpSpreadsheet\\Worksheet.');
226+
throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one Worksheet.');
224227
}
225228
}
226229

@@ -235,6 +238,11 @@ public function getCoordinates(): string
235238
public function setCoordinates(string $coordinates): self
236239
{
237240
$this->coordinates = $coordinates;
241+
if ($this->worksheet !== null) {
242+
if (!($this instanceof Drawing && $this->getPath() === '')) {
243+
$this->worksheet->getCell($this->coordinates);
244+
}
245+
}
238246

239247
return $this;
240248
}

‎src/PhpSpreadsheet/Worksheet/Drawing.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,12 @@ public function setPath(string $path, bool $verifyFile = true, ?ZipArchive $zip
148148
throw new PhpSpreadsheetException("File $path not found!");
149149
}
150150

151+
if ($this->worksheet !== null) {
152+
if ($this->path !== '') {
153+
$this->worksheet->getCell($this->coordinates);
154+
}
155+
}
156+
151157
return $this;
152158
}
153159

‎tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
6+
7+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
8+
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
9+
use PHPUnit\Framework\TestCase;
10+
11+
class Issue4241Test extends TestCase
12+
{
13+
public function testIssue4241(): void
14+
{
15+
// setWorksheet needed to come after setPath
16+
$badPath = 'tests/data/Writer/XLSX/xgreen_square.gif';
17+
$goodPath = 'tests/data/Writer/XLSX/green_square.gif';
18+
$spreadsheet = new Spreadsheet();
19+
$sheet = $spreadsheet->getActiveSheet();
20+
$drawing = new Drawing();
21+
$drawing->setName('Green Square');
22+
$drawing->setWorksheet($sheet);
23+
$drawings = $sheet->getDrawingCollection();
24+
self::assertCount(1, $drawings);
25+
$drawing0 = $drawings[0];
26+
self::assertInstanceOf(Drawing::class, $drawing0);
27+
self::assertSame('', $drawing0->getPath());
28+
self::assertSame('A1', $drawing0->getCoordinates());
29+
$maxRow = $sheet->getHighestDataRow();
30+
$maxCol = $sheet->getHighestDataColumn();
31+
self::assertSame(1, $maxRow);
32+
self::assertSame('A', $maxCol);
33+
34+
$drawing->setCoordinates('E5');
35+
$drawings = $sheet->getDrawingCollection();
36+
self::assertCount(1, $drawings);
37+
$drawing0 = $drawings[0];
38+
self::assertInstanceOf(Drawing::class, $drawing0);
39+
self::assertSame('', $drawing0->getPath());
40+
self::assertSame('E5', $drawing0->getCoordinates());
41+
$maxRow = $sheet->getHighestDataRow();
42+
$maxCol = $sheet->getHighestDataColumn();
43+
self::assertSame(1, $maxRow);
44+
self::assertSame('A', $maxCol);
45+
46+
$drawing->setPath($badPath, false);
47+
$drawings = $sheet->getDrawingCollection();
48+
self::assertCount(1, $drawings);
49+
$drawing0 = $drawings[0];
50+
self::assertInstanceOf(Drawing::class, $drawing0);
51+
self::assertSame('', $drawing0->getPath());
52+
self::assertSame('E5', $drawing0->getCoordinates());
53+
$maxRow = $sheet->getHighestDataRow();
54+
$maxCol = $sheet->getHighestDataColumn();
55+
self::assertSame(1, $maxRow);
56+
self::assertSame('A', $maxCol);
57+
58+
$drawing->setPath($goodPath);
59+
$drawings = $sheet->getDrawingCollection();
60+
self::assertCount(1, $drawings);
61+
$drawing0 = $drawings[0];
62+
self::assertInstanceOf(Drawing::class, $drawing0);
63+
self::assertSame($goodPath, $drawing0->getPath());
64+
self::assertSame('E5', $drawing0->getCoordinates());
65+
$maxRow = $sheet->getHighestDataRow();
66+
$maxCol = $sheet->getHighestDataColumn();
67+
self::assertSame(5, $maxRow);
68+
self::assertSame('E', $maxCol);
69+
70+
$drawing->setCoordinates('G3');
71+
$drawings = $sheet->getDrawingCollection();
72+
self::assertCount(1, $drawings);
73+
$drawing0 = $drawings[0];
74+
self::assertInstanceOf(Drawing::class, $drawing0);
75+
self::assertSame($goodPath, $drawing0->getPath());
76+
self::assertSame('G3', $drawing0->getCoordinates());
77+
$maxRow = $sheet->getHighestDataRow();
78+
$maxCol = $sheet->getHighestDataColumn();
79+
self::assertSame(5, $maxRow);
80+
self::assertSame('G', $maxCol);
81+
82+
$spreadsheet->disconnectWorksheets();
83+
}
84+
}

0 commit comments

Comments
 (0)