From 345b63a10aa3c598d2c4c9d74152da18b6b01645 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Mon, 25 Nov 2024 20:14:44 -0800 Subject: [PATCH] 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. --- CHANGELOG.md | 6 ++ src/PhpSpreadsheet/Worksheet/BaseDrawing.php | 22 +++-- src/PhpSpreadsheet/Worksheet/Drawing.php | 6 ++ .../Worksheet/Issue4241Test.php | 84 +++++++++++++++++++ 4 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ddade50e20..cc97a0fd52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com) and this project adheres to [Semantic Versioning](https://semver.org). +## 1.29.6 - TBD + +### Fixed + +- Fix Minor Break Handling Drawings. Backport of [PR #4244](https://github.com/PHPOffice/PhpSpreadsheet/pull/4244) + ## 1.29.5 - 2024-11-22 ### Changed diff --git a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php index 49e2eff104..da229c3b1c 100644 --- a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php +++ b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php @@ -219,15 +219,18 @@ public function getWorksheet(): ?Worksheet public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = false): self { if ($this->worksheet === null) { - // Add drawing to \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet - if ($worksheet !== null && !($this instanceof Drawing && $this->getPath() === '')) { + // Add drawing to Worksheet + if ($worksheet !== null) { $this->worksheet = $worksheet; - $this->worksheet->getCell($this->coordinates); - $this->worksheet->getDrawingCollection()->append($this); + if (!($this instanceof Drawing && $this->getPath() === '')) { + $this->worksheet->getCell($this->coordinates); + } + $this->worksheet->getDrawingCollection() + ->append($this); } } else { if ($overrideOld) { - // Remove drawing from old \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet + // Remove drawing from old Worksheet $iterator = $this->worksheet->getDrawingCollection()->getIterator(); while ($iterator->valid()) { @@ -239,10 +242,10 @@ public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = f } } - // Set new \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet + // Set new Worksheet $this->setWorksheet($worksheet); } else { - throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one \\PhpOffice\\PhpSpreadsheet\\Worksheet.'); + throw new PhpSpreadsheetException('A Worksheet has already been assigned. Drawings can only exist on one Worksheet.'); } } @@ -257,6 +260,11 @@ public function getCoordinates(): string public function setCoordinates(string $coordinates): self { $this->coordinates = $coordinates; + if ($this->worksheet !== null) { + if (!($this instanceof Drawing && $this->getPath() === '')) { + $this->worksheet->getCell($this->coordinates); + } + } return $this; } diff --git a/src/PhpSpreadsheet/Worksheet/Drawing.php b/src/PhpSpreadsheet/Worksheet/Drawing.php index 4b4d724b3f..ff5d80b54e 100644 --- a/src/PhpSpreadsheet/Worksheet/Drawing.php +++ b/src/PhpSpreadsheet/Worksheet/Drawing.php @@ -160,6 +160,12 @@ public function setPath($path, $verifyFile = true, $zip = null) throw new PhpSpreadsheetException("File $path not found!"); } + if ($this->worksheet !== null) { + if ($this->path !== '') { + $this->worksheet->getCell($this->coordinates); + } + } + return $this; } diff --git a/tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php b/tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php new file mode 100644 index 0000000000..84943cab42 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php @@ -0,0 +1,84 @@ +getActiveSheet(); + $drawing = new Drawing(); + $drawing->setName('Green Square'); + $drawing->setWorksheet($sheet); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $drawing0 = $drawings[0]; + self::assertInstanceOf(Drawing::class, $drawing0); + self::assertSame('', $drawing0->getPath()); + self::assertSame('A1', $drawing0->getCoordinates()); + $maxRow = $sheet->getHighestDataRow(); + $maxCol = $sheet->getHighestDataColumn(); + self::assertSame(1, $maxRow); + self::assertSame('A', $maxCol); + + $drawing->setCoordinates('E5'); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $drawing0 = $drawings[0]; + self::assertInstanceOf(Drawing::class, $drawing0); + self::assertSame('', $drawing0->getPath()); + self::assertSame('E5', $drawing0->getCoordinates()); + $maxRow = $sheet->getHighestDataRow(); + $maxCol = $sheet->getHighestDataColumn(); + self::assertSame(1, $maxRow); + self::assertSame('A', $maxCol); + + $drawing->setPath($badPath, false); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $drawing0 = $drawings[0]; + self::assertInstanceOf(Drawing::class, $drawing0); + self::assertSame('', $drawing0->getPath()); + self::assertSame('E5', $drawing0->getCoordinates()); + $maxRow = $sheet->getHighestDataRow(); + $maxCol = $sheet->getHighestDataColumn(); + self::assertSame(1, $maxRow); + self::assertSame('A', $maxCol); + + $drawing->setPath($goodPath); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $drawing0 = $drawings[0]; + self::assertInstanceOf(Drawing::class, $drawing0); + self::assertSame($goodPath, $drawing0->getPath()); + self::assertSame('E5', $drawing0->getCoordinates()); + $maxRow = $sheet->getHighestDataRow(); + $maxCol = $sheet->getHighestDataColumn(); + self::assertSame(5, $maxRow); + self::assertSame('E', $maxCol); + + $drawing->setCoordinates('G3'); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $drawing0 = $drawings[0]; + self::assertInstanceOf(Drawing::class, $drawing0); + self::assertSame($goodPath, $drawing0->getPath()); + self::assertSame('G3', $drawing0->getCoordinates()); + $maxRow = $sheet->getHighestDataRow(); + $maxCol = $sheet->getHighestDataColumn(); + self::assertSame(5, $maxRow); + self::assertSame('G', $maxCol); + + $spreadsheet->disconnectWorksheets(); + } +}