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.
PHPOffice · Nov 26, 2024 · 2dac558 · 2dac558
1 parent db57c5b
commit 2dac558
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 7 deletions.
diff --git 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).
 
+## TBD - 2.1.5
+
+### Fixed
+
+- Fix Minor Break Handling Drawings. Backport of [PR #4244](https://github.com/PHPOffice/PhpSpreadsheet/pull/4244)
+
 ## 2024-11-22 - 2.1.4
 
 ### Changed

diff --git a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php
@@ -191,15 +191,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()) {
@@ -211,10 +214,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.');
             }
         }
 
@@ -229,6 +232,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
@@ -148,6 +148,12 @@ public function setPath(string $path, bool $verifyFile = true, ?ZipArchive $zip
             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
@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
+use PHPUnit\Framework\TestCase;
+
+class Issue4241Test extends TestCase
+{
+    public function testIssue4241(): void
+    {
+        // setWorksheet needed to come after setPath
+        $badPath = 'tests/data/Writer/XLSX/xgreen_square.gif';
+        $goodPath = 'tests/data/Writer/XLSX/green_square.gif';
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->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();
+    }
+}