Merge pull request #91 from box/invalid_sheet_name_detection

Detection of invalid sheet name

README.md

@@ -263,6 +263,15 @@ $sheet = $writer->getCurrentSheet();
 $sheetName = $sheet->setName('My custom name');
 ``` 
 
+> Please note that Excel has some restrictions on the sheet's name:
+> * it should not exceed 31 characters
+> * it should not contain these characters: \ / ? * [ or ]
+> * it should not be blank
+> * it should be unique
+>
+> Handling these restrictions is the developer's responsibility. Spout does not try to automatically change the sheet's name, as one may rely on this name to be exactly what was passed in.
+
+
 ### Fluent interface
 
 Because fluent interfaces are great, you can use them with Spout:

src/Spout/Writer/Exception/InvalidSheetNameException.php

@@ -0,0 +1,12 @@
+<?php
+
+namespace Box\Spout\Writer\Exception;
+
+/**
+ * Class InvalidSheetNameException
+ *
+ * @package Box\Spout\Writer\Exception
+ */
+class InvalidSheetNameException extends WriterException
+{
+}

src/Spout/Writer/XLSX/Sheet.php

@@ -2,9 +2,11 @@
 
 namespace Box\Spout\Writer\XLSX;
 
+use Box\Spout\Writer\Exception\InvalidSheetNameException;
+
 /**
  * Class Sheet
- * Represents a worksheet within a XLSX file
+ * External representation of a worksheet within a XLSX file
  *
  * @package Box\Spout\Writer\XLSX
  */
@@ -12,6 +14,15 @@ class Sheet
 {
     const DEFAULT_SHEET_NAME_PREFIX = 'Sheet';
 
+    /** Sheet name should not exceed 31 characters */
+    const MAX_LENGTH_SHEET_NAME = 31;
+
+    /** @var array Invalid characters that cannot be contained in the sheet name */
+    private static $INVALID_CHARACTERS_IN_SHEET_NAME = ['\\', '/', '?', '*', '[', ']'];
+
+    /** @var array Associative array [SHEET_INDEX] => [SHEET_NAME] keeping track of sheets' name to enforce uniqueness */
+    protected static $SHEETS_NAME_USED = [];
+
     /** @var int Index of the sheet, based on order of creation (zero-based) */
     protected $index;
 
@@ -24,7 +35,7 @@ class Sheet
     public function __construct($sheetIndex)
     {
         $this->index = $sheetIndex;
-        $this->name = self::DEFAULT_SHEET_NAME_PREFIX . ($sheetIndex + 1);
+        $this->setName(self::DEFAULT_SHEET_NAME_PREFIX . ($sheetIndex + 1));
     }
 
     /**
@@ -44,12 +55,116 @@ class Sheet
     }
 
     /**
+     * Sets the name of the sheet. Note that Excel has some restrictions on the name:
+     *  - it should not be blank
+     *  - it should not exceed 31 characters
+     *  - it should not contain these characters: \ / ? * [ or ]
+     *  - it should be unique
+     *
      * @param string $name Name of the sheet
      * @return Sheet
+     * @throws \Box\Spout\Writer\Exception\InvalidSheetNameException If the sheet's name is invalid.
      */
     public function setName($name)
     {
+        if (!$this->isNameValid($name)) {
+            $errorMessage = "The sheet's name is invalid. It did not meet at least one of these requirements:\n";
+            $errorMessage .= " - It should not be blank\n";
+            $errorMessage .= " - It should not exceed 31 characters\n";
+            $errorMessage .= " - It should not contain these characters: \\ / ? * [ or ]\n";
+            $errorMessage .= " - It should be unique";
+            throw new InvalidSheetNameException($errorMessage);
+        }
+
         $this->name = $name;
+        self::$SHEETS_NAME_USED[$this->index] = $name;
+
         return $this;
     }
+
+    /**
+     * Returns whether the given sheet's name is valid.
+     * @see Sheet::setName for validity rules.
+     *
+     * @param string $name
+     * @return bool TRUE if the name is valid, FALSE otherwise.
+     */
+    protected function isNameValid($name)
+    {
+        if (!is_string($name)) {
+            return false;
+        }
+
+        $nameLength = $this->getStringLength($name);
+        $hasValidLength = ($nameLength > 0 && $nameLength <= self::MAX_LENGTH_SHEET_NAME);
+        $containsInvalidCharacters = $this->doesContainInvalidCharacters($name);
+        $isNameUnique = $this->isNameUnique($name);
+
+        return ($hasValidLength && !$containsInvalidCharacters && $isNameUnique);
+    }
+
+    /**
+     * Returns the length of the given string.
+     * It uses the multi-bytes function is available.
+     * @see strlen
+     * @see mb_strlen
+     *
+     * @param string $string
+     * @return int
+     */
+    protected function getStringLength($string)
+    {
+        return extension_loaded('mbstring') ? mb_strlen($string) : strlen($string);
+    }
+
+    /**
+     * Returns the position of the given character/substring in the given string.
+     * It uses the multi-bytes function is available.
+     * @see strpos
+     * @see mb_strpos
+     *
+     * @param string $string Haystack
+     * @param string $char Needle
+     * @return int Index of the char in the string if found (started at 0) or -1 if not found
+     */
+    protected function getCharPosition($string, $char)
+    {
+        $position = extension_loaded('mbstring') ? mb_strpos($string, $char) : strpos($string, $char);
+        return ($position !== false) ? $position : -1;
+    }
+
+    /**
+     * Returns whether the given name contains at least one invalid character.
+     * @see Sheet::$INVALID_CHARACTERS_IN_SHEET_NAME for the full list.
+     *
+     * @param string $name
+     * @return bool TRUE if the name contains invalid characters, FALSE otherwise.
+     */
+    protected function doesContainInvalidCharacters($name)
+    {
+        foreach (self::$INVALID_CHARACTERS_IN_SHEET_NAME as $invalidCharacter) {
+            if ($this->getCharPosition($name, $invalidCharacter) !== -1) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * Returns whether the given name is unique.
+     *
+     * @param string $name
+     * @return bool TRUE if the name is unique, FALSE otherwise.
+     */
+    protected function isNameUnique($name)
+    {
+        foreach (self::$SHEETS_NAME_USED as $sheetIndex => $sheetName) {
+            if ($sheetIndex !== $this->index && $sheetName === $name) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }

tests/Spout/Writer/XLSX/SheetTest.php

@@ -20,7 +20,7 @@ class SheetTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetSheetIndex()
     {
-        $sheets = $this->writeDataAndReturnSheets('test_get_sheet_index.xlsx');
+        $sheets = $this->writeDataToMulitpleSheetsAndReturnSheets('test_get_sheet_index.xlsx');
 
         $this->assertEquals(2, count($sheets), '2 sheets should have been created');
         $this->assertEquals(0, $sheets[0]->getIndex(), 'The first sheet should be index 0');
@@ -32,7 +32,7 @@ class SheetTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetSheetName()
     {
-        $sheets = $this->writeDataAndReturnSheets('test_get_sheet_name.xlsx');
+        $sheets = $this->writeDataToMulitpleSheetsAndReturnSheets('test_get_sheet_name.xlsx');
 
         $this->assertEquals(2, count($sheets), '2 sheets should have been created');
         $this->assertEquals('Sheet1', $sheets[0]->getName(), 'Invalid name for the first sheet');
@@ -45,27 +45,115 @@ class SheetTest extends \PHPUnit_Framework_TestCase
     public function testSetSheetNameShouldCreateSheetWithCustomName()
     {
         $fileName = 'test_set_name_should_create_sheet_with_custom_name.xlsx';
+        $customSheetName = 'CustomName';
+        $this->writeDataAndReturnSheetWithCustomName($fileName, $customSheetName);
+
+        $this->assertSheetNameEquals($customSheetName, $fileName, "The sheet name should have been changed to '$customSheetName'");
+    }
+
+    /**
+     * @return array
+     */
+    public function dataProviderForInvalidSheetNames()
+    {
+        return [
+            [null],
+            [21],
+            [''],
+            ['this title exceeds the 31 characters limit'],
+            ['Illegal \\'],
+            ['Illegal /'],
+            ['Illegal ?'],
+            ['Illegal *'],
+            ['Illegal ['],
+            ['Illegal ]'],
+        ];
+    }
+
+    /**
+     * @dataProvider dataProviderForInvalidSheetNames
+     * @expectedException \Box\Spout\Writer\Exception\InvalidSheetNameException
+     *
+     * @param string $customSheetName
+     * @return void
+     */
+    public function testSetSheetNameShouldThrowOnInvalidName($customSheetName)
+    {
+        $fileName = 'test_set_name_with_invalid_name_should_throw_exception.xlsx';
+        $this->writeDataAndReturnSheetWithCustomName($fileName, $customSheetName);
+    }
+
+    /**
+     * @return void
+     */
+    public function testSetSheetNameShouldNotThrowWhenSettingSameNameAsCurrentOne()
+    {
+        $fileName = 'test_set_name_with_same_as_current.xlsx';
         $this->createGeneratedFolderIfNeeded($fileName);
         $resourcePath = $this->getGeneratedResourcePath($fileName);
 
         $writer = WriterFactory::create(Type::XLSX);
         $writer->openToFile($resourcePath);
 
-        $customSheetName = 'CustomName';
+        $customSheetName = 'Sheet name';
         $sheet = $writer->getCurrentSheet();
         $sheet->setName($customSheetName);
+        $sheet->setName($customSheetName);
 
         $writer->addRow(['xlsx--11', 'xlsx--12']);
         $writer->close();
 
-        $this->assertSheetNameEquals($customSheetName, $resourcePath, "The sheet name should have been changed to '$customSheetName'");
+        $this->assertSheetNameEquals($customSheetName, $fileName, "The sheet name should have been changed to '$customSheetName'");
+    }
+
+    /**
+     * @expectedException \Box\Spout\Writer\Exception\InvalidSheetNameException
+     * @return void
+     */
+    public function testSetSheetNameShouldThrowWhenNameIsAlreadyUsed()
+    {
+        $fileName = 'test_set_name_with_non_unique_name.xlsx';
+        $this->createGeneratedFolderIfNeeded($fileName);
+        $resourcePath = $this->getGeneratedResourcePath($fileName);
+
+        $writer = WriterFactory::create(Type::XLSX);
+        $writer->openToFile($resourcePath);
+
+        $customSheetName = 'Sheet name';
+
+        $sheet = $writer->getCurrentSheet();
+        $sheet->setName($customSheetName);
+
+        $writer->addNewSheetAndMakeItCurrent();
+        $sheet = $writer->getCurrentSheet();
+        $sheet->setName($customSheetName);
+    }
+
+    /**
+     * @param string $fileName
+     * @param string $sheetName
+     * @return Sheet
+     */
+    private function writeDataAndReturnSheetWithCustomName($fileName, $sheetName)
+    {
+        $this->createGeneratedFolderIfNeeded($fileName);
+        $resourcePath = $this->getGeneratedResourcePath($fileName);
+
+        $writer = WriterFactory::create(Type::XLSX);
+        $writer->openToFile($resourcePath);
+
+        $sheet = $writer->getCurrentSheet();
+        $sheet->setName($sheetName);
+
+        $writer->addRow(['xlsx--11', 'xlsx--12']);
+        $writer->close();
     }
 
     /**
      * @param string $fileName
      * @return Sheet[]
      */
-    private function writeDataAndReturnSheets($fileName)
+    private function writeDataToMulitpleSheetsAndReturnSheets($fileName)
     {
         $this->createGeneratedFolderIfNeeded($fileName);
         $resourcePath = $this->getGeneratedResourcePath($fileName);
@@ -85,12 +173,13 @@ class SheetTest extends \PHPUnit_Framework_TestCase
 
     /**
      * @param string $expectedName
-     * @param string $resourcePath
+     * @param string $fileName
      * @param string $message
      * @return void
      */
-    private function assertSheetNameEquals($expectedName, $resourcePath, $message = '')
+    private function assertSheetNameEquals($expectedName, $fileName, $message = '')
     {
+        $resourcePath = $this->getGeneratedResourcePath($fileName);
         $pathToWorkbookFile = $resourcePath . '#xl/workbook.xml';
         $xmlContents = file_get_contents('zip://' . $pathToWorkbookFile);