Fix crash when using associative array with empty row (#354)

* Used ENT_DISALLOWED when escaping a string.
* Added workaround for environments where ENT_DISALLOWED is not defined

README.md

@@ -3,6 +3,7 @@
 [![Latest Stable Version](https://poser.pugx.org/box/spout/v/stable)](https://packagist.org/packages/box/spout)
 [![Project Status](http://opensource.box.com/badges/active.svg)](http://opensource.box.com/badges)
 [![Build Status](https://travis-ci.org/box/spout.svg?branch=master)](https://travis-ci.org/box/spout)
+[![Scrutinizer Code Quality](https://scrutinizer-ci.com/g/box/spout/badges/quality-score.png?b=master)](https://scrutinizer-ci.com/g/box/spout/?branch=master)
 [![Code Coverage](https://scrutinizer-ci.com/g/box/spout/badges/coverage.png?b=master)](https://scrutinizer-ci.com/g/box/spout/?branch=master)
 [![Total Downloads](https://poser.pugx.org/box/spout/downloads)](https://packagist.org/packages/box/spout)
 

composer.json

@@ -31,7 +31,7 @@
     },
     "extra": {
         "branch-alias": {
-            "dev-master": "2.7.x-dev"
+            "dev-master": "2.8.x-dev"
         }
     }
 

src/Spout/Common/Escaper/ODS.php

@@ -14,6 +14,31 @@ class ODS implements EscaperInterface
 {
     use Singleton;
 
+    /** @var string Regex pattern to detect control characters that need to be escaped */
+    protected $escapableControlCharactersPattern;
+
+    /**
+     * Initializes the singleton instance
+     */
+    protected function init()
+    {
+        $this->escapableControlCharactersPattern = $this->getEscapableControlCharactersPattern();
+    }
+
+    /**
+     * @return string Regex pattern containing all escapable control characters
+     */
+    protected function getEscapableControlCharactersPattern()
+    {
+        // control characters values are from 0 to 1F (hex values) in the ASCII table
+        // some characters should not be escaped though: "\t", "\r" and "\n".
+        return '[\x00-\x08' .
+        // skipping "\t" (0x9) and "\n" (0xA)
+        '\x0B-\x0C' .
+        // skipping "\r" (0xD)
+        '\x0E-\x1F]';
+    }
+
     /**
      * Escapes the given string to make it compatible with XLSX
      *
@@ -22,7 +47,17 @@ class ODS implements EscaperInterface
      */
     public function escape($string)
     {
-        return htmlspecialchars($string, ENT_QUOTES);
+        if (defined('ENT_DISALLOWED')) {
+            // 'ENT_DISALLOWED' ensures that invalid characters in the given document type are replaced.
+            // Otherwise characters like a vertical tab "\v" will make the XML document unreadable by the XML processor
+            // @link https://github.com/box/spout/issues/329
+            return htmlspecialchars($string, ENT_QUOTES | ENT_DISALLOWED);
+        } else {
+            // We are on hhvm or any other engine that does not support ENT_DISALLOWED
+            $escapedString =  htmlspecialchars($string, ENT_QUOTES);
+            $replacedString = preg_replace('/'.$this->escapableControlCharactersPattern.'/', '�', $escapedString);
+            return $replacedString;
+        }
     }
 
     /**

src/Spout/Writer/ODS/Helper/StyleHelper.php

@@ -214,13 +214,46 @@ EOD;
      */
     protected function getStyleSectionContent($style)
     {
-        $defaultStyle = $this->getDefaultStyle();
         $styleIndex = $style->getId() + 1; // 1-based
 
         $content = '<style:style style:data-style-name="N0" style:family="table-cell" style:name="ce' . $styleIndex . '" style:parent-style-name="Default">';
 
+        $content .= $this->getTextPropertiesSectionContent($style);
+        $content .= $this->getTableCellPropertiesSectionContent($style);
+
+        $content .= '</style:style>';
+
+        return $content;
+    }
+
+    /**
+     * Returns the contents of the "<style:text-properties>" section, inside "<style:style>" section
+     *
+     * @param \Box\Spout\Writer\Style\Style $style
+     * @return string
+     */
+    private function getTextPropertiesSectionContent($style)
+    {
+        $content = '';
+
         if ($style->shouldApplyFont()) {
-            $content .= '<style:text-properties';
+            $content .= $this->getFontSectionContent($style);
+        }
+
+        return $content;
+    }
+
+    /**
+     * Returns the contents of the "<style:text-properties>" section, inside "<style:style>" section
+     *
+     * @param \Box\Spout\Writer\Style\Style $style
+     * @return string
+     */
+    private function getFontSectionContent($style)
+    {
+        $defaultStyle = $this->getDefaultStyle();
+
+        $content = '<style:text-properties';
 
         $fontColor = $style->getFontColor();
         if ($fontColor !== $defaultStyle->getFontColor()) {
@@ -251,27 +284,73 @@ EOD;
         }
 
         $content .= '/>';
-        }
-
-        if ($style->shouldWrapText()) {
-            $content .= '<style:table-cell-properties fo:wrap-option="wrap" style:vertical-align="automatic"/>';
-        }
-
-        if ($style->shouldApplyBorder()) {
-            $borderProperty = '<style:table-cell-properties %s />';
-            $borders = array_map(function (BorderPart $borderPart) {
-                return BorderHelper::serializeBorderPart($borderPart);
-            }, $style->getBorder()->getParts());
-            $content .= sprintf($borderProperty, implode(' ', $borders));
-        }
-
-        if ($style->shouldApplyBackgroundColor()) {
-            $content .= sprintf('
-                <style:table-cell-properties fo:background-color="#%s"/>', $style->getBackgroundColor());
-        }
-
-        $content .= '</style:style>';
 
         return $content;
     }
+
+    /**
+     * Returns the contents of the "<style:table-cell-properties>" section, inside "<style:style>" section
+     *
+     * @param \Box\Spout\Writer\Style\Style $style
+     * @return string
+     */
+    private function getTableCellPropertiesSectionContent($style)
+    {
+        $content = '';
+
+        if ($style->shouldWrapText()) {
+            $content .= $this->getWrapTextXMLContent();
+        }
+
+        if ($style->shouldApplyBorder()) {
+            $content .= $this->getBorderXMLContent($style);
+        }
+
+        if ($style->shouldApplyBackgroundColor()) {
+            $content .= $this->getBackgroundColorXMLContent($style);
+        }
+
+        return $content;
+    }
+
+    /**
+     * Returns the contents of the wrap text definition for the "<style:table-cell-properties>" section
+     *
+     * @return string
+     */
+    private function getWrapTextXMLContent()
+    {
+        return '<style:table-cell-properties fo:wrap-option="wrap" style:vertical-align="automatic"/>';
+    }
+
+    /**
+     * Returns the contents of the borders definition for the "<style:table-cell-properties>" section
+     *
+     * @param \Box\Spout\Writer\Style\Style $style
+     * @return string
+     */
+    private function getBorderXMLContent($style)
+    {
+        $borderProperty = '<style:table-cell-properties %s />';
+
+        $borders = array_map(function (BorderPart $borderPart) {
+            return BorderHelper::serializeBorderPart($borderPart);
+        }, $style->getBorder()->getParts());
+
+        return sprintf($borderProperty, implode(' ', $borders));
+    }
+
+    /**
+     * Returns the contents of the background color definition for the "<style:table-cell-properties>" section
+     *
+     * @param \Box\Spout\Writer\Style\Style $style
+     * @return string
+     */
+    private function getBackgroundColorXMLContent($style)
+    {
+        return sprintf(
+            '<style:table-cell-properties fo:background-color="#%s"/>',
+            $style->getBackgroundColor()
+        );
+    }
 }

src/Spout/Writer/Style/Style.php

@@ -106,6 +106,7 @@ class Style
 
     /**
      * @param Border $border
+     * @return Style
      */
     public function setBorder(Border $border)
     {
@@ -359,37 +360,67 @@ class Style
     {
         $mergedStyle = clone $this;
 
-        if (!$this->hasSetFontBold && $baseStyle->isFontBold()) {
-            $mergedStyle->setFontBold();
-        }
-        if (!$this->hasSetFontItalic && $baseStyle->isFontItalic()) {
-            $mergedStyle->setFontItalic();
-        }
-        if (!$this->hasSetFontUnderline && $baseStyle->isFontUnderline()) {
-            $mergedStyle->setFontUnderline();
-        }
-        if (!$this->hasSetFontStrikethrough && $baseStyle->isFontStrikethrough()) {
-            $mergedStyle->setFontStrikethrough();
-        }
-        if (!$this->hasSetFontSize && $baseStyle->getFontSize() !== self::DEFAULT_FONT_SIZE) {
-            $mergedStyle->setFontSize($baseStyle->getFontSize());
-        }
-        if (!$this->hasSetFontColor && $baseStyle->getFontColor() !== self::DEFAULT_FONT_COLOR) {
-            $mergedStyle->setFontColor($baseStyle->getFontColor());
-        }
-        if (!$this->hasSetFontName && $baseStyle->getFontName() !== self::DEFAULT_FONT_NAME) {
-            $mergedStyle->setFontName($baseStyle->getFontName());
-        }
-        if (!$this->hasSetWrapText && $baseStyle->shouldWrapText()) {
-            $mergedStyle->setShouldWrapText();
-        }
-        if (!$this->getBorder() && $baseStyle->shouldApplyBorder()) {
-            $mergedStyle->setBorder($baseStyle->getBorder());
-        }
-        if (!$this->hasSetBackgroundColor && $baseStyle->shouldApplyBackgroundColor()) {
-            $mergedStyle->setBackgroundColor($baseStyle->getBackgroundColor());
-        }
+        $this->mergeFontStyles($mergedStyle, $baseStyle);
+        $this->mergeOtherFontProperties($mergedStyle, $baseStyle);
+        $this->mergeCellProperties($mergedStyle, $baseStyle);
 
         return $mergedStyle;
     }
+
+    /**
+     * @param Style $styleToUpdate (passed as reference)
+     * @param Style $baseStyle
+     * @return void
+     */
+    private function mergeFontStyles($styleToUpdate, $baseStyle)
+    {
+        if (!$this->hasSetFontBold && $baseStyle->isFontBold()) {
+            $styleToUpdate->setFontBold();
+        }
+        if (!$this->hasSetFontItalic && $baseStyle->isFontItalic()) {
+            $styleToUpdate->setFontItalic();
+        }
+        if (!$this->hasSetFontUnderline && $baseStyle->isFontUnderline()) {
+            $styleToUpdate->setFontUnderline();
+        }
+        if (!$this->hasSetFontStrikethrough && $baseStyle->isFontStrikethrough()) {
+            $styleToUpdate->setFontStrikethrough();
+        }
+    }
+
+    /**
+     * @param Style $styleToUpdate Style to update (passed as reference)
+     * @param Style $baseStyle
+     * @return void
+     */
+    private function mergeOtherFontProperties($styleToUpdate, $baseStyle)
+    {
+        if (!$this->hasSetFontSize && $baseStyle->getFontSize() !== self::DEFAULT_FONT_SIZE) {
+            $styleToUpdate->setFontSize($baseStyle->getFontSize());
+        }
+        if (!$this->hasSetFontColor && $baseStyle->getFontColor() !== self::DEFAULT_FONT_COLOR) {
+            $styleToUpdate->setFontColor($baseStyle->getFontColor());
+        }
+        if (!$this->hasSetFontName && $baseStyle->getFontName() !== self::DEFAULT_FONT_NAME) {
+            $styleToUpdate->setFontName($baseStyle->getFontName());
+        }
+    }
+
+    /**
+     * @param Style $styleToUpdate Style to update (passed as reference)
+     * @param Style $baseStyle
+     * @return void
+     */
+    private function mergeCellProperties($styleToUpdate, $baseStyle)
+    {
+        if (!$this->hasSetWrapText && $baseStyle->shouldWrapText()) {
+            $styleToUpdate->setShouldWrapText();
+        }
+        if (!$this->getBorder() && $baseStyle->shouldApplyBorder()) {
+            $styleToUpdate->setBorder($baseStyle->getBorder());
+        }
+        if (!$this->hasSetBackgroundColor && $baseStyle->shouldApplyBackgroundColor()) {
+            $styleToUpdate->setBackgroundColor($baseStyle->getBackgroundColor());
+        }
+    }
 }

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

@@ -149,7 +149,8 @@ EOD;
     private function isEmptyRow($dataRow)
     {
         $numCells = count($dataRow);
-        return ($numCells === 1 && CellHelper::isEmpty($dataRow[0]));
+        // using "reset()" instead of "$dataRow[0]" because $dataRow can be an associative array
+        return ($numCells === 1 && CellHelper::isEmpty(reset($dataRow)));
     }
 
     /**

tests/Spout/Common/Escaper/ODSTest.php

@@ -0,0 +1,42 @@
+<?php
+
+namespace Box\Spout\Common\Escaper;
+
+/**
+ * Class ODSTest
+ *
+ * @package Box\Spout\Common\Escaper
+ */
+class ODSTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @return array
+     */
+    public function dataProviderForTestEscape()
+    {
+        return [
+            ['test', 'test'],
+            ['carl\'s "pokemon"', 'carl&#039;s &quot;pokemon&quot;'],
+            ["\n", "\n"],
+            ["\r", "\r"],
+            ["\t", "\t"],
+            ["\v", "�"],
+            ["\f", "�"],
+        ];
+    }
+
+    /**
+     * @dataProvider dataProviderForTestEscape
+     *
+     * @param string $stringToEscape
+     * @param string $expectedEscapedString
+     * @return void
+     */
+    public function testEscape($stringToEscape, $expectedEscapedString)
+    {
+        $escaper = \Box\Spout\Common\Escaper\ODS::getInstance();
+        $escapedString = $escaper->escape($stringToEscape);
+
+        $this->assertEquals($expectedEscapedString, $escapedString, 'Incorrect escaped string');
+    }
+}

tests/Spout/Writer/XLSX/WriterTest.php

@@ -294,7 +294,7 @@ class WriterTest extends \PHPUnit_Framework_TestCase
         $dataRows = [
             [''],
             ['xlsx--21', 'xlsx--22'],
-            [''],
+            ['key' => ''],
             [''],
             ['xlsx--51', 'xlsx--52'],
         ];