Unexpected Namespacing in rels File

oleibman · oleibman · commit 68632aa1f3e3 · 2023-09-09T21:46:17.000-07:00
Fix PHPOffice#3720. Third-party product created a spreadsheet which PhpSpreadsheet could not read because of unexpected namespacing in workbook.xml.rels. The file which demonstrated the problem was attached to PHPOffice#3423, however I do not believe it was related to the original problem. Nevertheless, the original issue specifically called out Protection, so I put some Protection tests in the validation test for the fix. In doing so, I found that Style/Protection is particularly confusing. Its properties will often have the value `inherit`, which isn't all that helpful; and, even when the `locked` value is `protected`, the cell won't actually be locked unless the sheet is protected as well. The `hidden` property is even more obscure - it applies only to formulas, and refers to hiding the property on the formula bar, not in the cell. I have added methods `isLocked` and `isHiddenOnFormulaBar` to `Cell`. I corrected the docs to explain this. And, as long as I was looking at the docs, I corrected some examples to use `getHighestDataRow/Column` rather than `getHighestRow/Column`, a frequent problem for users (e.g. PHPOffice#3721). As a side note, the change to Cell.php is my first use of the nullsafe operator. This is one of many new options available now that we require Php8.0+.
diff --git a/docs/topics/accessing-cells.md b/docs/topics/accessing-cells.md
@@ -479,8 +479,8 @@ $spreadsheet = $reader->load("test.xlsx");
 
 $worksheet = $spreadsheet->getActiveSheet();
 // Get the highest row and column numbers referenced in the worksheet
-$highestRow = $worksheet->getHighestRow(); // e.g. 10
-$highestColumn = $worksheet->getHighestColumn(); // e.g 'F'
+$highestRow = $worksheet->getHighestDataRow(); // e.g. 10
+$highestColumn = $worksheet->getHighestDataColumn(); // e.g 'F'
 $highestColumnIndex = \PhpOffice\PhpSpreadsheet\Cell\Coordinate::columnIndexFromString($highestColumn); // e.g. 5
 
 echo '<table>' . "\n";
@@ -505,8 +505,8 @@ $spreadsheet = $reader->load("test.xlsx");
 
 $worksheet = $spreadsheet->getActiveSheet();
 // Get the highest row number and column letter referenced in the worksheet
-$highestRow = $worksheet->getHighestRow(); // e.g. 10
-$highestColumn = $worksheet->getHighestColumn(); // e.g 'F'
+$highestRow = $worksheet->getHighestDataRow(); // e.g. 10
+$highestColumn = $worksheet->getHighestDataColumn(); // e.g 'F'
 // Increment the highest column letter
 $highestColumn++;
 
diff --git a/docs/topics/recipes.md b/docs/topics/recipes.md
@@ -1309,12 +1309,17 @@ when setting a new password.
 
 ### Cell
 
-An example on setting cell security:
+An example on setting cell security.
+Note that cell security is honored only when sheet is protected.
+Also note that the `hidden` property applies only to formulas,
+and tells whether the formula is hidden on the formula bar,
+not in the cell.
 
 ```php
 $spreadsheet->getActiveSheet()->getStyle('B1')
     ->getProtection()
-    ->setLocked(\PhpOffice\PhpSpreadsheet\Style\Protection::PROTECTION_UNPROTECTED);
+    ->setLocked(\PhpOffice\PhpSpreadsheet\Style\Protection::PROTECTION_UNPROTECTED)
+    ->setHidden(\PhpOffice\PhpSpreadsheet\Style\Protection::PROTECTION_PROTECTED);
 ```
 
 ## Reading protected spreadsheet
diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php
@@ -11,6 +11,7 @@
 use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
 use PhpOffice\PhpSpreadsheet\Style\ConditionalFormatting\CellStyleAssessor;
 use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
+use PhpOffice\PhpSpreadsheet\Style\Protection;
 use PhpOffice\PhpSpreadsheet\Style\Style;
 use PhpOffice\PhpSpreadsheet\Worksheet\Table;
 use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
@@ -805,4 +806,29 @@ public function getIgnoredErrors(): IgnoredErrors
     {
         return $this->ignoredErrors;
     }
