Fix Xlsx Read Ignoring Comments

Fix PHPOffice#3654. Several places in Reader Xlsx make a truthy test for `$zip->locateName()`. However, zero is a legitimate result which should not be treated as false. Change all existing tests in this form to test for (in)equality to false. For the record, there is no existing exposure of this kind in Reader Ods.
oleibman · Jul 28, 2023 · 85849e7 · 85849e7
1 parent d6e2e24
commit 85849e7
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 8 deletions.
diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php
@@ -1041,7 +1041,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                                 $hyperlinkReader = new Hyperlinks($docSheet);
                                 // Locate hyperlink relations
                                 $relationsFileName = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
-                                if ($zip->locateName($relationsFileName)) {
+                                if ($zip->locateName($relationsFileName) !== false) {
                                     $relsWorksheet = $this->loadZip($relationsFileName, Namespaces::RELATIONSHIPS);
                                     $hyperlinkReader->readHyperlinks($relsWorksheet);
                                 }
@@ -1058,7 +1058,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                             if (!$this->readDataOnly) {
                                 // Locate comment relations
                                 $commentRelations = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
-                                if ($zip->locateName($commentRelations)) {
+                                if ($zip->locateName($commentRelations) !== false) {
                                     $relsWorksheet = $this->loadZip($commentRelations, Namespaces::RELATIONSHIPS);
                                     foreach ($relsWorksheet->Relationship as $elex) {
                                         $ele = self::getAttributes($elex);
@@ -1120,7 +1120,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                                     $drowingImages = [];
                                     $VMLDrawingsRelations = dirname($relPath) . '/_rels/' . basename($relPath) . '.rels';
                                     $vmlDrawingContents[$relName] = $this->getSecurityScannerOrThrow()->scan($this->getFromZipArchive($zip, $relPath));
-                                    if ($zip->locateName($VMLDrawingsRelations)) {
+                                    if ($zip->locateName($VMLDrawingsRelations) !== false) {
                                         $relsVMLDrawing = $this->loadZip($VMLDrawingsRelations, Namespaces::RELATIONSHIPS);
                                         foreach ($relsVMLDrawing->Relationship as $elex) {
                                             $ele = self::getAttributes($elex);
@@ -1241,7 +1241,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                                     if ($vmlHfRidAttr !== null && isset($vmlHfRidAttr['id'])) {
                                         $vmlHfRid = (string) $vmlHfRidAttr['id'][0];
                                     }
-                                    if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
+                                    if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') !== false) {
                                         $relsWorksheet = $this->loadZipNoNamespace(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels', Namespaces::RELATIONSHIPS);
                                         $vmlRelationship = '';
 
@@ -1320,7 +1320,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                             if (substr($drawingFilename, 0, 8) === '/xl//xl/') {
                                 $drawingFilename = substr($drawingFilename, 5);
                             }
-                            if ($zip->locateName($drawingFilename)) {
+                            if ($zip->locateName($drawingFilename) !== false) {
                                 $relsWorksheet = $this->loadZipNoNamespace($drawingFilename, Namespaces::RELATIONSHIPS);
                                 $drawings = [];
                                 foreach ($relsWorksheet->Relationship as $ele) {
@@ -2087,7 +2087,7 @@ private static function getLockValue(SimpleXmlElement $protection, string $key):
     private function readFormControlProperties(Spreadsheet $excel, string $dir, string $fileWorksheet, Worksheet $docSheet, array &$unparsedLoadedData): void
     {
         $zip = $this->zip;
-        if (!$zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
+        if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') === false) {
             return;
         }
 
@@ -2114,7 +2114,7 @@ private function readFormControlProperties(Spreadsheet $excel, string $dir, stri
     private function readPrinterSettings(Spreadsheet $excel, string $dir, string $fileWorksheet, Worksheet $docSheet, array &$unparsedLoadedData): void
     {
         $zip = $this->zip;
-        if (!$zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
+        if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') === false) {
             return;
         }
 
@@ -2225,7 +2225,7 @@ private function readTablesInTablesFile(
             $tablePartRel = (string) $relation['id'];
             $relationsFileName = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
 
-            if ($zip->locateName($relationsFileName)) {
+            if ($zip->locateName($relationsFileName) !== false) {
                 $relsTableReferences = $this->loadZip($relationsFileName, Namespaces::RELATIONSHIPS);
                 foreach ($relsTableReferences->Relationship as $relationship) {
                     $relationshipAttributes = self::getAttributes($relationship, '');

diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/CommentTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/CommentTest.php
@@ -23,4 +23,29 @@ public function testIssue2316(): void
         self::assertStringContainsString('若為宅配物流僅能選「純配送」', $commentString);
         self::assertSame('Anderson Chen 陳宗棠', $comment->getAuthor());
     }
+
+    public function testIssue3654(): void
+    {
+        // Reader was ignoring comments.
+        $filename = 'tests/data/Reader/XLSX/issue.3654.xlsx';
+        $reader = new Xlsx();
+        $spreadsheet = $reader->load($filename);
+
+        $sheet = $spreadsheet->getActiveSheet();
+        $expectedComments = [
+            'X4', 'AD4', 'AN4',
+            'X5', 'AD5', 'AN5',
+            'AN6', // X6 and AD6 are uncommented on original
+            'X7', 'AD7', 'AN7',
+            'X8', 'AD8', 'AN8',
+            'X9', 'AD9', 'AN9',
+            'X10', 'AD10', 'AN10',
+            'X11', 'AD11', 'AN11',
+            'X12', 'AD12', 'AN12',
+        ];
+        self::assertEquals($expectedComments, array_keys($sheet->getComments()));
+        self::assertNotEmpty($sheet->getComment('X4')->getBackgroundImage()->getPath());
+        self::assertNotEmpty($sheet->getComment('AN12')->getBackgroundImage()->getPath());
+        $spreadsheet->disconnectWorksheets();
+    }
 }
diff --git a/tests/data/Reader/XLSX/issue.3654.xlsx b/tests/data/Reader/XLSX/issue.3654.xlsx