Repository: PHPOffice/PhpSpreadsheet (php)

Issue:
Inserting Image into a Cell
Inserting images fails without any error messages since version 1.29.2. However, version 1.29.1 works normally
```php
$filename = "path_to_file.jpg";
$drawing = new Drawing();
$drawing->setWorksheet($worksheet);
$drawing->setPath($filename);
$drawing->setResizeProportional(true);
$drawing->setCoordinates('A2');
```

Pull request description:
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.

This is:

- [x] a bugfix
- [ ] a new feature
- [ ] refactoring
- [ ] additional unit tests

Checklist:

- [ ] Changes are covered by unit tests
  - [ ] Changes are covered by existing unit tests
  - [x] New unit tests have been added
- [x] Code style is respected
- [x] Commit message explains **why** the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
- [ ] CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
- [ ] Documentation is updated as necessary

### Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0993b77eb..dd265511f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 
 ### Fixed
 
+- Fix minor break handling drawings. [Issue #4241](https://github.com/PHPOffice/PhpSpreadsheet/issues/4241) [PR #4244](https://github.com/PHPOffice/PhpSpreadsheet/pull/4244)
 - Ignore cell formatting when the format is a single @. [Issue #4242](https://github.com/PHPOffice/PhpSpreadsheet/issues/4242) [PR #4243](https://github.com/PHPOffice/PhpSpreadsheet/pull/4243)
 
 ## 2024-11-22 - 3.5.0
diff --git a/samples/Basic4/53_ImageOpacity.php b/samples/Basic4/53_ImageOpacity.php
index 75d22bae9..ec3916a9c 100644
--- a/samples/Basic4/53_ImageOpacity.php
+++ b/samples/Basic4/53_ImageOpacity.php
@@ -5,9 +5,6 @@ require __DIR__ . '/../Header.php';
 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->setOpacity(80000);
 $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->setOpacity(20000);
 $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']);
diff --git a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php
index 326b8d6d2..135f5b216 100644
--- a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php
+++ b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php
@@ -197,15 +197,18 @@ class BaseDrawing implements IComparable
     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 @@ class BaseDrawing implements IComparable
                     }
                 }
 
-                // 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 @@ class BaseDrawing implements IComparable
     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 bee049100..35933af3f 100644
--- a/src/PhpSpreadsheet/Worksheet/Drawing.php
+++ b/src/PhpSpreadsheet/Worksheet/Drawing.php
@@ -148,6 +148,12 @@ class Drawing extends BaseDrawing
             throw new PhpSpreadsheetException("File $path not found!");
         }
 
+        if ($this->worksheet !== null) {
+            if ($this->path !== '') {
+                $this->worksheet->getCell($this->coordinates);
+            }
+        }
+
         return $this;
     }
 


Test patch:
diff --git a/tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php b/tests/PhpSpreadsheetTests/Worksheet/Issue4241Test.php
new file mode 100644
index 000000000..84943cab4
--- /dev/null
+++ 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();
+    }
+}