Merge pull request #189 from madflow/ods-missing-values

Fixes for #184

src/Spout/Reader/ODS/RowIterator.php

@@ -20,6 +20,7 @@ class RowIterator implements IteratorInterface
     const XML_NODE_TABLE = 'table:table';
     const XML_NODE_ROW = 'table:table-row';
     const XML_NODE_CELL = 'table:table-cell';
+    const MAX_COLUMNS_EXCEL = 16384;
 
     /** Definition of XML attribute used to parse data */
     const XML_ATTRIBUTE_NUM_COLUMNS_REPEATED = 'table:number-columns-repeated';
@@ -127,22 +128,23 @@ class RowIterator implements IteratorInterface
 
                 } else if ($this->xmlReader->isPositionedOnEndingNode(self::XML_NODE_ROW)) {
                     // End of the row description
-                    $isEmptyRow = ($numCellsRead <= 1 && empty($cellValue));
+                    $isEmptyRow = ($numCellsRead <= 1 && $this->isEmptyCellValue($cellValue));
                     if ($isEmptyRow) {
                         // skip empty rows
                         $this->next();
                         return;
                     }
 
-                    // Only add value if the last read cell is not empty or does not need to repeat cell values.
+                    // Only add the value if the last read cell is not a trailing empty cell repeater in Excel.
+                    // The current count of read columns is determined by counting the values in $rowData.
                     // This is to avoid creating a lot of empty cells, as Excel adds a last empty "<table:table-cell>"
                     // with a number-columns-repeated value equals to the number of (supported columns - used columns).
-                    // In Excel, the number of supported columns is 16384, but we don't want to returns rows with always 16384 cells.
+                    // In Excel, the number of supported columns is 16384, but we don't want to returns rows with
-                    if (!empty($cellValue) || $numColumnsRepeated === 1) {
+                    // always 16384 cells.
+                    if ((count($rowData) + $numColumnsRepeated) !== self::MAX_COLUMNS_EXCEL) {
                         for ($i = 0; $i < $numColumnsRepeated; $i++) {
                             $rowData[] = $cellValue;
                         }
-
                         $this->numReadRows++;
                     }
                     break;
@@ -181,6 +183,17 @@ class RowIterator implements IteratorInterface
         return $this->cellValueFormatter->extractAndFormatNodeValue($node);
     }
 
+    /**
+     * empty() replacement that honours 0 as a valid value
+     *
+     * @param $value The cell value
+     * @return bool
+     */
+    protected function isEmptyCellValue($value)
+    {
+        return (!isset($value) || trim($value) === '');
+    }
+
     /**
      * Return the current element, from the buffer.
      * @link http://php.net/manual/en/iterator.current.php

tests/Spout/Reader/CSV/ReaderTest.php

@@ -343,6 +343,38 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals($expectedRows, $allRows);
     }
 
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldInludeRowsWithZerosOnly()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_zeros_in_row.csv');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['1', '2', '3'],
+            ['0', '0', '0']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be only 3 rows, because zeros (0) are valid values');
+    }
+
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldCreateOutputEmptyCellPreserved()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_empty_cells.csv');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['0', '', ''],
+            ['1', '1', '']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be 3 rows, with equal length');
+    }
+
     /**
      * @param string $fileName
      * @param string|void $fieldDelimiter

tests/Spout/Reader/ODS/ReaderTest.php

@@ -387,6 +387,38 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $reader->open('php://memory');
     }
 
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldInludeRowsWithZerosOnly()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_zeros_in_row.ods');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['1', '2', '3'],
+            ['0', '0', '0']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be only 3 rows, because zeros (0) are valid values');
+    }
+
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldCreateOutputEmptyCellPreserved()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_empty_cells.ods');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['0', '', ''],
+            ['1', '1', '']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be 3 rows, with equal length');
+    }
+
     /**
      * @param string $fileName
      * @return array All the read rows the given file

tests/Spout/Reader/XLSX/ReaderTest.php

@@ -428,6 +428,39 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $reader->open('php://memory');
     }
 
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldInludeRowsWithZerosOnly()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_zeros_in_row.xlsx');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['1', '2', '3'],
+            ['0', '0', '0']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be only 3 rows, because zeros (0) are valid values');
+    }
+
+    /**
+     * https://github.com/box/spout/issues/184
+     * @return void
+     */
+    public function testReadShouldCreateOutputEmptyCellPreserved()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_empty_cells.xlsx');
+
+        $expectedRows = [
+            ['A', 'B', 'C'],
+            ['0', '', ''],
+            ['1', '1', '']
+        ];
+        $this->assertEquals($expectedRows, $allRows, 'There should be 3 rows, with equal length');
+    }
+
+
     /**
      * @param string $fileName
      * @return array All the read rows the given file

tests/resources/csv/sheet_with_empty_cells.csv

@@ -0,0 +1,3 @@
+"A","B","C"
+0,,
+1,1,

tests/resources/csv/sheet_with_zeros_in_row.csv

@@ -0,0 +1,3 @@
+"A","B","C"
+1,2,3
+0,0,0