From ddfa40e8b3c8991298a3e8bd6debc545a3a5e8af Mon Sep 17 00:00:00 2001 From: Adrien Loison Date: Fri, 10 Nov 2017 22:22:08 +0100 Subject: [PATCH] StyleMerger and RowManager changes Move DI of StyleMerger to get rid of business logic in the entities Cell and Row Simplify RowManager --- .../Writer/Common/Creator/EntityFactory.php | 12 +---- src/Spout/Writer/Common/Entity/Cell.php | 16 ------- src/Spout/Writer/Common/Entity/Row.php | 44 +----------------- .../Writer/Common/Manager/RowManager.php | 23 ---------- .../Writer/ODS/Creator/ManagerFactory.php | 7 +-- .../Writer/ODS/Manager/WorksheetManager.php | 13 +++++- .../Writer/XLSX/Creator/ManagerFactory.php | 26 ++++++++++- .../Writer/XLSX/Manager/WorksheetManager.php | 22 +++++++-- tests/Spout/Writer/Common/Entity/RowTest.php | 24 +++------- .../Writer/Common/Manager/RowManagerTest.php | 46 ------------------- 10 files changed, 67 insertions(+), 166 deletions(-) diff --git a/src/Spout/Writer/Common/Creator/EntityFactory.php b/src/Spout/Writer/Common/Creator/EntityFactory.php index ee31b89..54daa6c 100644 --- a/src/Spout/Writer/Common/Creator/EntityFactory.php +++ b/src/Spout/Writer/Common/Creator/EntityFactory.php @@ -5,8 +5,6 @@ namespace Box\Spout\Writer\Common\Creator; use Box\Spout\Writer\Common\Entity\Cell; use Box\Spout\Writer\Common\Entity\Row; use Box\Spout\Writer\Common\Entity\Style\Style; -use Box\Spout\Writer\Common\Manager\RowManager; -use Box\Spout\Writer\Common\Manager\Style\StyleMerger; use Box\Spout\Writer\WriterInterface; /** @@ -34,10 +32,7 @@ class EntityFactory */ public static function createRow(array $cells = [], Style $rowStyle = null) { - $styleMerger = new StyleMerger(); - $rowManager = new RowManager($styleMerger); - - return new Row($cells, $rowStyle, $rowManager); + return new Row($cells, $rowStyle); } /** @@ -47,14 +42,11 @@ class EntityFactory */ public static function createRowFromArray(array $cellValues = [], Style $rowStyle = null) { - $styleMerger = new StyleMerger(); - $rowManager = new RowManager($styleMerger); - $cells = array_map(function ($cellValue) { return new Cell($cellValue); }, $cellValues); - return new Row($cells, $rowStyle, $rowManager); + return new Row($cells, $rowStyle); } /** diff --git a/src/Spout/Writer/Common/Entity/Cell.php b/src/Spout/Writer/Common/Entity/Cell.php index 9660b42..db0bd11 100644 --- a/src/Spout/Writer/Common/Entity/Cell.php +++ b/src/Spout/Writer/Common/Entity/Cell.php @@ -198,20 +198,4 @@ class Cell { return (string) $this->value; } - - /** - * @param Style|null $style - * @return Cell - */ - public function applyStyle($style) - { - if ($style === null) { - return $this; - } - - $mergedStyle = $this->styleMerger->merge($this->style, $style); - $this->setStyle($mergedStyle); - - return $this; - } } diff --git a/src/Spout/Writer/Common/Entity/Row.php b/src/Spout/Writer/Common/Entity/Row.php index fae771f..821c42d 100644 --- a/src/Spout/Writer/Common/Entity/Row.php +++ b/src/Spout/Writer/Common/Entity/Row.php @@ -3,7 +3,6 @@ namespace Box\Spout\Writer\Common\Entity; use Box\Spout\Writer\Common\Entity\Style\Style; -use Box\Spout\Writer\Common\Manager\RowManager; class Row { @@ -19,25 +18,16 @@ class Row */ protected $style; - /** - * Thw row manager - * @var RowManager - */ - protected $rowManager; - /** * Row constructor. * @param Cell[] $cells * @param Style|null $style - * @param RowManager $rowManager */ - public function __construct(array $cells, $style, RowManager $rowManager) + public function __construct(array $cells, $style) { $this ->setCells($cells) ->setStyle($style); - - $this->rowManager = $rowManager; } /** @@ -81,17 +71,6 @@ class Row return $this; } - /** - * @param Style $style - * @return Row - */ - public function applyStyle($style) - { - $this->rowManager->applyStyle($this, $style); - - return $this; - } - /** * @param Cell $cell * @return Row @@ -102,25 +81,4 @@ class Row return $this; } - - /** - * Returns whether a row has cells - * - * @return bool - */ - public function hasCells() - { - return $this->rowManager->hasCells($this); - } - - /** - * Detect whether this row is considered empty. - * An empty row has either no cells at all - or only empty cells - * - * @return bool - */ - public function isEmpty() - { - return $this->rowManager->isEmpty($this); - } } diff --git a/src/Spout/Writer/Common/Manager/RowManager.php b/src/Spout/Writer/Common/Manager/RowManager.php index 109fa06..7534db5 100644 --- a/src/Spout/Writer/Common/Manager/RowManager.php +++ b/src/Spout/Writer/Common/Manager/RowManager.php @@ -3,7 +3,6 @@ namespace Box\Spout\Writer\Common\Manager; use Box\Spout\Writer\Common\Entity\Row; -use Box\Spout\Writer\Common\Entity\Style\Style; use Box\Spout\Writer\Common\Manager\Style\StyleMerger; class RowManager @@ -21,28 +20,6 @@ class RowManager $this->styleMerger = $styleMerger; } - /** - * @param Row $row - * @param Style $style - * @return $this - */ - public function applyStyle(Row $row, Style $style) - { - $mergedStyle = $this->styleMerger->merge($row->getStyle(), $style); - $row->setStyle($mergedStyle); - } - - /** - * Returns whether a row has cells - * - * @param Row $row - * @return bool - */ - public function hasCells(Row $row) - { - return count($row->getCells()) !== 0; - } - /** * Detect whether a row is considered empty. * An empty row has either no cells at all - or only one empty cell diff --git a/src/Spout/Writer/ODS/Creator/ManagerFactory.php b/src/Spout/Writer/ODS/Creator/ManagerFactory.php index eb4c7e1..f38c500 100644 --- a/src/Spout/Writer/ODS/Creator/ManagerFactory.php +++ b/src/Spout/Writer/ODS/Creator/ManagerFactory.php @@ -48,7 +48,7 @@ class ManagerFactory implements ManagerFactoryInterface $styleMerger = $this->createStyleMerger(); $styleManager = $this->createStyleManager($optionsManager); - $worksheetManager = $this->createWorksheetManager($styleManager); + $worksheetManager = $this->createWorksheetManager($styleManager, $styleMerger); return new WorkbookManager( $workbook, @@ -64,14 +64,15 @@ class ManagerFactory implements ManagerFactoryInterface /** * @param StyleManager $styleManager + * @param StyleMerger $styleMerger * @return WorksheetManager */ - private function createWorksheetManager(StyleManager $styleManager) + private function createWorksheetManager(StyleManager $styleManager, StyleMerger $styleMerger) { $stringsEscaper = $this->helperFactory->createStringsEscaper(); $stringsHelper = $this->helperFactory->createStringHelper(); - return new WorksheetManager($styleManager, $stringsEscaper, $stringsHelper); + return new WorksheetManager($styleManager, $styleMerger, $stringsEscaper, $stringsHelper); } /** diff --git a/src/Spout/Writer/ODS/Manager/WorksheetManager.php b/src/Spout/Writer/ODS/Manager/WorksheetManager.php index 4eb559e..1dfe580 100644 --- a/src/Spout/Writer/ODS/Manager/WorksheetManager.php +++ b/src/Spout/Writer/ODS/Manager/WorksheetManager.php @@ -10,6 +10,7 @@ use Box\Spout\Writer\Common\Entity\Cell; use Box\Spout\Writer\Common\Entity\Row; use Box\Spout\Writer\Common\Entity\Style\Style; use Box\Spout\Writer\Common\Entity\Worksheet; +use Box\Spout\Writer\Common\Manager\Style\StyleMerger; use Box\Spout\Writer\Common\Manager\WorksheetManagerInterface; use Box\Spout\Writer\ODS\Manager\Style\StyleManager; @@ -28,19 +29,25 @@ class WorksheetManager implements WorksheetManagerInterface /** @var StyleManager Manages styles */ private $styleManager; + /** @var StyleMerger Helper to merge styles together */ + private $styleMerger; + /** * WorksheetManager constructor. * * @param StyleManager $styleManager + * @param StyleMerger $styleMerger * @param ODSEscaper $stringsEscaper * @param StringHelper $stringHelper */ public function __construct( StyleManager $styleManager, + StyleMerger $styleMerger, ODSEscaper $stringsEscaper, StringHelper $stringHelper ) { $this->styleManager = $styleManager; + $this->styleMerger = $styleMerger; $this->stringsEscaper = $stringsEscaper; $this->stringHelper = $stringHelper; } @@ -151,9 +158,11 @@ class WorksheetManager implements WorksheetManagerInterface */ private function applyStyleAndGetCellXML(Cell $cell, Style $rowStyle, $currentCellIndex, $nextCellIndex) { - // Apply styles - the row style is merged at this point - $cell->applyStyle($rowStyle); + // Apply row and extra styles + $mergedCellAndRowStyle = $this->styleMerger->merge($cell->getStyle(), $rowStyle); + $cell->setStyle($mergedCellAndRowStyle); $newCellStyle = $this->styleManager->applyExtraStylesIfNeeded($cell); + $registeredStyle = $this->styleManager->registerStyle($newCellStyle); $styleIndex = $registeredStyle->getId() + 1; // 1-based diff --git a/src/Spout/Writer/XLSX/Creator/ManagerFactory.php b/src/Spout/Writer/XLSX/Creator/ManagerFactory.php index c8b3d60..9196825 100644 --- a/src/Spout/Writer/XLSX/Creator/ManagerFactory.php +++ b/src/Spout/Writer/XLSX/Creator/ManagerFactory.php @@ -6,6 +6,7 @@ use Box\Spout\Common\Manager\OptionsManagerInterface; use Box\Spout\Writer\Common\Creator\InternalEntityFactory; use Box\Spout\Writer\Common\Creator\ManagerFactoryInterface; use Box\Spout\Writer\Common\Entity\Options; +use Box\Spout\Writer\Common\Manager\RowManager; use Box\Spout\Writer\Common\Manager\SheetManager; use Box\Spout\Writer\Common\Manager\Style\StyleMerger; use Box\Spout\Writer\XLSX\Manager\SharedStringsManager; @@ -52,7 +53,7 @@ class ManagerFactory implements ManagerFactoryInterface $styleMerger = $this->createStyleMerger(); $styleManager = $this->createStyleManager($optionsManager); - $worksheetManager = $this->createWorksheetManager($optionsManager, $styleManager, $sharedStringsManager); + $worksheetManager = $this->createWorksheetManager($optionsManager, $styleManager, $styleMerger, $sharedStringsManager); return new WorkbookManager( $workbook, @@ -69,18 +70,30 @@ class ManagerFactory implements ManagerFactoryInterface /** * @param OptionsManagerInterface $optionsManager * @param StyleManager $styleManager + * @param StyleMerger $styleMerger * @param SharedStringsManager $sharedStringsManager * @return WorksheetManager */ private function createWorksheetManager( OptionsManagerInterface $optionsManager, StyleManager $styleManager, + StyleMerger $styleMerger, SharedStringsManager $sharedStringsManager ) { + $rowManager = $this->createRowManager($styleMerger); $stringsEscaper = $this->helperFactory->createStringsEscaper(); $stringsHelper = $this->helperFactory->createStringHelper(); - return new WorksheetManager($optionsManager, $styleManager, $sharedStringsManager, $stringsEscaper, $stringsHelper, $this->entityFactory); + return new WorksheetManager( + $optionsManager, + $rowManager, + $styleManager, + $styleMerger, + $sharedStringsManager, + $stringsEscaper, + $stringsHelper, + $this->entityFactory + ); } /** @@ -93,6 +106,15 @@ class ManagerFactory implements ManagerFactoryInterface return new SheetManager($stringHelper); } + /** + * @param StyleMerger $styleMerger + * @return RowManager + */ + public function createRowManager(StyleMerger $styleMerger) + { + return new RowManager($styleMerger); + } + /** * @param OptionsManagerInterface $optionsManager * @return StyleManager diff --git a/src/Spout/Writer/XLSX/Manager/WorksheetManager.php b/src/Spout/Writer/XLSX/Manager/WorksheetManager.php index 613eabd..2995fd5 100644 --- a/src/Spout/Writer/XLSX/Manager/WorksheetManager.php +++ b/src/Spout/Writer/XLSX/Manager/WorksheetManager.php @@ -14,6 +14,8 @@ use Box\Spout\Writer\Common\Entity\Row; use Box\Spout\Writer\Common\Entity\Style\Style; use Box\Spout\Writer\Common\Entity\Worksheet; use Box\Spout\Writer\Common\Helper\CellHelper; +use Box\Spout\Writer\Common\Manager\RowManager; +use Box\Spout\Writer\Common\Manager\Style\StyleMerger; use Box\Spout\Writer\Common\Manager\WorksheetManagerInterface; use Box\Spout\Writer\XLSX\Manager\Style\StyleManager; @@ -39,9 +41,15 @@ EOD; /** @var bool Whether inline or shared strings should be used */ protected $shouldUseInlineStrings; + /** @var RowManager Manages rows */ + private $rowManager; + /** @var StyleManager Manages styles */ private $styleManager; + /** @var StyleMerger Helper to merge styles together */ + private $styleMerger; + /** @var SharedStringsManager Helper to write shared strings */ private $sharedStringsManager; @@ -58,7 +66,9 @@ EOD; * WorksheetManager constructor. * * @param OptionsManagerInterface $optionsManager + * @param RowManager $rowManager * @param StyleManager $styleManager + * @param StyleMerger $styleMerger * @param SharedStringsManager $sharedStringsManager * @param XLSXEscaper $stringsEscaper * @param StringHelper $stringHelper @@ -66,14 +76,18 @@ EOD; */ public function __construct( OptionsManagerInterface $optionsManager, + RowManager $rowManager, StyleManager $styleManager, + StyleMerger $styleMerger, SharedStringsManager $sharedStringsManager, XLSXEscaper $stringsEscaper, StringHelper $stringHelper, InternalEntityFactory $entityFactory ) { $this->shouldUseInlineStrings = $optionsManager->getOption(Options::SHOULD_USE_INLINE_STRINGS); + $this->rowManager = $rowManager; $this->styleManager = $styleManager; + $this->styleMerger = $styleMerger; $this->sharedStringsManager = $sharedStringsManager; $this->stringsEscaper = $stringsEscaper; $this->stringHelper = $stringHelper; @@ -121,7 +135,7 @@ EOD; */ public function addRow(Worksheet $worksheet, Row $row) { - if (!$row->isEmpty()) { + if (!$this->rowManager->isEmpty($row)) { $this->addNonEmptyRow($worksheet, $row); } @@ -172,9 +186,11 @@ EOD; */ private function applyStyleAndGetCellXML(Cell $cell, Style $rowStyle, $rowIndex, $cellIndex) { - // Apply styles - the row style is merged at this point - $cell->applyStyle($rowStyle); + // Apply row and extra styles + $mergedCellAndRowStyle = $this->styleMerger->merge($cell->getStyle(), $rowStyle); + $cell->setStyle($mergedCellAndRowStyle); $newCellStyle = $this->styleManager->applyExtraStylesIfNeeded($cell); + $registeredStyle = $this->styleManager->registerStyle($newCellStyle); return $this->getCellXML($rowIndex, $cellIndex, $cell, $registeredStyle->getId()); diff --git a/tests/Spout/Writer/Common/Entity/RowTest.php b/tests/Spout/Writer/Common/Entity/RowTest.php index ff9a1c7..c036813 100644 --- a/tests/Spout/Writer/Common/Entity/RowTest.php +++ b/tests/Spout/Writer/Common/Entity/RowTest.php @@ -3,7 +3,6 @@ namespace Box\Spout\Writer\Common\Entity; use Box\Spout\Writer\Common\Entity\Style\Style; -use Box\Spout\Writer\Common\Manager\RowManager; use PHPUnit\Framework\TestCase; class RowTest extends TestCase @@ -24,23 +23,12 @@ class RowTest extends TestCase return $this->createMock(Cell::class); } - /** - * @return \PHPUnit_Framework_MockObject_MockObject|RowManager - */ - private function getRowManagerMock() - { - return $this->createMock(RowManager::class); - } - /** * @return void */ public function testValidInstance() { - $this->assertInstanceOf( - Row::class, - new Row([], null, $this->getRowManagerMock()) - ); + $this->assertInstanceOf(Row::class, new Row([], null)); } /** @@ -48,7 +36,7 @@ class RowTest extends TestCase */ public function testSetCells() { - $row = new Row([], null, $this->getRowManagerMock()); + $row = new Row([], null); $row->setCells([$this->getCellMock(), $this->getCellMock()]); $this->assertEquals(2, count($row->getCells())); @@ -59,7 +47,7 @@ class RowTest extends TestCase */ public function testSetCellsResets() { - $row = new Row([], null, $this->getRowManagerMock()); + $row = new Row([], null); $row->setCells([$this->getCellMock(), $this->getCellMock()]); $this->assertEquals(2, count($row->getCells())); @@ -74,7 +62,7 @@ class RowTest extends TestCase */ public function testGetCells() { - $row = new Row([], null, $this->getRowManagerMock()); + $row = new Row([], null); $this->assertEquals(0, count($row->getCells())); @@ -88,7 +76,7 @@ class RowTest extends TestCase */ public function testAddCell() { - $row = new Row([], null, $this->getRowManagerMock()); + $row = new Row([], null); $row->setCells([$this->getCellMock(), $this->getCellMock()]); $this->assertEquals(2, count($row->getCells())); @@ -103,7 +91,7 @@ class RowTest extends TestCase */ public function testFluentInterface() { - $row = new Row([], null, $this->getRowManagerMock()); + $row = new Row([], null); $row ->addCell($this->getCellMock()) ->setStyle($this->getStyleMock()) diff --git a/tests/Spout/Writer/Common/Manager/RowManagerTest.php b/tests/Spout/Writer/Common/Manager/RowManagerTest.php index 93c6844..56d6128 100644 --- a/tests/Spout/Writer/Common/Manager/RowManagerTest.php +++ b/tests/Spout/Writer/Common/Manager/RowManagerTest.php @@ -2,7 +2,6 @@ namespace Spout\Writer\Common\Manager; -use Box\Spout\Writer\Common\Creator\Style\StyleBuilder; use Box\Spout\Writer\Common\Entity\Cell; use Box\Spout\Writer\Common\Entity\Row; use Box\Spout\Writer\Common\Manager\RowManager; @@ -11,51 +10,6 @@ use PHPUnit\Framework\TestCase; class RowManagerTest extends TestCase { - /** - * @return void - */ - public function testApplyStyle() - { - $rowManager = new RowManager(new StyleMerger()); - $row = new Row([new Cell('test')], null, $rowManager); - - $this->assertFalse($row->getStyle()->isFontBold()); - - $style = (new StyleBuilder())->setFontBold()->build(); - $rowManager->applyStyle($row, $style); - - $this->assertTrue($row->getStyle()->isFontBold()); - } - - /** - * @return array - */ - public function dataProviderForTestHasCells() - { - return [ - // cells, expected hasCells - [[], false], - [[new Cell('')], true], - [[new Cell(null)], true], - [[new Cell('test')], true], - ]; - } - - /** - * @dataProvider dataProviderForTestHasCells - * - * @param array $cells - * @param bool $expectedHasCells - * @return void - */ - public function testHasCells(array $cells, $expectedHasCells) - { - $rowManager = new RowManager(new StyleMerger()); - - $row = new Row($cells, null, $rowManager); - $this->assertEquals($expectedHasCells, $rowManager->hasCells($row)); - } - /** * @return array */