Improve Coverage for ODS Reader (#1545)

* Improve Coverage for ODS Reader

Reader/ODS/Properties is now 100% covered.
Reader/ODS is covered except for 1 statement. As the original author
put it, "table-header-rows TODO: figure this out ... I'm not sure that
PhpExcel has an API for this". I'm still thinking about it, but, so far,
I agree with the author.

There are minimal code changes.
- Several places test !zip->open() to see whether the test failed.
  However, zip->open() returns true or a string, so the test never
  detects failure. Change to zip->open() !== true. No previous tests.
- Suppress warning messages from simplexml_load_string (there had
  been no tests for invalid xml).
- One document property was misnamed, and one non-existent property
  was tested for.

I added a number of tests, creating an ODS directory, and moving
OdsTest to that directory.

* Scrutinizer Recommendation

Unused variable in one test.

* Update CHANGELOG

Co-authored-by: Adrien Crivelli <adrien.crivelli@gmail.com>

CHANGELOG.md

@@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 
 ### Changed
 
-- Nothing.
+- Improve Coverage for ODS Reader [#1545](https://github.com/phpoffice/phpspreadsheet/pull/1544)
 
 ### Deprecated
 

src/PhpSpreadsheet/Reader/Ods.php

@@ -11,6 +11,7 @@ use DOMNode;
 use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
 use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
 use PhpOffice\PhpSpreadsheet\Cell\DataType;
+use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException;
 use PhpOffice\PhpSpreadsheet\Reader\Ods\PageSettings;
 use PhpOffice\PhpSpreadsheet\Reader\Ods\Properties as DocumentProperties;
 use PhpOffice\PhpSpreadsheet\Reader\Security\XmlScanner;
@@ -76,11 +77,9 @@ class Ods extends BaseReader
             }
 
             $zip->close();
-
-            return $mimeType === 'application/vnd.oasis.opendocument.spreadsheet';
         }
 
