Skip to content

Commit 0adf030

Browse files
oleibmanoleibman
committed
Fix Minor Break Handling Drawings
Backport of #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.
1 parent 526cdf3 commit 0adf030

File tree

4 files changed

+111
-7
lines changed

4 files changed

+111
-7
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](https://keepachangelog.com)
66
and this project adheres to [Semantic Versioning](https://semver.org).
77

8+
## TBD - 2.3.4
9+
10+
### Fixed
11+
12+
- Fix Minor Break Handling Drawings. Backport of [PR #4244](https://github.com/PHPOffice/PhpSpreadsheet/pull/4244)
13+
814
## 2024-11-22 - 2.3.3
915

1016
### Changed

‎src/PhpSpreadsheet/Worksheet/BaseDrawing.php‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,15 +191,18 @@ public function getWorksheet(): ?Worksheet
191191
public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = false): self
192192
{
193193
if ($this->worksheet === null) {
194-
// Add drawing to \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
195-
if ($worksheet !== null && !($this instanceof Drawing && $this->getPath() === '')) {
194+
// Add drawing to Worksheet
195+
if ($worksheet !== null) {
196196
$this->worksheet = $worksheet;
197-
$this->worksheet->getCell($this->coordinates);
198-
$this->worksheet->getDrawingCollection()->append($this);
197+
if (!($this instanceof Drawing && $this->getPath() === '')) {
198+
$this->worksheet->getCell($this->coordinates);
199+
}
200+
$this->worksheet->getDrawingCollection()
201+
->append($this);
199202
}
200203
} else {
201204
if ($overrideOld) {
202-
// Remove drawing from old \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
205+
// Remove drawing from old Worksheet
203206
$iterator = $this->worksheet->getDrawingCollection()->getIterator();
204207

205208
while ($iterator->valid()) {
@@ -211,10 +214,10 @@ public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = f
211214
}
212215
}
213216

214-
// Set new \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
217+
// Set new Worksheet
215218
$this->setWorksheet($worksheet);
216219
} else {
217-
throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one \\PhpOffice\\PhpSpreadsheet\\Worksheet.');
220+
throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one Worksheet.');
218221
}
219222
}
220223

@@ -229,6 +232,11 @@ public function getCoordinates(): string
229232
public function setCoordinates(string $coordinates): self
230233
{
231234
$this->coordinates = $coordinates;
235+
if ($this->worksheet !== null) {
236+
if (!($this instanceof Drawing && $this->getPath() === '')) {
237+
$this->worksheet->getCell($this->coordinates);
238+
}
239+
}
232240

233241
return $this;
234242
}

‎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)