Fix sheet name uniqueness bug when creating multiple workbooks.

This commit is contained in:
Bob Wienholt 2017-02-06 13:09:19 -05:00
parent 6f4ddb1569
commit c7e5305014
4 changed files with 53 additions and 19 deletions

View File

@ -3,6 +3,7 @@
namespace Box\Spout\Writer\Common; namespace Box\Spout\Writer\Common;
use Box\Spout\Common\Helper\StringHelper; use Box\Spout\Common\Helper\StringHelper;
use Box\Spout\Writer\Common\Internal\WorkbookInterface;
use Box\Spout\Writer\Exception\InvalidSheetNameException; use Box\Spout\Writer\Exception\InvalidSheetNameException;
/** /**
@ -21,8 +22,8 @@ class Sheet
/** @var array Invalid characters that cannot be contained in the sheet name */ /** @var array Invalid characters that cannot be contained in the sheet name */
private static $INVALID_CHARACTERS_IN_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 */ /** @var \Box\Spout\Writer\Common\Internal\WorkbookInterface reference to Workbook this sheet is a part of */
protected static $SHEETS_NAME_USED = []; protected $workbook;
/** @var int Index of the sheet, based on order in the workbook (zero-based) */ /** @var int Index of the sheet, based on order in the workbook (zero-based) */
protected $index; protected $index;
@ -36,9 +37,10 @@ class Sheet
/** /**
* @param int $sheetIndex Index of the sheet, based on order in the workbook (zero-based) * @param int $sheetIndex Index of the sheet, based on order in the workbook (zero-based)
*/ */
public function __construct($sheetIndex) public function __construct($sheetIndex, WorkbookInterface $wb)
{ {
$this->index = $sheetIndex; $this->index = $sheetIndex;
$this->workbook = $wb;
$this->stringHelper = new StringHelper(); $this->stringHelper = new StringHelper();
$this->setName(self::DEFAULT_SHEET_NAME_PREFIX . ($sheetIndex + 1)); $this->setName(self::DEFAULT_SHEET_NAME_PREFIX . ($sheetIndex + 1));
} }
@ -71,14 +73,13 @@ class Sheet
* @api * @api
* @param string $name Name of the sheet * @param string $name Name of the sheet
* @return Sheet * @return Sheet
* @throws \Box\Spout\Writer\Exception\InvalidSheetNameException If the sheet's name is invalid. * @throws InvalidSheetNameException If the sheet's name is invalid.
*/ */
public function setName($name) public function setName($name)
{ {
$this->throwIfNameIsInvalid($name); $this->throwIfNameIsInvalid($name);
$this->name = $name; $this->name = $name;
self::$SHEETS_NAME_USED[$this->index] = $name;
return $this; return $this;
} }
@ -89,7 +90,7 @@ class Sheet
* *
* @param string $name * @param string $name
* @return void * @return void
* @throws \Box\Spout\Writer\Exception\InvalidSheetNameException If the sheet's name is invalid. * @throws InvalidSheetNameException If the sheet's name is invalid.
*/ */
protected function throwIfNameIsInvalid($name) protected function throwIfNameIsInvalid($name)
{ {
@ -163,8 +164,8 @@ class Sheet
*/ */
protected function isNameUnique($name) protected function isNameUnique($name)
{ {
foreach (self::$SHEETS_NAME_USED as $sheetIndex => $sheetName) { foreach ($this->workbook->getWorksheets() as $sheetIndex => $sheet) {
if ($sheetIndex !== $this->index && $sheetName === $name) { if ($sheetIndex !== $this->index && $sheet->getName() === $name) {
return false; return false;
} }
} }

View File

@ -69,7 +69,7 @@ class Workbook extends AbstractWorkbook
public function addNewSheet() public function addNewSheet()
{ {
$newSheetIndex = count($this->worksheets); $newSheetIndex = count($this->worksheets);
$sheet = new Sheet($newSheetIndex); $sheet = new Sheet($newSheetIndex, $this);
$sheetsContentTempFolder = $this->fileSystemHelper->getSheetsContentTempFolder(); $sheetsContentTempFolder = $this->fileSystemHelper->getSheetsContentTempFolder();
$worksheet = new Worksheet($sheet, $sheetsContentTempFolder); $worksheet = new Worksheet($sheet, $sheetsContentTempFolder);

View File

@ -83,7 +83,7 @@ class Workbook extends AbstractWorkbook
public function addNewSheet() public function addNewSheet()
{ {
$newSheetIndex = count($this->worksheets); $newSheetIndex = count($this->worksheets);
$sheet = new Sheet($newSheetIndex); $sheet = new Sheet($newSheetIndex, $this);
$worksheetFilesFolder = $this->fileSystemHelper->getXlWorksheetsFolder(); $worksheetFilesFolder = $this->fileSystemHelper->getXlWorksheetsFolder();
$worksheet = new Worksheet($sheet, $worksheetFilesFolder, $this->sharedStringsHelper, $this->styleHelper, $this->shouldUseInlineStrings); $worksheet = new Worksheet($sheet, $worksheetFilesFolder, $this->sharedStringsHelper, $this->styleHelper, $this->shouldUseInlineStrings);

View File

@ -14,7 +14,9 @@ class SheetTest extends \PHPUnit_Framework_TestCase
*/ */
public function testGetSheetName() public function testGetSheetName()
{ {
$sheets = [new Sheet(0), new Sheet(1)]; $workbook = new SheetTestWorkbook();
$sheets = [$workbook->addNewSheet(), $workbook->addNewSheet()];
$this->assertEquals('Sheet1', $sheets[0]->getName(), 'Invalid name for the first sheet'); $this->assertEquals('Sheet1', $sheets[0]->getName(), 'Invalid name for the first sheet');
$this->assertEquals('Sheet2', $sheets[1]->getName(), 'Invalid name for the second sheet'); $this->assertEquals('Sheet2', $sheets[1]->getName(), 'Invalid name for the second sheet');
@ -25,8 +27,10 @@ class SheetTest extends \PHPUnit_Framework_TestCase
*/ */
public function testSetSheetNameShouldCreateSheetWithCustomName() public function testSetSheetNameShouldCreateSheetWithCustomName()
{ {
$workbook = new SheetTestWorkbook();
$customSheetName = 'CustomName'; $customSheetName = 'CustomName';
$sheet = new Sheet(0); $sheet = $workbook->addNewSheet();
$sheet->setName($customSheetName); $sheet->setName($customSheetName);
$this->assertEquals($customSheetName, $sheet->getName(), "The sheet name should have been changed to '$customSheetName'"); $this->assertEquals($customSheetName, $sheet->getName(), "The sheet name should have been changed to '$customSheetName'");
@ -63,7 +67,8 @@ class SheetTest extends \PHPUnit_Framework_TestCase
*/ */
public function testSetSheetNameShouldThrowOnInvalidName($customSheetName) public function testSetSheetNameShouldThrowOnInvalidName($customSheetName)
{ {
(new Sheet(0))->setName($customSheetName); $workbook = new SheetTestWorkbook();
$workbook->addNewSheet()->setName($customSheetName);
} }
/** /**
@ -71,8 +76,9 @@ class SheetTest extends \PHPUnit_Framework_TestCase
*/ */
public function testSetSheetNameShouldNotThrowWhenSettingSameNameAsCurrentOne() public function testSetSheetNameShouldNotThrowWhenSettingSameNameAsCurrentOne()
{ {
$workbook = new SheetTestWorkbook();
$customSheetName = 'Sheet name'; $customSheetName = 'Sheet name';
$sheet = new Sheet(0); $sheet = $workbook->addNewSheet();
$sheet->setName($customSheetName); $sheet->setName($customSheetName);
$sheet->setName($customSheetName); $sheet->setName($customSheetName);
} }
@ -83,12 +89,39 @@ class SheetTest extends \PHPUnit_Framework_TestCase
*/ */
public function testSetSheetNameShouldThrowWhenNameIsAlreadyUsed() public function testSetSheetNameShouldThrowWhenNameIsAlreadyUsed()
{ {
$workbook = new SheetTestWorkbook();
$customSheetName = 'Sheet name'; $customSheetName = 'Sheet name';
$sheet = new Sheet(0); $sheet = $workbook->addNewSheet();
$sheet->setName($customSheetName); $sheet->setName($customSheetName);
$sheet = new Sheet(1); $sheet = $workbook->addNewSheet();
$sheet->setName($customSheetName); $sheet->setName($customSheetName);
} }
} }
class SheetTestWorkbook extends \Box\Spout\Writer\Common\Internal\AbstractWorkbook
{
protected function getMaxRowsPerWorksheet()
{
return 0;
}
protected function getStyleHelper()
{
return null;
}
public function addNewSheet()
{
$newSheetIndex = count($this->worksheets);
$sheet = new Sheet($newSheetIndex, $this);
$this->worksheets[] = $sheet;
return $sheet;
}
public function close($finalFilePointer)
{
}
}