Merge pull request #15 from box/add_test_for_skipping_empty_rows

Add test for skipping empty rows

README.md

@@ -149,7 +149,7 @@ $writer->setShouldCreateNewSheetsAutomatically(true); // default value
 $writer->setShouldCreateNewSheetsAutomatically(false); // will stop writing new data when limit is reached
 ```
 
-### Using custom temporary folder
+#### Using custom temporary folder
 
 Processing XLSX files require temporary files to be created. By default, Spout will use the system default temporary folder (as returned by sys_get_temp_dir()). It is possible to override this by explicitly setting it on the reader or writer:
 ```php

composer.json

@@ -2,7 +2,7 @@
     "name": "box/spout",
     "description": "PHP Library to read and write CSV and XLSX files, in a fast and scalable way",
     "type": "library",
-    "version": "1.0.0",
+    "version": "1.0.2",
     "keywords": ["php","read","write","csv","xlsx","excel","spreadsheet","scale","memory","stream","ooxml"],
     "license": "Apache-2.0",
     "homepage": "https://www.github.com/box/spout",

src/Spout/Common/Escaper/XLSX.php

@@ -27,7 +27,7 @@ class XLSX implements EscaperInterface
     public function escape($string)
     {
         $escapedString = $this->escapeControlCharacters($string);
-        $escapedString = htmlspecialchars($escapedString, ENT_QUOTES, 'utf-8');
+        $escapedString = htmlspecialchars($escapedString, ENT_QUOTES);
 
         return $escapedString;
     }

src/Spout/Reader/CSV.php

@@ -52,7 +52,7 @@ class CSV extends AbstractReader
      * The file must be UTF-8 encoded.
      * @TODO add encoding detection/conversion
      *
-     * @param  string $filePath Path of the XLSX file to be read
+     * @param  string $filePath Path of the CSV file to be read
      * @return void
      * @throws \Box\Spout\Common\Exception\IOException
      */

src/Spout/Reader/Helper/XLSX/SharedStringsHelper.php

@@ -31,6 +31,9 @@ class SharedStringsHelper
      */
     const MAX_NUM_STRINGS_PER_TEMP_FILE = 10000;
 
+    /** Value to use to escape the line feed character ("\n") */
+    const ESCAPED_LINE_FEED_CHARACTER = '_x000A_';
+
     /** @var string Path of the XLSX file being read */
     protected $filePath;
 
@@ -80,7 +83,6 @@ class SharedStringsHelper
      * Please note that SimpleXML does not provide such a functionality but since it is faster
      * and more handy to parse few XML nodes, it is used in combination with XMLReader for that purpose.
      *
-     * @param  string $filePath
      * @return void
      * @throws \Box\Spout\Common\Exception\IOException If sharedStrings.xml can't be read
      */
@@ -120,7 +122,12 @@ class SharedStringsHelper
             }
 
             $unescapedTextValue = $escaper->unescape($textValue);
-            $this->writeSharedStringToTempFile($unescapedTextValue, $sharedStringIndex);
+
+            // The shared string retrieval logic expects each cell data to be on one line only
+            // Encoding the line feed character allows to preserve this assumption
+            $lineFeedEncodedTextValue = $this->escapeLineFeed($unescapedTextValue);
+
+            $this->writeSharedStringToTempFile($lineFeedEncodedTextValue, $sharedStringIndex);
 
             $sharedStringIndex++;
 
@@ -246,7 +253,8 @@ class SharedStringsHelper
 
         $sharedString = null;
         if (array_key_exists($indexInFile, $this->inMemoryTempFileContents)) {
-            $sharedString = $this->inMemoryTempFileContents[$indexInFile];
+            $escapedSharedString = $this->inMemoryTempFileContents[$indexInFile];
+            $sharedString = $this->unescapeLineFeed($escapedSharedString);
         }
 
         if (!$sharedString) {
@@ -256,6 +264,28 @@ class SharedStringsHelper
         return rtrim($sharedString, PHP_EOL);
     }
 