+
+    public function isLocked(): bool
+    {
+        $sheet = $this->parent?->getParent();
+        if ($sheet === null || $sheet->getProtection()->getSheet() !== true) {
+            return false;
+        }
+        $locked = $this->getStyle()->getProtection()->getLocked();
+
+        return $locked !== Protection::PROTECTION_UNPROTECTED;
+    }
+
+    public function isHiddenOnFormulaBar(): bool
+    {
+        if ($this->getDataType() !== DataType::TYPE_FORMULA) {
+            return false;
+        }
+        $sheet = $this->parent?->getParent();
+        if ($sheet === null || $sheet->getProtection()->getSheet() !== true) {
+            return false;
+        }
+        $hidden = $this->getStyle()->getProtection()->getHidden();
+
+        return $hidden !== Protection::PROTECTION_UNPROTECTED;
+    }
 }
diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php
@@ -236,7 +236,7 @@ public function listWorksheetInfo($filename)
                 $relTarget = (string) $rel['Target'];
                 $dir = dirname($relTarget);
                 $namespace = dirname($relType);
-                $relsWorkbook = $this->loadZip("$dir/_rels/" . basename($relTarget) . '.rels', '');
+                $relsWorkbook = $this->loadZip("$dir/_rels/" . basename($relTarget) . '.rels', Namespaces::RELATIONSHIPS);
 
                 $worksheets = [];
                 foreach ($relsWorkbook->Relationship as $elex) {
@@ -556,7 +556,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                     $dir = dirname($relTarget);
 
                     // Do not specify namespace in next stmt - do it in Xpath
-                    $relsWorkbook = $this->loadZip("$dir/_rels/" . basename($relTarget) . '.rels', '');
+                    $relsWorkbook = $this->loadZip("$dir/_rels/" . basename($relTarget) . '.rels', Namespaces::RELATIONSHIPS);
                     $relsWorkbook->registerXPathNamespace('rel', Namespaces::RELATIONSHIPS);
 
                     $worksheets = [];
@@ -1342,9 +1342,10 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                                 $drawingFilename = substr($drawingFilename, 5);
                             }
                             if ($zip->locateName($drawingFilename) !== false) {
-                                $relsWorksheet = $this->loadZipNoNamespace($drawingFilename, Namespaces::RELATIONSHIPS);
+                                $relsWorksheet = $this->loadZip($drawingFilename, Namespaces::RELATIONSHIPS);
                                 $drawings = [];
-                                foreach ($relsWorksheet->Relationship as $ele) {
+                                foreach ($relsWorksheet->Relationship as $elex) {
+                                    $ele = self::getAttributes($elex);
                                     if ((string) $ele['Type'] === "$xmlNamespaceBase/drawing") {
                                         $eleTarget = (string) $ele['Target'];
                                         if (substr($eleTarget, 0, 4) === '/xl/') {
@@ -1362,12 +1363,13 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
                                         $drawingRelId = (string) self::getArrayItem(self::getAttributes($drawing, $xmlNamespaceBase), 'id');
                                         $fileDrawing = $drawings[$drawingRelId];
                                         $drawingFilename = dirname($fileDrawing) . '/_rels/' . basename($fileDrawing) . '.rels';
-                                        $relsDrawing = $this->loadZipNoNamespace($drawingFilename, $xmlNamespaceBase);
+                                        $relsDrawing = $this->loadZip($drawingFilename, Namespaces::RELATIONSHIPS);
 
                                         $images = [];
                                         $hyperlinks = [];
                                         if ($relsDrawing && $relsDrawing->Relationship) {
-                                            foreach ($relsDrawing->Relationship as $ele) {
+                                            foreach ($relsDrawing->Relationship as $elex) {
+                                                $ele = self::getAttributes($elex);
                                                 $eleType = (string) $ele['Type'];
                                                 if ($eleType === Namespaces::HYPERLINK) {
                                                     $hyperlinks[(string) $ele['Id']] = (string) $ele['Target'];
@@ -1607,7 +1609,8 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
 
                                     // store original rId of drawing files
                                     $unparsedLoadedData['sheets'][$docSheet->getCodeName()]['drawingOriginalIds'] = [];
-                                    foreach ($relsWorksheet->Relationship as $ele) {
+                                    foreach ($relsWorksheet->Relationship as $elex) {
+                                        $ele = self::getAttributes($elex);
                                         if ((string) $ele['Type'] === "$xmlNamespaceBase/drawing") {
                                             $drawingRelId = (string) $ele['Id'];
                                             $unparsedLoadedData['sheets'][$docSheet->getCodeName()]['drawingOriginalIds'][(string) $ele['Target']] = $drawingRelId;
diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue3720Test.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue3720Test.php
@@ -0,0 +1,97 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
+
+use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
+
+class Issue3720Test extends \PHPUnit\Framework\TestCase
+{
+    private static string $testbook = 'tests/data/Reader/XLSX/issue.3720.xlsx';
+
+    public function testPreliminaries(): void
+    {
+        $file = 'zip://';
+        $file .= self::$testbook;
+        $file .= '#xl/_rels/workbook.xml.rels';
+        $data = file_get_contents($file);
+        // confirm that file contains expected namespaced xml tag
+        if ($data === false) {
+            self::fail('Unable to read file');
+        } else {
+            self::assertStringContainsString('<ns3:Relationships ', $data);
+        }
+    }
+
+    public function testInfo(): void
+    {
+        $reader = new Xlsx();
+        $workSheetInfo = $reader->listWorkSheetInfo(self::$testbook);
+        $info1 = $workSheetInfo[1];
+        self::assertEquals('Welcome', $info1['worksheetName']);
+        self::assertEquals('H', $info1['lastColumnLetter']);
+        self::assertEquals(7, $info1['lastColumnIndex']);
+        self::assertEquals(49, $info1['totalRows']);
+        self::assertEquals(8, $info1['totalColumns']);
+    }
+
+    public function testSheetNames(): void
+    {
+        $reader = new Xlsx();
+        $worksheetNames = $reader->listWorksheetNames(self::$testbook);
+        $expected = [
+            'Data',
+            'Welcome',
+            'Sheet 1',
+            'Sheet 2',
+            'Sheet 3',
+            'Sheet 4',
+            'Sheet 5',
+            'Sheet 6',
+            'Sheet 7',
+            'Sheet 8',
+            'Sheet 9',
+            'Sheet 10',
+        ];
+        self::assertEquals($expected, $worksheetNames);
+    }
+
+    public function testLoadXlsx(): void
+    {
+        $reader = new Xlsx();
+        $spreadsheet = $reader->load(self::$testbook);
+        $sheets = $spreadsheet->getAllSheets();
+        self::assertCount(12, $sheets);
+        $sheet = $spreadsheet->getSheetByNameOrThrow('Sheet 1');
+        $sheetProtection = $sheet->getProtection();
+        self::assertTrue($sheetProtection->getSheet());
+        self::assertSame(' FILL IN WHITE CELLS ONLY', $sheet->getCell('B3')->getValue());
+        // inherit because no cell, row, or column style.
+        // effectively protected because sheet is locked.
+        self::assertTrue($sheet->getCell('A12')->isLocked());
+        // unprotected because column is unprotected (no cell or row style)
+        self::assertFalse($sheet->getCell('B12')->isLocked());
+        // inherit because cell has style with protection omitted.
+        // effectively protected because sheet is locked.
+        self::assertTrue($sheet->getCell('B11')->isLocked());
+        $sheet = $spreadsheet->getSheetByNameOrThrow('Welcome');
+        $drawings = $sheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        $failmsg = '';
+        if (isset($drawings[0])) {
+            $draw0 = $drawings[0];
+            if (method_exists($draw0, 'getPath')) {
+                self::assertSame('image1.jpeg', basename($draw0->getPath()));
+            } else {
+                $failmsg = 'unexpected missing getPath method';
+            }
+        } else {
+            $failmsg = 'unexpected missing array item 0';
+        }
+        $spreadsheet->disconnectWorksheets();
+        if ($failmsg !== '') {
+            self::fail($failmsg);
+        }
+    }
+}
diff --git a/tests/PhpSpreadsheetTests/Worksheet/Protection2Test.php b/tests/PhpSpreadsheetTests/Worksheet/Protection2Test.php
@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Style\Protection;
+use PHPUnit\Framework\TestCase;
+
+class Protection2Test extends TestCase
+{
+    public function testisHiddenOnFormulaBar(): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        $sheet->getCell('A1')->setValue('X')
+            ->getStyle()->getProtection()
+            ->setHidden(Protection::PROTECTION_UNPROTECTED);
+        $sheet->getCell('A2')->setValue('=SUM(1,2)')
+            ->getStyle()->getProtection()
+            ->setHidden(Protection::PROTECTION_UNPROTECTED);
+        $sheet->getCell('B1')->setValue('X')
+            ->getStyle()->getProtection()
+            ->setHidden(Protection::PROTECTION_PROTECTED);
+        $sheet->getCell('B2')->setValue('=SUM(1,2)')
+            ->getStyle()->getProtection()
+            ->setHidden(Protection::PROTECTION_PROTECTED);
+        $sheet->getCell('C1')->setValue('X');
+        $sheet->getCell('C2')->setValue('=SUM(1,2)');
+        self::assertFalse($sheet->getCell('A1')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('A2')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('B1')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('B2')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('C1')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('C2')->isHiddenOnFormulaBar());
+        $sheetProtection = $sheet->getProtection();
+        $sheetProtection->setSheet(true);
+        self::assertFalse($sheet->getCell('A1')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('A2')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('B1')->isHiddenOnFormulaBar(), 'not a formula1');
+        self::assertTrue($sheet->getCell('B2')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('C1')->isHiddenOnFormulaBar(), 'not a formula2');
+        self::assertTrue($sheet->getCell('C2')->isHiddenOnFormulaBar());
+        self::assertFalse($sheet->getCell('D1')->isHiddenOnFormulaBar(), 'uninitialized cell is not formula');
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public function testisLocked(): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        $sheet->getCell('A1')->setValue('X')
+            ->getStyle()->getProtection()
+            ->setLocked(Protection::PROTECTION_UNPROTECTED);
+        $sheet->getCell('A2')->setValue('=SUM(1,2)')
+            ->getStyle()->getProtection()
+            ->setLocked(Protection::PROTECTION_UNPROTECTED);
+        $sheet->getCell('B1')->setValue('X')
+            ->getStyle()->getProtection()
+            ->setLocked(Protection::PROTECTION_PROTECTED);
+        $sheet->getCell('B2')->setValue('=SUM(1,2)')
+            ->getStyle()->getProtection()
+            ->setLocked(Protection::PROTECTION_PROTECTED);
+        $sheet->getCell('C1')->setValue('X');
+        $sheet->getCell('C2')->setValue('=SUM(1,2)');
+        self::assertFalse($sheet->getCell('A1')->isLocked());
+        self::assertFalse($sheet->getCell('A2')->isLocked());
+        self::assertFalse($sheet->getCell('B1')->isLocked());
+        self::assertFalse($sheet->getCell('B2')->isLocked());
+        self::assertFalse($sheet->getCell('C1')->isLocked());
+        self::assertFalse($sheet->getCell('C2')->isLocked());
+        $sheetProtection = $sheet->getProtection();
+        $sheetProtection->setSheet(true);
+        self::assertFalse($sheet->getCell('A1')->isLocked());
+        self::assertFalse($sheet->getCell('A2')->isLocked());
+        self::assertTrue($sheet->getCell('B1')->isLocked());
+        self::assertTrue($sheet->getCell('B2')->isLocked());
+        self::assertTrue($sheet->getCell('C1')->isLocked());
+        self::assertTrue($sheet->getCell('C2')->isLocked());
+        self::assertTrue($sheet->getCell('D1')->isLocked(), 'uninitialized cell');
+        $spreadsheet->disconnectWorksheets();
+    }
+}
diff --git a/tests/data/Reader/XLSX/issue.3720.xlsx b/tests/data/Reader/XLSX/issue.3720.xlsx
