From c7e5305014afab687290065fac3c14967a0119a4 Mon Sep 17 00:00:00 2001 From: Bob Wienholt Date: Mon, 6 Feb 2017 13:09:19 -0500 Subject: [PATCH 1/2] Fix sheet name uniqueness bug when creating multiple workbooks. --- src/Spout/Writer/Common/Sheet.php | 23 ++++++----- src/Spout/Writer/ODS/Internal/Workbook.php | 2 +- src/Spout/Writer/XLSX/Internal/Workbook.php | 2 +- tests/Spout/Writer/Common/SheetTest.php | 45 ++++++++++++++++++--- 4 files changed, 53 insertions(+), 19 deletions(-) diff --git a/src/Spout/Writer/Common/Sheet.php b/src/Spout/Writer/Common/Sheet.php index 2c0d1f2..d7a9037 100644 --- a/src/Spout/Writer/Common/Sheet.php +++ b/src/Spout/Writer/Common/Sheet.php @@ -3,6 +3,7 @@ namespace Box\Spout\Writer\Common; use Box\Spout\Common\Helper\StringHelper; +use Box\Spout\Writer\Common\Internal\WorkbookInterface; use Box\Spout\Writer\Exception\InvalidSheetNameException; /** @@ -21,8 +22,8 @@ class Sheet /** @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 \Box\Spout\Writer\Common\Internal\WorkbookInterface reference to Workbook this sheet is a part of */ + protected $workbook; /** @var int Index of the sheet, based on order in the workbook (zero-based) */ protected $index; @@ -36,9 +37,10 @@ class Sheet /** * @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->workbook = $wb; $this->stringHelper = new StringHelper(); $this->setName(self::DEFAULT_SHEET_NAME_PREFIX . ($sheetIndex + 1)); } @@ -71,14 +73,13 @@ class Sheet * @api * @param string $name Name of the 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) { $this->throwIfNameIsInvalid($name); $this->name = $name; - self::$SHEETS_NAME_USED[$this->index] = $name; return $this; } @@ -89,7 +90,7 @@ class Sheet * * @param string $name * @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) { @@ -163,11 +164,11 @@ class Sheet */ protected function isNameUnique($name) { - foreach (self::$SHEETS_NAME_USED as $sheetIndex => $sheetName) { - if ($sheetIndex !== $this->index && $sheetName === $name) { - return false; - } - } + foreach ($this->workbook->getWorksheets() as $sheetIndex => $sheet) { + if ($sheetIndex !== $this->index && $sheet->getName() === $name) { + return false; + } + } return true; } diff --git a/src/Spout/Writer/ODS/Internal/Workbook.php b/src/Spout/Writer/ODS/Internal/Workbook.php index ce24f2f..880f276 100644 --- a/src/Spout/Writer/ODS/Internal/Workbook.php +++ b/src/Spout/Writer/ODS/Internal/Workbook.php @@ -69,7 +69,7 @@ class Workbook extends AbstractWorkbook public function addNewSheet() { $newSheetIndex = count($this->worksheets); - $sheet = new Sheet($newSheetIndex); + $sheet = new Sheet($newSheetIndex, $this); $sheetsContentTempFolder = $this->fileSystemHelper->getSheetsContentTempFolder(); $worksheet = new Worksheet($sheet, $sheetsContentTempFolder); diff --git a/src/Spout/Writer/XLSX/Internal/Workbook.php b/src/Spout/Writer/XLSX/Internal/Workbook.php index bdf027f..5eba944 100644 --- a/src/Spout/Writer/XLSX/Internal/Workbook.php +++ b/src/Spout/Writer/XLSX/Internal/Workbook.php @@ -83,7 +83,7 @@ class Workbook extends AbstractWorkbook public function addNewSheet() { $newSheetIndex = count($this->worksheets); - $sheet = new Sheet($newSheetIndex); + $sheet = new Sheet($newSheetIndex, $this); $worksheetFilesFolder = $this->fileSystemHelper->getXlWorksheetsFolder(); $worksheet = new Worksheet($sheet, $worksheetFilesFolder, $this->sharedStringsHelper, $this->styleHelper, $this->shouldUseInlineStrings); diff --git a/tests/Spout/Writer/Common/SheetTest.php b/tests/Spout/Writer/Common/SheetTest.php index 6bee6c6..b38e501 100644 --- a/tests/Spout/Writer/Common/SheetTest.php +++ b/tests/Spout/Writer/Common/SheetTest.php @@ -14,7 +14,9 @@ class SheetTest extends \PHPUnit_Framework_TestCase */ 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('Sheet2', $sheets[1]->getName(), 'Invalid name for the second sheet'); @@ -25,8 +27,10 @@ class SheetTest extends \PHPUnit_Framework_TestCase */ public function testSetSheetNameShouldCreateSheetWithCustomName() { + $workbook = new SheetTestWorkbook(); + $customSheetName = 'CustomName'; - $sheet = new Sheet(0); + $sheet = $workbook->addNewSheet(); $sheet->setName($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) { - (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() { + $workbook = new SheetTestWorkbook(); $customSheetName = 'Sheet name'; - $sheet = new Sheet(0); + $sheet = $workbook->addNewSheet(); $sheet->setName($customSheetName); $sheet->setName($customSheetName); } @@ -83,12 +89,39 @@ class SheetTest extends \PHPUnit_Framework_TestCase */ public function testSetSheetNameShouldThrowWhenNameIsAlreadyUsed() { + $workbook = new SheetTestWorkbook(); + $customSheetName = 'Sheet name'; - $sheet = new Sheet(0); + $sheet = $workbook->addNewSheet(); $sheet->setName($customSheetName); - $sheet = new Sheet(1); + $sheet = $workbook->addNewSheet(); $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) + { + } +} From cc8971875b37bc2392807275dbe56be211142810 Mon Sep 17 00:00:00 2001 From: Bob Wienholt Date: Mon, 6 Feb 2017 14:36:46 -0500 Subject: [PATCH 2/2] Use the "external" sheet when checking the name. Also, fix the phpUnit tests. --- src/Spout/Writer/Common/Sheet.php | 3 +- tests/Spout/Writer/Common/SheetTest.php | 46 +++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/Spout/Writer/Common/Sheet.php b/src/Spout/Writer/Common/Sheet.php index d7a9037..66e4d57 100644 --- a/src/Spout/Writer/Common/Sheet.php +++ b/src/Spout/Writer/Common/Sheet.php @@ -164,7 +164,8 @@ class Sheet */ protected function isNameUnique($name) { - foreach ($this->workbook->getWorksheets() as $sheetIndex => $sheet) { + foreach ($this->workbook->getWorksheets() as $sheetIndex => $worksheet) { + $sheet = $worksheet->getExternalSheet(); if ($sheetIndex !== $this->index && $sheet->getName() === $name) { return false; } diff --git a/tests/Spout/Writer/Common/SheetTest.php b/tests/Spout/Writer/Common/SheetTest.php index b38e501..d91dc06 100644 --- a/tests/Spout/Writer/Common/SheetTest.php +++ b/tests/Spout/Writer/Common/SheetTest.php @@ -2,12 +2,16 @@ namespace Box\Spout\Writer\Common; +use Box\Spout\Writer\Common\Internal\AbstractWorkbook; +use Box\Spout\Writer\Common\Internal\WorksheetInterface; +use PHPUnit_Framework_TestCase; + /** * Class SheetTest * * @package Box\Spout\Writer\Common */ -class SheetTest extends \PHPUnit_Framework_TestCase +class SheetTest extends PHPUnit_Framework_TestCase { /** * @return void @@ -101,8 +105,13 @@ class SheetTest extends \PHPUnit_Framework_TestCase } } -class SheetTestWorkbook extends \Box\Spout\Writer\Common\Internal\AbstractWorkbook +class SheetTestWorkbook extends AbstractWorkbook { + public function __construct() + { + parent::__construct(false, null); + } + protected function getMaxRowsPerWorksheet() { return 0; @@ -117,7 +126,7 @@ class SheetTestWorkbook extends \Box\Spout\Writer\Common\Internal\AbstractWorkbo { $newSheetIndex = count($this->worksheets); $sheet = new Sheet($newSheetIndex, $this); - $this->worksheets[] = $sheet; + $this->worksheets[] = new SheetTestWorksheet($sheet); return $sheet; } @@ -125,3 +134,34 @@ class SheetTestWorkbook extends \Box\Spout\Writer\Common\Internal\AbstractWorkbo { } } + +class SheetTestWorksheet implements WorksheetInterface +{ + private $sheet; + + public function __construct(Sheet $sheet) + { + $this->sheet = $sheet; + } + + public function addRow($dataRow, $style) + { + + } + + public function close() + { + + } + + public function getExternalSheet() + { + return $this->sheet; + } + + public function getLastWrittenRowIndex() + { + + } + +}