From 817e4f1f01dfe98ac6642ab329111bfe44a51e18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Istv=C3=A1n=20Ign=C3=A1cz?= Date: Fri, 14 Aug 2020 16:31:07 +0200 Subject: [PATCH 1/2] #729 - fix shrink to fit + add test shrinkToFit was not handled in StyleMerger so it was overwritten by the default cell style StyleManager didn't add the property to the xml if shrinkToFit was used without alignment or text wrap. Unit test --- src/Spout/Common/Entity/Style/Style.php | 14 ++++++++++++-- .../Common/Creator/Style/StyleBuilder.php | 3 ++- .../Common/Manager/Style/StyleMerger.php | 3 +++ .../Writer/XLSX/Manager/Style/StyleManager.php | 2 +- .../Spout/Writer/XLSX/WriterWithStyleTest.php | 18 ++++++++++++++++++ 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/Spout/Common/Entity/Style/Style.php b/src/Spout/Common/Entity/Style/Style.php index 7a49728..97688e9 100644 --- a/src/Spout/Common/Entity/Style/Style.php +++ b/src/Spout/Common/Entity/Style/Style.php @@ -66,8 +66,10 @@ class Style /** @var bool Whether the wrap text property was set */ private $hasSetWrapText = false; - private $shrinkToFit = false; + /** @var bool Whether the cell should shrink to fit to content */ private $shouldShrinkToFit = false; + /** @var bool Whether the shouldShrinkToFit text property was set */ + private $hasSetShrinkToFit = false; /** @var Border */ private $border; @@ -474,7 +476,7 @@ class Style */ public function setShouldShrinkToFit($shrinkToFit = true) { - $this->shrinkToFit = $shrinkToFit; + $this->hasSetShrinkToFit = true; $this->shouldShrinkToFit = $shrinkToFit; return $this; @@ -487,4 +489,12 @@ class Style { return $this->shouldShrinkToFit; } + + /** + * @return bool + */ + public function hasSetShrinkToFit() + { + return $this->hasSetShrinkToFit; + } } diff --git a/src/Spout/Writer/Common/Creator/Style/StyleBuilder.php b/src/Spout/Writer/Common/Creator/Style/StyleBuilder.php index 1341233..72fdcf0 100644 --- a/src/Spout/Writer/Common/Creator/Style/StyleBuilder.php +++ b/src/Spout/Writer/Common/Creator/Style/StyleBuilder.php @@ -186,7 +186,8 @@ class StyleBuilder /** * Set should shrink to fit * @param bool $shrinkToFit - * @return void + * @return StyleBuilder + * @api */ public function setShouldShrinkToFit($shrinkToFit = true) { diff --git a/src/Spout/Writer/Common/Manager/Style/StyleMerger.php b/src/Spout/Writer/Common/Manager/Style/StyleMerger.php index 806c8d5..1adeb18 100644 --- a/src/Spout/Writer/Common/Manager/Style/StyleMerger.php +++ b/src/Spout/Writer/Common/Manager/Style/StyleMerger.php @@ -85,6 +85,9 @@ class StyleMerger if (!$style->hasSetWrapText() && $baseStyle->shouldWrapText()) { $styleToUpdate->setShouldWrapText(); } + if (!$style->hasSetShrinkToFit() && $baseStyle->shouldShrinkToFit()) { + $styleToUpdate->setShouldShrinkToFit(); + } if (!$style->hasSetCellAlignment() && $baseStyle->shouldApplyCellAlignment()) { $styleToUpdate->setCellAlignment($baseStyle->getCellAlignment()); } diff --git a/src/Spout/Writer/XLSX/Manager/Style/StyleManager.php b/src/Spout/Writer/XLSX/Manager/Style/StyleManager.php index b79b1da..437b433 100644 --- a/src/Spout/Writer/XLSX/Manager/Style/StyleManager.php +++ b/src/Spout/Writer/XLSX/Manager/Style/StyleManager.php @@ -249,7 +249,7 @@ EOD; $content .= \sprintf(' applyBorder="%d"', $style->shouldApplyBorder() ? 1 : 0); - if ($style->shouldApplyCellAlignment() || $style->shouldWrapText()) { + if ($style->shouldApplyCellAlignment() || $style->shouldWrapText() || $style->shouldShrinkToFit()) { $content .= ' applyAlignment="1">'; $content .= 'shouldApplyCellAlignment()) { diff --git a/tests/Spout/Writer/XLSX/WriterWithStyleTest.php b/tests/Spout/Writer/XLSX/WriterWithStyleTest.php index da0aec2..761ca85 100644 --- a/tests/Spout/Writer/XLSX/WriterWithStyleTest.php +++ b/tests/Spout/Writer/XLSX/WriterWithStyleTest.php @@ -306,6 +306,24 @@ class WriterWithStyleTest extends TestCase $this->assertFirstChildHasAttributeEquals(CellAlignment::RIGHT, $xfElement, 'alignment', 'horizontal'); } + /** + * @return void + */ + public function testAddRowShouldApplyShrinkToFit() + { + $fileName = 'test_add_row_should_apply_shrink_to_fit.xlsx'; + + $shrinkToFitStyle = (new StyleBuilder())->setShouldShrinkToFit()->build(); + $dataRows = $this->createStyledRowsFromValues([['xlsx--11']], $shrinkToFitStyle); + + $this->writeToXLSXFile($dataRows, $fileName); + + $cellXfsDomElement = $this->getXmlSectionFromStylesXmlFile($fileName, 'cellXfs'); + $xfElement = $cellXfsDomElement->getElementsByTagName('xf')->item(1); + $this->assertEquals(1, $xfElement->getAttribute('applyAlignment')); + $this->assertFirstChildHasAttributeEquals("true", $xfElement, 'alignment', 'shrinkToFit'); + } + /** * @return void */ From 41a9ae89ee50ada9c8d31658f7cbb4f80c401472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Istv=C3=A1n=20Ign=C3=A1cz?= Date: Fri, 14 Aug 2020 18:24:26 +0200 Subject: [PATCH 2/2] #529 - tests for column width, row height and cell merge + minor refact Tests Added 'addOption' to OptionsManagerInterface Moved 'setColumnWidths' and 'mergeCells' methods to Xlsx Writer implementation since the actual feature only works for Xlsx at the moment --- .../Manager/OptionsManagerInterface.php | 9 ++ .../Writer/WriterMultiSheetsAbstract.php | 27 +--- src/Spout/Writer/XLSX/Writer.php | 30 ++++ .../Common/Manager/OptionsManagerTest.php | 24 +++ tests/Spout/Writer/XLSX/WriterTest.php | 138 ++++++++++++++++++ .../Spout/Writer/XLSX/WriterWithStyleTest.php | 2 +- 6 files changed, 204 insertions(+), 26 deletions(-) diff --git a/src/Spout/Common/Manager/OptionsManagerInterface.php b/src/Spout/Common/Manager/OptionsManagerInterface.php index 21fea0c..bccb1c3 100644 --- a/src/Spout/Common/Manager/OptionsManagerInterface.php +++ b/src/Spout/Common/Manager/OptionsManagerInterface.php @@ -19,4 +19,13 @@ interface OptionsManagerInterface * @return mixed|null The set option or NULL if no option with given name found */ public function getOption($optionName); + + /** + * Add an option to the internal list of options + * Used only for mergeCells() for now + * @param mixed $optionName + * @param mixed $optionValue + * @return void + */ + public function addOption($optionName, $optionValue); } diff --git a/src/Spout/Writer/WriterMultiSheetsAbstract.php b/src/Spout/Writer/WriterMultiSheetsAbstract.php index 57efae4..58f22b8 100644 --- a/src/Spout/Writer/WriterMultiSheetsAbstract.php +++ b/src/Spout/Writer/WriterMultiSheetsAbstract.php @@ -4,6 +4,7 @@ namespace Box\Spout\Writer; use Box\Spout\Common\Creator\HelperFactory; use Box\Spout\Common\Entity\Row; +use Box\Spout\Common\Exception\IOException; use Box\Spout\Common\Helper\GlobalFunctionsHelper; use Box\Spout\Common\Manager\OptionsManagerInterface; use Box\Spout\Writer\Common\Creator\ManagerFactoryInterface; @@ -61,31 +62,6 @@ abstract class WriterMultiSheetsAbstract extends WriterAbstract return $this; } - /** - * Set columns widths as list. If value is null will set column with default width (8.43) - * @param array $columnWidths - * @return WriterMultiSheetsAbstract - */ - public function setColumnWidths(array $columnWidths) - { - $this->optionsManager->setOption(Options::COLUMN_WIDTHS, $columnWidths); - - return $this; - } - - /** - * Undocumented function - * - * @param array $range - * @return void - */ - public function mergeCells(array $range1, array $range2) - { - $this->optionsManager->addOption(Options::MERGE_CELLS, [$range1, $range2]); - - return $this; - } - /** * {@inheritdoc} */ @@ -122,6 +98,7 @@ abstract class WriterMultiSheetsAbstract extends WriterAbstract * Creates a new sheet and make it the current sheet. The data will now be written to this sheet. * * @throws WriterNotOpenedException If the writer has not been opened yet + * @throws IOException If unable to open the sheet for writing * @return Sheet The created sheet */ public function addNewSheetAndMakeItCurrent() diff --git a/src/Spout/Writer/XLSX/Writer.php b/src/Spout/Writer/XLSX/Writer.php index 9d928fc..41eb03a 100644 --- a/src/Spout/Writer/XLSX/Writer.php +++ b/src/Spout/Writer/XLSX/Writer.php @@ -47,4 +47,34 @@ class Writer extends WriterMultiSheetsAbstract return $this; } + + /** + * Set columns widths as list. If value is null will set column with default width (8.43) + * @param array $columnWidths + * @return WriterMultiSheetsAbstract + */ + public function setColumnWidths(array $columnWidths) + { + $this->optionsManager->setOption(Options::COLUMN_WIDTHS, $columnWidths); + + return $this; + } + + /** + * Merge cells. + * Row coordinates are indexed from 1, columns from 0 (A = 0), + * so a merge B2:G2 looks like $writer->mergeCells([1,2], [6, 2]); + * + * You may use CellHelper::getColumnLettersFromColumnIndex() to convert from "B2" to "[1,2]" + * + * @param int[] $range1 - top left cell's coordinate [column, row] + * @param int[] $range2 - bottom right cell's coordinate [column, row] + * @return $this + */ + public function mergeCells(array $range1, array $range2) + { + $this->optionsManager->addOption(Options::MERGE_CELLS, [$range1, $range2]); + + return $this; + } } diff --git a/tests/Spout/Common/Manager/OptionsManagerTest.php b/tests/Spout/Common/Manager/OptionsManagerTest.php index e2f0c5c..66da195 100644 --- a/tests/Spout/Common/Manager/OptionsManagerTest.php +++ b/tests/Spout/Common/Manager/OptionsManagerTest.php @@ -73,4 +73,28 @@ class OptionsManagerTest extends TestCase $optionsManager->setOption('not-supported', 'something'); $this->assertNull($optionsManager->getOption('not-supported')); } + + /** + * @return void + */ + public function testOptionManagerShouldReturnArrayIfListOptionsAdded() + { + $optionsManager = $this->optionsManager; + $optionsManager->addOption('bar', 'something'); + $optionsManager->addOption('bar', 'something-else'); + $this->assertIsArray($optionsManager->getOption('bar')); + $this->assertCount(2, $optionsManager->getOption('bar')); + $this->assertEquals('something', $optionsManager->getOption('bar')[0]); + $this->assertEquals('something-else', $optionsManager->getOption('bar')[1]); + } + + /** + * @return void + */ + public function testOptionsManagerShouldReturnNullIfListOptionNotSupported() + { + $optionsManager = $this->optionsManager; + $optionsManager->addOption('not-supported', 'something'); + $this->assertNull($optionsManager->getOption('not-supported')); + } } diff --git a/tests/Spout/Writer/XLSX/WriterTest.php b/tests/Spout/Writer/XLSX/WriterTest.php index 15cc4e4..16b852c 100644 --- a/tests/Spout/Writer/XLSX/WriterTest.php +++ b/tests/Spout/Writer/XLSX/WriterTest.php @@ -7,6 +7,7 @@ use Box\Spout\Common\Entity\Row; use Box\Spout\Common\Exception\InvalidArgumentException; use Box\Spout\Common\Exception\IOException; use Box\Spout\Common\Exception\SpoutException; +use Box\Spout\Reader\Wrapper\XMLReader; use Box\Spout\TestUsingResource; use Box\Spout\Writer\Common\Creator\WriterEntityFactory; use Box\Spout\Writer\Exception\WriterAlreadyOpenedException; @@ -527,6 +528,105 @@ class WriterTest extends TestCase $this->assertInlineDataWasWrittenToSheet($fileName, 1, 'control _x0015_ character'); } + /** + * @return void + */ + public function testAddRowShouldSupportRowHeights() + { + $fileName = 'test_add_row_should_support_row_heights.xlsx'; + $dataRows = $this->createRowsFromValues([ + ['First row with default height'], + ['Second row with custom height'], + ]); + + $dataRows[1]->setHeight('23'); + + $this->writeToXLSXFile($dataRows, $fileName); + $firstRow = $this->getXmlRowFromXmlFile($fileName, 1, 1); + $secondRow = $this->getXmlRowFromXmlFile($fileName, 1, 2); + $this->assertEquals('15', $firstRow->getAttribute('ht'), '1st row does not have default height.'); + $this->assertEquals('23', $secondRow->getAttribute('ht'), '2nd row does not have custom height.'); + } + + /** + * @return void + */ + public function testAddRowShouldSupportColumnWidths() + { + $fileName = 'test_add_row_should_support_column_widths.xlsx'; + $this->createGeneratedFolderIfNeeded($fileName); + $resourcePath = $this->getGeneratedResourcePath($fileName); + $writer = WriterEntityFactory::createXLSXWriter(); + $writer->setShouldUseInlineStrings(true); + $writer->openToFile($resourcePath); + + $columnWidths = [1, 2, 3, 4]; + $writer->setColumnWidths($columnWidths); + $writer->addRows($this->createRowsFromValues([ + ['Test cell'], + ])); + $writer->close(); + + $xmlReader = $this->getXmlReaderForSheetFromXmlFile($fileName, 1); + $xmlReader->readUntilNodeFound('cols'); + $this->assertEquals('cols', $xmlReader->getCurrentNodeName(), 'Sheet does not have cols tag'); + $this->assertEquals(count($columnWidths), $xmlReader->expand()->childNodes->length, 'Sheet does not have the specified number of column definitions'); + foreach ($columnWidths as $index => $columnWidth) { + $xmlReader->readUntilNodeFound('col'); + $this->assertEquals($index + 1, $xmlReader->expand()->getAttribute('min')); + $this->assertEquals($index + 1, $xmlReader->expand()->getAttribute('max')); + $this->assertEquals($columnWidth, $xmlReader->expand()->getAttribute('width')); + } + } + + /** + * @return void + */ + public function testCloseShouldAddMergeCellTags() + { + $fileName = 'test_add_row_should_support_column_widths.xlsx'; + $this->createGeneratedFolderIfNeeded($fileName); + $resourcePath = $this->getGeneratedResourcePath($fileName); + $writer = WriterEntityFactory::createXLSXWriter(); + $writer->setShouldUseInlineStrings(true); + $writer->openToFile($resourcePath); + + $writer->mergeCells([0, 1], [3, 1]); + $writer->mergeCells([2, 3], [10, 3]); + $writer->close(); + + $xmlReader = $this->getXmlReaderForSheetFromXmlFile($fileName, 1); + $xmlReader->readUntilNodeFound('mergeCells'); + $this->assertEquals('mergeCells', $xmlReader->getCurrentNodeName(), 'Sheet does not have mergeCells tag'); + $this->assertEquals(2, $xmlReader->expand()->childNodes->length, 'Sheet does not have the specified number of mergeCell definitions'); + $xmlReader->readUntilNodeFound('mergeCell'); + $this->assertEquals('A1:D1', $xmlReader->expand()->getAttribute('ref'), 'Merge ref for first range is not valid.'); + $xmlReader->readUntilNodeFound('mergeCell'); + $this->assertEquals('C3:K3', $xmlReader->expand()->getAttribute('ref'), 'Merge ref for second range is not valid.'); + } + + /** + * @return void + */ + public function testGeneratedFileShouldBeValidForEmptySheets() + { + $fileName = 'test_empty_sheet.xlsx'; + $this->createGeneratedFolderIfNeeded($fileName); + $resourcePath = $this->getGeneratedResourcePath($fileName); + $writer = WriterEntityFactory::createXLSXWriter(); + $writer->openToFile($resourcePath); + + $writer->addNewSheetAndMakeItCurrent(); + $writer->close(); + + $xmlReader = $this->getXmlReaderForSheetFromXmlFile($fileName, 1); + $xmlReader->setParserProperty(XMLReader::VALIDATE, true); + $this->assertTrue($xmlReader->isValid(), 'worksheet xml is not valid'); + $xmlReader->setParserProperty(XMLReader::VALIDATE, false); + $xmlReader->readUntilNodeFound('sheetData'); + $this->assertEquals('sheetData', $xmlReader->getCurrentNodeName(), 'worksheet xml does not have sheetData'); + } + /** * @return void */ @@ -641,4 +741,42 @@ class WriterTest extends TestCase $this->assertContains($sharedString, $xmlContents, $message); } + + /** + * @param $fileName + * @param $sheetIndex - 1 based + * @return XMLReader + */ + private function getXmlReaderForSheetFromXmlFile($fileName, $sheetIndex) + { + $resourcePath = $this->getGeneratedResourcePath($fileName); + + $xmlReader = new XMLReader(); + $xmlReader->openFileInZip($resourcePath, 'xl/worksheets/sheet' . $sheetIndex . '.xml'); + + return $xmlReader; + } + + /** + * @param $fileName + * @param $sheetIndex - 1 based + * @param $rowIndex - 1 based + * @throws \Box\Spout\Reader\Exception\XMLProcessingException + * @return \DOMNode|null + */ + private function getXmlRowFromXmlFile($fileName, $sheetIndex, $rowIndex) + { + $xmlReader = $this->getXmlReaderForSheetFromXmlFile($fileName, $sheetIndex); + $xmlReader->readUntilNodeFound('sheetData'); + + for ($i = 0; $i < $rowIndex; $i++) { + $xmlReader->readUntilNodeFound('row'); + } + + $row = $xmlReader->expand(); + + $xmlReader->close(); + + return $row; + } } diff --git a/tests/Spout/Writer/XLSX/WriterWithStyleTest.php b/tests/Spout/Writer/XLSX/WriterWithStyleTest.php index 761ca85..3baa665 100644 --- a/tests/Spout/Writer/XLSX/WriterWithStyleTest.php +++ b/tests/Spout/Writer/XLSX/WriterWithStyleTest.php @@ -321,7 +321,7 @@ class WriterWithStyleTest extends TestCase $cellXfsDomElement = $this->getXmlSectionFromStylesXmlFile($fileName, 'cellXfs'); $xfElement = $cellXfsDomElement->getElementsByTagName('xf')->item(1); $this->assertEquals(1, $xfElement->getAttribute('applyAlignment')); - $this->assertFirstChildHasAttributeEquals("true", $xfElement, 'alignment', 'shrinkToFit'); + $this->assertFirstChildHasAttributeEquals('true', $xfElement, 'alignment', 'shrinkToFit'); } /**