StyleMerger and RowManager changes

Move DI of StyleMerger to get rid of business logic in the entities Cell and Row
Simplify RowManager

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();
+        return new Row($cells, $rowStyle);
-        $rowManager = new RowManager($styleMerger);
-
-        return new Row($cells, $rowStyle, $rowManager);
     }
 
     /**
@@ -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);
     }
 
     /**

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;
-    }
 }

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);
-    }
 }

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

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);
     }
 
     /**

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
+        // Apply row and extra styles
-        $cell->applyStyle($rowStyle);
+        $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
 

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

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
+        // Apply row and extra styles
-        $cell->applyStyle($rowStyle);
+        $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());

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(
+        $this->assertInstanceOf(Row::class, new Row([], null));
-            Row::class,
-            new Row([], null, $this->getRowManagerMock())
-        );
     }
 
     /**
@@ -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())

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
      */