Skip to content

Commit b879cdd

Browse files
oleibmanoleibman
authored
Fix Xlsx Read Ignoring Comments (#3655)
* Fix Xlsx Read Ignoring Comments Fix #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. * Absolute Path In Printer Settings File Another unexpected use of an absolute path in a rels file. Easily fixed, but the test file is 7+ MB. I have asked the problem reporter to try to find a smaller file. * Reduced Size of Test File It was 7.8MB. I manipulated the file to replace all the comment backgrounds in column AN with a single image, much smaller than the existing ones. This reduced the file size by over 6MB. It's still larger than I'd like, but might be acceptable. (AN121 uses a different background than the others just so that I could check that they would all be copied faithfully.)
1 parent dcf7415 commit b879cdd

File tree

4 files changed

+59
-11
lines changed

4 files changed

+59
-11
lines changed

‎src/PhpSpreadsheet/Reader/Xlsx.php

+12-11
Original file line numberDiff line numberDiff line change
@@ -1049,7 +1049,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
10491049
$hyperlinkReader = new Hyperlinks($docSheet);
10501050
// Locate hyperlink relations
10511051
$relationsFileName = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
1052-
if ($zip->locateName($relationsFileName)) {
1052+
if ($zip->locateName($relationsFileName) !== false) {
10531053
$relsWorksheet = $this->loadZip($relationsFileName, Namespaces::RELATIONSHIPS);
10541054
$hyperlinkReader->readHyperlinks($relsWorksheet);
10551055
}
@@ -1066,7 +1066,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
10661066
if (!$this->readDataOnly) {
10671067
// Locate comment relations
10681068
$commentRelations = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
1069-
if ($zip->locateName($commentRelations)) {
1069+
if ($zip->locateName($commentRelations) !== false) {
10701070
$relsWorksheet = $this->loadZip($commentRelations, Namespaces::RELATIONSHIPS);
10711071
foreach ($relsWorksheet->Relationship as $elex) {
10721072
$ele = self::getAttributes($elex);
@@ -1128,7 +1128,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
11281128
$drowingImages = [];
11291129
$VMLDrawingsRelations = dirname($relPath) . '/_rels/' . basename($relPath) . '.rels';
11301130
$vmlDrawingContents[$relName] = $this->getSecurityScannerOrThrow()->scan($this->getFromZipArchive($zip, $relPath));
1131-
if ($zip->locateName($VMLDrawingsRelations)) {
1131+
if ($zip->locateName($VMLDrawingsRelations) !== false) {
11321132
$relsVMLDrawing = $this->loadZip($VMLDrawingsRelations, Namespaces::RELATIONSHIPS);
11331133
foreach ($relsVMLDrawing->Relationship as $elex) {
11341134
$ele = self::getAttributes($elex);
@@ -1192,7 +1192,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
11921192
if (isset($drowingImages[$fillImageRelId])) {
11931193
$objDrawing = new \PhpOffice\PhpSpreadsheet\Worksheet\Drawing();
11941194
$objDrawing->setName($fillImageTitle);
1195-
$imagePath = str_replace('../', 'xl/', $drowingImages[$fillImageRelId]);
1195+
$imagePath = str_replace(['../', '/xl/'], 'xl/', $drowingImages[$fillImageRelId]);
11961196
$objDrawing->setPath(
11971197
'zip://' . File::realpath($filename) . '#' . $imagePath,
11981198
true,
@@ -1249,7 +1249,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
12491249
if ($vmlHfRidAttr !== null && isset($vmlHfRidAttr['id'])) {
12501250
$vmlHfRid = (string) $vmlHfRidAttr['id'][0];
12511251
}
1252-
if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
1252+
if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') !== false) {
12531253
$relsWorksheet = $this->loadZipNoNamespace(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels', Namespaces::RELATIONSHIPS);
12541254
$vmlRelationship = '';
12551255

@@ -1328,7 +1328,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
13281328
if (substr($drawingFilename, 0, 8) === '/xl//xl/') {
13291329
$drawingFilename = substr($drawingFilename, 5);
13301330
}
1331-
if ($zip->locateName($drawingFilename)) {
1331+
if ($zip->locateName($drawingFilename) !== false) {
13321332
$relsWorksheet = $this->loadZipNoNamespace($drawingFilename, Namespaces::RELATIONSHIPS);
13331333
$drawings = [];
13341334
foreach ($relsWorksheet->Relationship as $ele) {
@@ -2095,7 +2095,7 @@ private static function getLockValue(SimpleXmlElement $protection, string $key):
20952095
private function readFormControlProperties(Spreadsheet $excel, string $dir, string $fileWorksheet, Worksheet $docSheet, array &$unparsedLoadedData): void
20962096
{
20972097
$zip = $this->zip;
2098-
if (!$zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
2098+
if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') === false) {
20992099
return;
21002100
}
21012101

@@ -2122,7 +2122,7 @@ private function readFormControlProperties(Spreadsheet $excel, string $dir, stri
21222122
private function readPrinterSettings(Spreadsheet $excel, string $dir, string $fileWorksheet, Worksheet $docSheet, array &$unparsedLoadedData): void
21232123
{
21242124
$zip = $this->zip;
2125-
if (!$zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {
2125+
if ($zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels') === false) {
21262126
return;
21272127
}
21282128

@@ -2142,8 +2142,9 @@ private function readPrinterSettings(Spreadsheet $excel, string $dir, string $fi
21422142
$rId = $rId . 'ps'; // rIdXXX, add 'ps' suffix to avoid identical resource identifier collision with unparsed vmlDrawing
21432143
}
21442144
$unparsedPrinterSettings[$rId] = [];
2145-
$unparsedPrinterSettings[$rId]['filePath'] = self::dirAdd("$dir/$fileWorksheet", $printerSettings['Target']);
2146-
$unparsedPrinterSettings[$rId]['relFilePath'] = (string) $printerSettings['Target'];
2145+
$target = (string) str_replace('/xl/', '../', (string) $printerSettings['Target']);
2146+
$unparsedPrinterSettings[$rId]['filePath'] = self::dirAdd("$dir/$fileWorksheet", $target);
2147+
$unparsedPrinterSettings[$rId]['relFilePath'] = $target;
21472148
$unparsedPrinterSettings[$rId]['content'] = $this->getSecurityScannerOrThrow()->scan($this->getFromZipArchive($zip, $unparsedPrinterSettings[$rId]['filePath']));
21482149
}
21492150
unset($unparsedPrinterSettings);
@@ -2238,7 +2239,7 @@ private function readTablesInTablesFile(
22382239
$tablePartRel = (string) $relation['id'];
22392240
$relationsFileName = dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels';
22402241

2241-
if ($zip->locateName($relationsFileName)) {
2242+
if ($zip->locateName($relationsFileName) !== false) {
22422243
$relsTableReferences = $this->loadZip($relationsFileName, Namespaces::RELATIONSHIPS);
22432244
foreach ($relsTableReferences->Relationship as $relationship) {
22442245
$relationshipAttributes = self::getAttributes($relationship, '');

‎tests/PhpSpreadsheetTests/Reader/Xlsx/CommentTest.php

+47
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,51 @@ public function testIssue2316(): void
2323
self::assertStringContainsString('若為宅配物流僅能選「純配送」', $commentString);
2424
self::assertSame('Anderson Chen 陳宗棠', $comment->getAuthor());
2525
}
26+
27+
public function testIssue3654(): void
28+
{
29+
// Reader was ignoring comments.
30+
$filename = 'tests/data/Reader/XLSX/issue.3654.xlsx';
31+
$reader = new Xlsx();
32+
$spreadsheet = $reader->load($filename);
33+
34+
$sheet = $spreadsheet->getActiveSheet();
35+
$expectedComments = [
36+
'X4', 'AD4', 'AN4',
37+
'X5', 'AD5', 'AN5',
38+
'AN6', // X6 and AD6 are uncommented on original
39+
'X7', 'AD7', 'AN7',
40+
'X8', 'AD8', 'AN8',
41+
'X9', 'AD9', 'AN9',
42+
'X10', 'AD10', 'AN10',
43+
'X11', 'AD11', 'AN11',
44+
'X12', 'AD12', 'AN12',
45+
];
46+
self::assertEquals($expectedComments, array_keys($sheet->getComments()));
47+
self::assertStringContainsString('.png', $sheet->getComment('X4')->getBackgroundImage()->getPath());
48+
self::assertStringContainsString('.jpeg', $sheet->getComment('AN12')->getBackgroundImage()->getPath());
49+
$spreadsheet->disconnectWorksheets();
50+
}
51+
52+
public function testIssue3654c(): void
53+
{
54+
// Reader was ignoring comments.
55+
$filename = 'tests/data/Reader/XLSX/issue.3654c.xlsx';
56+
$reader = new Xlsx();
57+
$spreadsheet = $reader->load($filename);
58+
59+
$sheet = $spreadsheet->getActiveSheet();
60+
$comments = $sheet->getComments();
61+
self::assertCount(326, $comments);
62+
self::assertStringContainsString('.png', $sheet->getComment('X4')->getBackgroundImage()->getPath());
63+
self::assertStringContainsString('.jpeg', $sheet->getComment('AN12')->getBackgroundImage()->getPath());
64+
65+
// Spreadsheet generated by Flexcel. Make sure we managed
66+
// to locate printerSettings since it used an unexpected link.
67+
$pageSetupRel = $spreadsheet->getUnparsedLoadedData()['sheets']['Worksheet']['pageSetupRelId'] ?? '';
68+
self::assertSame('flId1ps', $pageSetupRel);
69+
$pageSetupPath = $spreadsheet->getUnparsedLoadedData()['sheets']['Worksheet']['printerSettings'][substr($pageSetupRel, 3)]['filePath'] ?? '';
70+
self::assertSame('xl/printerSettings/printerSettings1.bin', $pageSetupPath);
71+
$spreadsheet->disconnectWorksheets();
72+
}
2673
}

‎tests/data/Reader/XLSX/issue.3654.xlsx

558 KB
Binary file not shown.

‎tests/data/Reader/XLSX/issue.3654c.xlsx

1.7 MB
Binary file not shown.

0 commit comments

Comments
 (0)