Name Clashes Between Parsed and Unparsed Drawings (#2423)

* Name Clashes Between Parsed and Unparsed Drawings

This is at least a partial fix for #2396 and #1767 (which has been around for a long time). PhpSpreadsheet renames drawing XML files when it reads them from a spreadsheet. However, when it writes unparsed drawing files, it uses the original names, which can result in a clash with the renamed files. The solution in this PR is to write the unparsed files using the same renaming convention as the the others.

This is an incredibly simple fix, basically a one-line change, for such a long-lived problem. It is conceivable that this PR breaks a more sophisticated file than I have come across, e.g. with multiple unparsed files associated with a single worksheet. However, this PR does fix at least part of the problem for both issues, and causes no regression issues. The changed code was covered in only 2 tests - Reader/XlsxTest testLoadSaveWithEmptyDrawings and Writer/Xlsx/UnparsedDataTest testLoadSaveXlsxWithUnparsedData.

2396 is covered by a new test Unparsed2396Test. I had trouble figuring out what to test for 1767. Since it is a problem that becomes evident only when the output file is opened in Excel, I added a new sample to cover it.

* Sloppy Errors

I neglected to run php-cs-fixer and phpstan, and it bit me.

* Scrutinizer

It's not as good as Phpstan at recognizing problems that can't happen due to previous assertions.

* Scrutinizer Again

It can be really stupid sometimes.

Author: oleibman

samples/Reader/22_Reader_issue1767.php

@@ -0,0 +1,21 @@
+<?php
+
+use PhpOffice\PhpSpreadsheet\IOFactory;
+
+require __DIR__ . '/../Header.php';
+
+$helper->log('Start');
+
+$inputFileType = 'Xlsx';
+$inputFileName = __DIR__ . '/sampleData/issue.1767.xlsx';
+
+$helper->log('Loading file ' . $inputFileName . ' using IOFactory with a defined reader type of ' . $inputFileType);
+$reader = IOFactory::createReader($inputFileType);
+$helper->log('Loading all WorkSheets');
+$reader->setLoadAllSheets();
+$spreadsheet = $reader->load($inputFileName);
+
+// Save
+$helper->write($spreadsheet, __FILE__);
+
+$helper->log('end');

samples/Reader/sampleData/issue.1767.xlsx

@@ -431,8 +431,9 @@ public function save($filename, int $flags = 0): void
                 foreach ($unparsedLoadedData['sheets'][$sheetCodeName]['Drawings'] as $relId => $drawingXml) {
                     $drawingFile = array_search($relId, $unparsedLoadedData['sheets'][$sheetCodeName]['drawingOriginalIds']);
                     if ($drawingFile !== false) {
-                        $drawingFile = ltrim($drawingFile, '.');
-                        $zipContent['xl' . $drawingFile] = $drawingXml;
+                        //$drawingFile = ltrim($drawingFile, '.');
+                        //$zipContent['xl' . $drawingFile] = $drawingXml;
+                        $zipContent['xl/drawings/drawing' . ($i + 1) . '.xml'] = $drawingXml;
                     }
                 }
             }

src/PhpSpreadsheet/Writer/Xlsx.php

@@ -0,0 +1,90 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
+
+use PhpOffice\PhpSpreadsheet\Reader\Xlsx as Reader;
+use PhpOffice\PhpSpreadsheet\Shared\File;
+use PhpOffice\PhpSpreadsheet\Worksheet\BaseDrawing;
+use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
+use PhpOffice\PhpSpreadsheet\Writer\Xlsx as Writer;
+use PHPUnit\Framework\TestCase;
+
+class Unparsed2396Test extends TestCase
+{
+    /** @var string */
+    private $filename = '';
+
+    protected function tearDown(): void
+    {
+        if ($this->filename !== '') {
+            unlink($this->filename);
+            $this->filename = '';
+        }
+    }
+
+    private function getContents(?BaseDrawing $drawing): string
+    {
+        $contents = '';
+        if ($drawing instanceof Drawing) {
+            $contents = (string) file_get_contents($drawing->getPath());
+        } else {
+            self::fail('Unexpected null or baseDrawing which is not Drawing');
+        }
+        self::assertNotSame('', $contents);
+
+        return $contents;
+    }
+
+    // Don't drop image as in issue 2396.
+    public function testUnparsed2396(): void
+    {
+        $sampleFilename = 'tests/data/Writer/XLSX/issue.2396.xlsx';
+        $reader = new Reader();
+        $excel = $reader->load($sampleFilename);
+        $outputFilename = $this->filename = File::temporaryFilename();
+        $writer = new Writer($excel);
+        $writer->save($outputFilename);
+        //$spreadsheet = $this->writeAndReload($excel, 'Xlsx');
+        $excel->disconnectWorksheets();
+        $reader = new Reader();
+        $spreadsheet = $reader->load($outputFilename);
+        $sheet = $spreadsheet->getSheet(0);
+        $drawing1 = $sheet->getDrawingCollection();
+        self::assertCount(1, $drawing1);
+        $hash = $this->getContents($drawing1[0]);
+
+        $sheet = $spreadsheet->getSheet(1);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        self::assertSame($hash, $this->getContents($drawings[0]));
+
+        $sheet = $spreadsheet->getSheet(2);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(0, $drawings);
+        //self::assertSame($hash, md5(file_get_contents($drawings[0]->getPath())));
+
+        $sheet = $spreadsheet->getSheet(3);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        self::assertSame($hash, $this->getContents($drawings[0]));
+
+        $sheet = $spreadsheet->getSheet(4);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(0, $drawings);
+        //self::assertSame($hash, md5(file_get_contents($drawings[0]->getPath())));
+
+        // The next 2 sheets have 'legacyDrawing', button, and listbox.
+        // If support is added for those, these tests may need adjustment.
+        $sheet = $spreadsheet->getSheet(5);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(0, $drawings);
+        //self::assertSame($hash, md5(file_get_contents($drawings[0]->getPath())));
+
+        $sheet = $spreadsheet->getSheet(6);
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        self::assertSame($hash, $this->getContents($drawings[0]));
+
+        $spreadsheet->disconnectWorksheets();
+    }
+}