+    /**
+     * Escapes the line feed character (\n)
+     *
+     * @param string $unescapedString
+     * @return string
+     */
+    private function escapeLineFeed($unescapedString)
+    {
+        return str_replace("\n", self::ESCAPED_LINE_FEED_CHARACTER, $unescapedString);
+    }
+
+    /**
+     * Unescapes the line feed character (\n)
+     *
+     * @param string $escapedString
+     * @return string
+     */
+    private function unescapeLineFeed($escapedString)
+    {
+        return str_replace(self::ESCAPED_LINE_FEED_CHARACTER, "\n", $escapedString);
+    }
+
     /**
      * Deletes the created temporary folder and all its contents
      *

src/Spout/Reader/Internal/XLSX/Worksheet.php

@@ -31,7 +31,7 @@ class Worksheet
      */
     public function getDataXmlFilePath()
     {
-        return ltrim($this->dataXmlFilePath, DIRECTORY_SEPARATOR);
+        return ltrim($this->dataXmlFilePath, '/');
     }
 
     /**

src/Spout/Reader/XLSX.php

@@ -190,9 +190,11 @@ class XLSX extends AbstractReader
         while ($this->xmlReader->read()) {
             if ($this->xmlReader->nodeType == \XMLReader::ELEMENT && $this->xmlReader->name === 'dimension') {
                 // Read dimensions of the worksheet
-                $dimensionRef = $this->xmlReader->getAttribute('ref'); // returns 'A1:M13' for instance
+                $dimensionRef = $this->xmlReader->getAttribute('ref'); // returns 'A1:M13' for instance (or 'A1' for empty sheet)
-                list(, $lastCellIndex) = explode(':', $dimensionRef);
+                if (preg_match('/[A-Z\d]+:([A-Z\d]+)/', $dimensionRef, $matches)) {
-                $this->numberOfColumns = CellHelper::getColumnIndexFromCellIndex($lastCellIndex) + 1;
+                    $lastCellIndex = $matches[1];
+                    $this->numberOfColumns = CellHelper::getColumnIndexFromCellIndex($lastCellIndex) + 1;
+                }
             } else if ($this->xmlReader->nodeType == \XMLReader::ELEMENT && $this->xmlReader->name === 'row') {
                 // Start of the row description
                 $isInsideRowTag = true;

src/Spout/Writer/AbstractWriter.php

@@ -39,6 +39,7 @@ abstract class AbstractWriter implements WriterInterface
 
     /**
      * Adds data to the currently openned writer.
+     * The data must be UTF-8 encoded.
      *
      * @param  array $dataRow Array containing data to be streamed.
      *          Example $dataRow = ['data1', 1234, null, '', 'data5'];
@@ -135,9 +136,11 @@ abstract class AbstractWriter implements WriterInterface
 
     /**
      * Write given data to the output. New data will be appended to end of stream.
+     * The data must be UTF-8 encoded.
      *
      * @param  array $dataRow Array containing data to be streamed.
-     *          Example $dataRow = ['data1', 1234, null, '', 'data5'];
+     *                        If empty, no data is added (i.e. not even as a blank row)
+     *                        Example: $dataRow = ['data1', 1234, null, '', 'data5', false];
      *
      * @return \Box\Spout\Writer\AbstractWriter
      * @throws \Box\Spout\Writer\Exception\WriterNotOpenedException If this function is called before opening the writer
@@ -146,7 +149,10 @@ abstract class AbstractWriter implements WriterInterface
     public function addRow(array $dataRow)
     {
         if ($this->isWriterOpened) {
-            $this->addRowToWriter($dataRow);
+            // empty $dataRow should not add an empty line
+            if (!empty($dataRow)) {
+                $this->addRowToWriter($dataRow);
+            }
         } else {
             throw new WriterNotOpenedException('The writer needs to be opened before adding row.');
         }
@@ -156,12 +162,14 @@ abstract class AbstractWriter implements WriterInterface
 
     /**
      * Write given data to the output. New data will be appended to end of stream.
+     * The data must be UTF-8 encoded.
      *
      * @param  array $dataRows Array of array containing data to be streamed.
-     *          Example $dataRow = [
+     *                         If a row is empty, it won't be added (i.e. not even as a blank row)
-     *              ['data11', 12, , '', 'data13'],
+     *                         Example: $dataRows = [
-     *              ['data21', 'data22', null],
+     *                             ['data11', 12, , '', 'data13'],
-     *          ];
+     *                             ['data21', 'data22', null, false],
+     *                         ];
      *
      * @return \Box\Spout\Writer\AbstractWriter
      * @throws \Box\Spout\Common\Exception\InvalidArgumentException If the input param is not valid

src/Spout/Writer/Internal/XLSX/Workbook.php

@@ -5,7 +5,6 @@ namespace Box\Spout\Writer\Internal\XLSX;
 use Box\Spout\Writer\Exception\SheetNotFoundException;
 use Box\Spout\Writer\Helper\XLSX\FileSystemHelper;
 use Box\Spout\Writer\Helper\XLSX\SharedStringsHelper;
-use Box\Spout\Writer\Helper\XLSX\ZipHelper;
 use Box\Spout\Writer\Sheet;
 
 /**

src/Spout/Writer/Internal/XLSX/Worksheet.php

@@ -42,7 +42,8 @@ EOD;
 
     /**
      * @param \Box\Spout\Writer\Sheet $externalSheet The associated "external" sheet
-     * @param string $tempFolder Temporary folder where the files to create the XLSX will be stored
+     * @param string $worksheetFilesFolder Temporary folder where the files to create the XLSX will be stored
+     * @param \Box\Spout\Writer\Helper\XLSX\SharedStringsHelper $sharedStringsHelper Helper for shared strings
      * @param bool $shouldUseInlineStrings Whether inline or shared strings should be used
      * @throws \Box\Spout\Common\Exception\IOException If the sheet data file cannot be opened for writing
      */

