Avoid memory exhaustion when cloning worksheet with a drawing

When cloning `BaseDrawing`, its worksheet will be set as null and thus be orphaned. But when cloning the worksheet, it will re-assign itself as the new worksheet for the BaseDrawing. That way we avoid recursive cloning of a Worksheet that would clone a BaseDrawing that would clone a Worksheet etc. Fixes PHPOffice#437 Fixes PHPOffice#613
Dfred · Oct 29, 2018 · 99e32d2 · 99e32d2
1 parent 871287d
commit 99e32d2
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -65,6 +65,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Could not open CSV file containing HTML fragment - [#564](https://github.com/PHPOffice/PhpSpreadsheet/issues/564)
 - Exclude the vendor folder in migration - [#481](https://github.com/PHPOffice/PhpSpreadsheet/issues/481)
 - Chained operations on cell ranges involving borders operated on last cell only [#428](https://github.com/PHPOffice/PhpSpreadsheet/issues/428)
+- Avoid memory exhaustion when cloning worksheet with a drawing [#437](https://github.com/PHPOffice/PhpSpreadsheet/issues/437)
 
 ## [1.3.1] - 2018-06-12
 

diff --git a/src/PhpSpreadsheet/Worksheet/AutoFilter.php b/src/PhpSpreadsheet/Worksheet/AutoFilter.php
@@ -841,7 +841,7 @@ public function __clone()
         $vars = get_object_vars($this);
         foreach ($vars as $key => $value) {
             if (is_object($value)) {
-                if ($key == 'workSheet') {
+                if ($key === 'workSheet') {
                     //    Detach from worksheet
                     $this->{$key} = null;
                 } else {

diff --git a/src/PhpSpreadsheet/Worksheet/BaseDrawing.php b/src/PhpSpreadsheet/Worksheet/BaseDrawing.php
@@ -509,7 +509,9 @@ public function __clone()
     {
         $vars = get_object_vars($this);
         foreach ($vars as $key => $value) {
-            if (is_object($value)) {
+            if ($key == 'worksheet') {
+                $this->worksheet = null;
+            } elseif (is_object($value)) {
                 $this->$key = clone $value;
             } else {
                 $this->$key = $value;

diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php
@@ -2985,13 +2985,14 @@ public function __clone()
                     $newCollection = $this->cellCollection->cloneCellCollection($this);
                     $this->cellCollection = $newCollection;
                 } elseif ($key == 'drawingCollection') {
-                    $newCollection = new ArrayObject();
-                    foreach ($this->drawingCollection as $id => $item) {
+                    $currentCollection = $this->drawingCollection;
+                    $this->drawingCollection = new ArrayObject();
+                    foreach ($currentCollection as $item) {
                         if (is_object($item)) {
-                            $newCollection[$id] = clone $this->drawingCollection[$id];
+                            $newDrawing = clone $item;
+                            $newDrawing->setWorksheet($this);
                         }
                     }
-                    $this->drawingCollection = $newCollection;
                 } elseif (($key == 'autoFilter') && ($this->autoFilter instanceof AutoFilter)) {
                     $newAutoFilter = clone $this->autoFilter;
                     $this->autoFilter = $newAutoFilter;

diff --git a/tests/PhpSpreadsheetTests/Worksheet/DrawingTest.php b/tests/PhpSpreadsheetTests/Worksheet/DrawingTest.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing;
+use PHPUnit\Framework\TestCase;
+
+class DrawingTest extends TestCase
+{
+    public function testCloningWorksheetWithImages()
+    {
+        $spreadsheet = new Spreadsheet();
+        $aSheet = $spreadsheet->getActiveSheet();
+
+        $gdImage = @imagecreatetruecolor(120, 20);
+        $textColor = imagecolorallocate($gdImage, 255, 255, 255);
+        imagestring($gdImage, 1, 5, 5, 'Created with PhpSpreadsheet', $textColor);
+
+        $drawing = new MemoryDrawing();
+        $drawing->setName('In-Memory image 1');
+        $drawing->setDescription('In-Memory image 1');
+        $drawing->setCoordinates('A1');
+        $drawing->setImageResource($gdImage);
+        $drawing->setRenderingFunction(
+            MemoryDrawing::RENDERING_JPEG
+        );
+        $drawing->setMimeType(MemoryDrawing::MIMETYPE_DEFAULT);
+        $drawing->setHeight(36);
+        $drawing->setWorksheet($aSheet);
+
+        $originDrawingCount = count($aSheet->getDrawingCollection());
+        $clonedWorksheet = clone $aSheet;
+        $clonedDrawingCount = count($clonedWorksheet->getDrawingCollection());
+
+        self::assertEquals($originDrawingCount, $clonedDrawingCount);
+        self::assertNotSame($aSheet, $clonedWorksheet);
+        self::assertNotSame($aSheet->getDrawingCollection(), $clonedWorksheet->getDrawingCollection());
+    }
+}