-        return false;
+        return $mimeType === 'application/vnd.oasis.opendocument.spreadsheet';
     }
 
     /**
@@ -95,8 +94,8 @@ class Ods extends BaseReader
         File::assertFile($pFilename);
 
         $zip = new ZipArchive();
-        if (!$zip->open($pFilename)) {
+        if ($zip->open($pFilename) !== true) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! Error opening file.');
+            throw new ReaderException('Could not open ' . $pFilename . ' for reading! Error opening file.');
         }
 
         $worksheetNames = [];
@@ -149,8 +148,8 @@ class Ods extends BaseReader
         $worksheetInfo = [];
 
         $zip = new ZipArchive();
-        if (!$zip->open($pFilename)) {
+        if ($zip->open($pFilename) !== true) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! Error opening file.');
+            throw new ReaderException('Could not open ' . $pFilename . ' for reading! Error opening file.');
         }
 
         $xml = new XMLReader();
@@ -198,18 +197,18 @@ class Ods extends BaseReader
                             // Step into the row
                             $xml->read();
                             do {
+                                $doread = true;
                                 if ($xml->name == 'table:table-cell' && $xml->nodeType == XMLReader::ELEMENT) {
                                     if (!$xml->isEmptyElement) {
                                         ++$currCells;
                                         $xml->next();
-                                    } else {
+                                        $doread = false;
-                                        $xml->read();
                                     }
                                 } elseif ($xml->name == 'table:covered-table-cell' && $xml->nodeType == XMLReader::ELEMENT) {
                                     $mergeSize = $xml->getAttribute('table:number-columns-repeated');
                                     $currCells += (int) $mergeSize;
-                                    $xml->read();
+                                }
-                                } else {
+                                if ($doread) {
                                     $xml->read();
                                 }
                             } while ($xml->name != 'table:table-row');
@@ -258,13 +257,13 @@ class Ods extends BaseReader
         $GMT = new DateTimeZone('UTC');
 
         $zip = new ZipArchive();
-        if (!$zip->open($pFilename)) {
+        if ($zip->open($pFilename) !== true) {
             throw new Exception("Could not open {$pFilename} for reading! Error opening file.");
         }
 
         // Meta
 
-        $xml = simplexml_load_string(
+        $xml = @simplexml_load_string(
             $this->securityScanner->scan($zip->getFromName('meta.xml')),
             'SimpleXMLElement',
             Settings::getLibXmlLoaderOptions()
@@ -465,9 +464,10 @@ class Ods extends BaseReader
                                             $type = DataType::TYPE_NUMERIC;
                                             $dataValue = (float) $cellData->getAttributeNS($officeNs, 'value');
 
-                                            if (floor($dataValue) == $dataValue) {
+                                            // percentage should always be float
-                                                $dataValue = (int) $dataValue;
+                                            //if (floor($dataValue) == $dataValue) {
-                                            }
+                                            //    $dataValue = (int) $dataValue;
+                                            //}
                                             $formatting = NumberFormat::FORMAT_PERCENTAGE_00;
 
                                             break;
@@ -488,8 +488,6 @@ class Ods extends BaseReader
                                             if (floor($dataValue) == $dataValue) {
                                                 if ($dataValue == (int) $dataValue) {
                                                     $dataValue = (int) $dataValue;
-                                                } else {
-                                                    $dataValue = (float) $dataValue;
                                                 }
                                             }
 

src/PhpSpreadsheet/Reader/Ods/Properties.php

@@ -54,15 +54,11 @@ class Properties
                     $docProps->setLastModifiedBy($propertyValue);
 
                     break;
-                case 'creation-date':
+                case 'date':
                     $creationDate = strtotime($propertyValue);
                     $docProps->setCreated($creationDate);
                     $docProps->setModified($creationDate);
 
-                    break;
-                case 'keyword':
-                    $docProps->setKeywords($propertyValue);
-
                     break;
                 case 'description':
                     $docProps->setDescription($propertyValue);

tests/PhpSpreadsheetTests/Reader/Ods/OdsInfoTest.php

@@ -0,0 +1,110 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Ods;
+
+use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException;
+use PhpOffice\PhpSpreadsheet\Reader\Ods;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * @TODO The class doesn't read the bold/italic/underline properties (rich text)
+ */
+class OdsInfoTest extends TestCase
+{
+    public function testReadFileProperties(): void
+    {
+        $filename = 'tests/data/Reader/Ods/data.ods';
+
+        // Load into this instance
+        $reader = new Ods();
+
+        // Test "listWorksheetNames" method
+
+        self::assertEquals([
+            'Sheet1',
+            'Second Sheet',
+        ], $reader->listWorksheetNames($filename));
+    }
+
+    public function testNoMimeType(): void
+    {
+        $filename = 'tests/data/Reader/Ods/nomimetype.ods';
+
+        // Load into this instance
+        $reader = new Ods();
+
+        self::assertTrue($reader->canRead($filename));
+    }
+
+    public function testReadBadFileProperties(): void
+    {
+        $this->expectException(ReaderException::class);
+
+        // Load into this instance
+        $reader = new Ods();
+
+        // Test "listWorksheetNames" method
+
+        self::assertEquals([
+            'Sheet1',
+            'Second Sheet',
+        ], $reader->listWorksheetNames(__FILE__));
+    }
+
+    public function testReadFileInfo(): void
+    {
+        $filename = 'tests/data/Reader/Ods/data.ods';
+
+        // Load into this instance
+        $reader = new Ods();
+
+        // Test "listWorksheetNames" method
+
+        $wsinfo = $reader->listWorkSheetInfo($filename);
+        self::assertEquals([
+            [
+                'worksheetName' => 'Sheet1',
+                'lastColumnLetter' => 'C',
+                'lastColumnIndex' => 2,
+                'totalRows' => 12,
+                'totalColumns' => 3,
+            ],
+            [
+                'worksheetName' => 'Second Sheet',
+                'lastColumnLetter' => 'A',
+                'lastColumnIndex' => 0,
+                'totalRows' => 2,
+                'totalColumns' => 1,
+            ],
+        ], $wsinfo);
+    }
+
+    public function testReadBadFileInfo(): void
+    {
+        $this->expectException(ReaderException::class);
+        $filename = __FILE__;
+
+        // Load into this instance
+        $reader = new Ods();
+
+        // Test "listWorksheetNames" method
+
+        $wsinfo = $reader->listWorkSheetInfo($filename);
+        self::assertEquals([
+            [
+                'worksheetName' => 'Sheet1',
+                'lastColumnLetter' => 'C',
+                'lastColumnIndex' => 2,
+                'totalRows' => 11,
+                'totalColumns' => 3,
+            ],
+            [
+                'worksheetName' => 'Second Sheet',
+                'lastColumnLetter' => 'A',
+                'lastColumnIndex' => 0,
+                'totalRows' => 2,
+                'totalColumns' => 1,
+            ],
+        ], $wsinfo);
+    }
+}

