Fix Minor Break Handling Drawings

Fix PHPOffice#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.

samples/Basic4/53_ImageOpacity.php

@@ -5,9 +5,6 @@
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
 
-//var_dump(realpath(__DIR__ . '/../images/blue_square.png'));
-//exit();
-
 $path = __DIR__ . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR . 'images/blue_square.png';
 $spreadsheet = new Spreadsheet();
 $spreadsheet->getProperties()->setTitle('53_ImageOpacity');
@@ -33,12 +30,12 @@
 $drawing->setWorksheet($sheet);
 
 $drawing = new Drawing();
+$drawing->setWorksheet($sheet);
 $drawing->setName('Blue Square opacity 60%');
 $drawing->setPath($path);
 $drawing->setCoordinates('E1');
 $drawing->setCoordinates2('F5');
 $drawing->setOpacity(60000);
-$drawing->setWorksheet($sheet);
 
 $drawing = new Drawing();
 $drawing->setName('Blue Square opacity 40%');
@@ -57,12 +54,12 @@
 $drawing->setWorksheet($sheet);
 
 $drawing = new Drawing();
+$drawing->setWorksheet($sheet);
 $drawing->setName('Blue Square opacity 0%');
 $drawing->setPath($path);
 $drawing->setCoordinates('E8');
 $drawing->setCoordinates2('F12');
 $drawing->setOpacity(0);
-$drawing->setWorksheet($sheet);
 
 // Save
 $helper->write($spreadsheet, __FILE__, ['Xlsx', 'Html', 'Dompdf', 'Mpdf']);

src/PhpSpreadsheet/Worksheet/BaseDrawing.php

@@ -197,15 +197,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()) {
@@ -217,10 +220,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.');
             }
         }
 
@@ -235,6 +238,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;
     }

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;
     }
 

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();
+    }
+}