tests/Spout/Reader/Helper/XLSX/SharedStringsHelperTest.php

@@ -46,7 +46,7 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(1, count($filesInTempFolder), 'One temp file should have been created in the temp folder.');
 
         $tempFileContents = file_get_contents($filesInTempFolder[0]);
-        $tempFileContentsPerLine = explode("\n", $tempFileContents);
+        $tempFileContentsPerLine = explode(PHP_EOL, $tempFileContents);
 
         $this->assertEquals('s1--A1', $tempFileContentsPerLine[0]);
         $this->assertEquals('s1--E5', $tempFileContentsPerLine[24]);
@@ -96,4 +96,23 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
         $sharedString = $this->sharedStringsHelper->getStringAtIndex(24);
         $this->assertEquals('s1--E5', $sharedString);
     }
+
+    /**
+     * @return void
+     */
+    public function testGetStringAtIndexShouldWorkWithMultilineStrings()
+    {
+        $resourcePath = $this->getResourcePath('one_sheet_with_shared_multiline_strings.xlsx');
+        $sharedStringsHelper = new SharedStringsHelper($resourcePath);
+
+        $sharedStringsHelper->extractSharedStrings();
+
+        $sharedString = $sharedStringsHelper->getStringAtIndex(0);
+        $this->assertEquals("s1\nA1", $sharedString);
+
+        $sharedString = $sharedStringsHelper->getStringAtIndex(24);
+        $this->assertEquals("s1\nE5", $sharedString);
+
+        $sharedStringsHelper->cleanup();
+    }
 }

tests/Spout/Reader/XLSXTest.php

@@ -165,6 +165,15 @@ class XLSXTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals($expectedFirstRow, $allRows[0], 'Entities should be ignored when reading XML files.');
     }
 
+    /**
+     * @return void
+     */
+    public function testReadShouldBeAbleToProcessEmptySheets()
+    {
+        $allRows = $this->getAllRowsForFile('sheet_with_no_cells.xlsx');
+        $this->assertEquals([], $allRows, 'Sheet with no cells should be correctly processed.');
+    }
+
     /**
      * @param string $fileName
      * @return array All the read rows the given file

tests/Spout/Writer/CSVTest.php

@@ -76,6 +76,22 @@ class CSVTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('csv--11,,csv--13', $writtenContent, 'The null values should be replaced by empty values');
     }
 
+    /**
+     * @return void
+     */
+    public function testWriteShouldSkipEmptyRows()
+    {
+        $allRows = [
+            ['csv--11', 'csv--12'],
+            [],
+            ['csv--31', 'csv--32'],
+        ];
+        $writtenContent = $this->writeToCsvFileAndReturnWrittenContent($allRows, 'csv_with_empty_rows.csv');
+        $writtenContent = $this->trimWrittenContent($writtenContent);
+
+        $this->assertEquals("csv--11,csv--12\ncsv--31,csv--32", $writtenContent, 'Empty rows should be skipped');
+    }
+
     /**
      * @return void
      */
@@ -135,6 +151,6 @@ class CSVTest extends \PHPUnit_Framework_TestCase
     private function trimWrittenContent($writtenContent)
     {
         // remove line feeds and UTF-8 BOM
-        return trim($writtenContent, "\n" . CSV::UTF8_BOM);
+        return trim($writtenContent, PHP_EOL . CSV::UTF8_BOM);
     }
 }