tests/PhpSpreadsheetTests/Reader/Ods/OdsTest.php

@@ -1,9 +1,10 @@
 <?php
 
-namespace PhpOffice\PhpSpreadsheetTests\Reader;
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Ods;
 
 use PhpOffice\PhpSpreadsheet\Cell\DataType;
 use PhpOffice\PhpSpreadsheet\Document\Properties;
+use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException;
 use PhpOffice\PhpSpreadsheet\Reader\Ods;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Style\Font;
@@ -50,30 +51,66 @@ class OdsTest extends TestCase
 
             // Load into this instance
             $reader = new Ods();
-            $this->spreadsheetData = $reader->loadIntoExisting($filename, new Spreadsheet());
+            $this->spreadsheetData = $reader->load($filename);
         }
 
         return $this->spreadsheetData;
     }
 
-    public function testReadFileProperties(): void
+    public function testLoadWorksheets(): void
+    {
+        $spreadsheet = $this->loadDataFile();
+
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Spreadsheet', $spreadsheet);
+
+        self::assertEquals(2, $spreadsheet->getSheetCount());
+
+        $firstSheet = $spreadsheet->getSheet(0);
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Worksheet\Worksheet', $firstSheet);
+
+        $secondSheet = $spreadsheet->getSheet(1);
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Worksheet\Worksheet', $secondSheet);
+        self::assertEquals('Sheet1', $spreadsheet->getSheet(0)->getTitle());
+        self::assertEquals('Second Sheet', $spreadsheet->getSheet(1)->getTitle());
+    }
+
+    public function testLoadOneWorksheet(): void
     {
         $filename = 'tests/data/Reader/Ods/data.ods';
 
         // Load into this instance
         $reader = new Ods();
+        $reader->setLoadSheetsOnly(['Sheet1']);
+        $spreadsheet = $reader->load($filename);
 
-        // Test "listWorksheetNames" method
+        self::assertEquals(1, $spreadsheet->getSheetCount());
 
-        self::assertEquals([
+        self::assertEquals('Sheet1', $spreadsheet->getSheet(0)->getTitle());
-            'Sheet1',
-            'Second Sheet',
-        ], $reader->listWorksheetNames($filename));
     }
 
-    public function testLoadWorksheets(): void
+    public function testLoadBadFile(): void
     {
-        $spreadsheet = $this->loadDataFile();
+        $this->expectException(ReaderException::class);
+        $reader = new Ods();
+        $spreadsheet = $reader->load(__FILE__);
+
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Spreadsheet', $spreadsheet);
+
+        self::assertEquals(2, $spreadsheet->getSheetCount());
+
+        $firstSheet = $spreadsheet->getSheet(0);
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Worksheet\Worksheet', $firstSheet);
+
+        $secondSheet = $spreadsheet->getSheet(1);
+        self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Worksheet\Worksheet', $secondSheet);
+    }
+
+    public function testLoadCorruptFile(): void
+    {
+        $this->expectException(ReaderException::class);
+        $filename = 'tests/data/Reader/Ods/corruptMeta.ods';
+        $reader = new Ods();
+        $spreadsheet = $reader->load($filename);
 
         self::assertInstanceOf('PhpOffice\PhpSpreadsheet\Spreadsheet', $spreadsheet);