Code review changes

src/Spout/Common/Entity/Cell.php

@@ -2,7 +2,6 @@
 
 namespace Box\Spout\Common\Entity;
 
-use Box\Spout\Common\Entity\Style\EmptyStyle;
 use Box\Spout\Common\Entity\Style\Style;
 use Box\Spout\Common\Helper\CellTypeHelper;
 
@@ -105,7 +104,7 @@ class Cell
      */
     public function setStyle($style)
     {
-        $this->style = $style ?: new EmptyStyle();
+        $this->style = $style ?: new Style();
     }
 
     /**

src/Spout/Common/Entity/Style/EmptyStyle.php

@@ -1,7 +0,0 @@
-<?php
-
-namespace Box\Spout\Common\Entity\Style;
-
-class EmptyStyle extends Style
-{
-}

src/Spout/Common/Entity/Style/Style.php

@@ -87,6 +87,9 @@ class Style
     /** @var bool */
     private $isRegistered = false;
 
+    /** @var bool */
+    private $isEmpty = true;
+
     /**
      * @return int|null
      */
@@ -122,6 +125,7 @@ class Style
     {
         $this->shouldApplyBorder = true;
         $this->border = $border;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -150,6 +154,7 @@ class Style
         $this->fontBold = true;
         $this->hasSetFontBold = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -178,6 +183,7 @@ class Style
         $this->fontItalic = true;
         $this->hasSetFontItalic = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -206,6 +212,7 @@ class Style
         $this->fontUnderline = true;
         $this->hasSetFontUnderline = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -234,6 +241,7 @@ class Style
         $this->fontStrikethrough = true;
         $this->hasSetFontStrikethrough = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -263,6 +271,7 @@ class Style
         $this->fontSize = $fontSize;
         $this->hasSetFontSize = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -294,6 +303,7 @@ class Style
         $this->fontColor = $fontColor;
         $this->hasSetFontColor = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -323,6 +333,7 @@ class Style
         $this->fontName = $fontName;
         $this->hasSetFontName = true;
         $this->shouldApplyFont = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -353,6 +364,7 @@ class Style
         $this->cellAlignment = $cellAlignment;
         $this->hasSetCellAlignment = true;
         $this->shouldApplyCellAlignment = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -389,6 +401,7 @@ class Style
     {
         $this->shouldWrapText = $shouldWrap;
         $this->hasSetWrapText = true;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -418,6 +431,7 @@ class Style
     {
         $this->hasSetBackgroundColor = true;
         $this->backgroundColor = $color;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -447,6 +461,7 @@ class Style
     {
         $this->hasSetFormat = true;
         $this->format = $format;
+        $this->isEmpty = false;
 
         return $this;
     }
@@ -467,11 +482,6 @@ class Style
         return $this->hasSetFormat;
     }
 
-    public function setRegistered(bool $isRegistered = true) : void
-    {
-        $this->isRegistered = $isRegistered;
-    }
-
     /**
      * @return bool
      */
@@ -485,4 +495,15 @@ class Style
         $this->setId($id);
         $this->isRegistered = true;
     }
+
+    public function unregister() : void
+    {
+        $this->setId(0);
+        $this->isRegistered = false;
+    }
+
+    public function isEmpty() : bool
+    {
+        return $this->isEmpty;
+    }
 }

src/Spout/Writer/Common/Manager/Style/ManagedStyle.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace Box\Spout\Writer\Common\Manager\Style;
+
+use Box\Spout\Common\Entity\Style\Style;
+
+class ManagedStyle
+{
+    private $style;
+    private $isUpdated;
+
+    public function __construct(Style $style, bool $isUpdated)
+    {
+        $this->style = $style;
+        $this->isUpdated = $isUpdated;
+    }
+
+    public function getStyle() : Style
+    {
+        return $this->style;
+    }
+
+    public function isUpdated() : bool
+    {
+        return $this->isUpdated;
+    }
+}

src/Spout/Writer/Common/Manager/Style/StyleManager.php

@@ -50,9 +50,9 @@ class StyleManager implements StyleManagerInterface
      * Typically, set "wrap text" if a cell contains a new line.
      *
      * @param Cell $cell
-     * @return Style|null The eventually updated style
+     * @return ManagedStyle The eventually updated style
      */
-    public function applyExtraStylesIfNeeded(Cell $cell) : ?Style
+    public function applyExtraStylesIfNeeded(Cell $cell) : ManagedStyle
     {
         return $this->applyWrapTextIfCellContainsNewLine($cell);
     }
@@ -67,23 +67,19 @@ class StyleManager implements StyleManagerInterface
      *        on the Windows version of Excel...
      *
      * @param Cell $cell The cell the style should be applied to
-     * @return Style|null The eventually updated style
+     * @return ManagedStyle The eventually updated style
      */
-    protected function applyWrapTextIfCellContainsNewLine(Cell $cell) : ?Style
+    protected function applyWrapTextIfCellContainsNewLine(Cell $cell) : ManagedStyle
     {
         $cellStyle = $cell->getStyle();
 
         // if the "wrap text" option is already set, no-op
-        if ($cellStyle->hasSetWrapText()) {
+        if (!$cellStyle->hasSetWrapText() && $cell->isString() && \strpos($cell->getValue(), "\n") !== false) {
-            return null;
-        }
-
-        if ($cell->isString() && \strpos($cell->getValue(), "\n") !== false) {
             $cellStyle->setShouldWrapText();
 
-            return $cellStyle;
+            return new ManagedStyle($cellStyle, true);
         }
 
-        return null;
+        return new ManagedStyle($cellStyle, false);
     }
 }

src/Spout/Writer/Common/Manager/Style/StyleManagerInterface.php

@@ -24,7 +24,7 @@ interface StyleManagerInterface
      * Typically, set "wrap text" if a cell contains a new line.
      *
      * @param Cell $cell
-     * @return Style|null The eventually updated style
+     * @return ManagedStyle|null The eventually updated style
      */
-    public function applyExtraStylesIfNeeded(Cell $cell) : ?Style;
+    public function applyExtraStylesIfNeeded(Cell $cell) : ManagedStyle;
 }

src/Spout/Writer/Common/Manager/Style/StyleRegistry.php

@@ -114,8 +114,7 @@ class StyleRegistry
     {
         // In order to be able to properly compare style, set static ID value and reset registration
         $currentId = $style->getId();
-        $style->setId(0);
+        $style->unregister();
-        $style->setRegistered(false);
 
         $serializedStyle = \serialize($style);
 

src/Spout/Writer/ODS/Manager/WorksheetManager.php

@@ -4,7 +4,6 @@ namespace Box\Spout\Writer\ODS\Manager;
 
 use Box\Spout\Common\Entity\Cell;
 use Box\Spout\Common\Entity\Row;
-use Box\Spout\Common\Entity\Style\EmptyStyle;
 use Box\Spout\Common\Entity\Style\Style;
 use Box\Spout\Common\Exception\InvalidArgumentException;
 use Box\Spout\Common\Exception\IOException;
@@ -158,13 +157,13 @@ class WorksheetManager implements WorksheetManagerInterface
      */
     private function applyStyleAndGetCellXML(Cell $cell, Style &$rowStyle, $currentCellIndex, $nextCellIndex)
     {
-        if ($cell->getStyle() instanceof EmptyStyle) {
+        if ($cell->getStyle()->isEmpty()) {
             $cell->setStyle($rowStyle);
 
-            $extraStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
+            $managedStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
 
-            if ($extraStyle) {
+            if ($managedStyle->isUpdated()) {
-                $registeredStyle = $this->styleManager->registerStyle($extraStyle);
+                $registeredStyle = $this->styleManager->registerStyle($managedStyle->getStyle());
             } else {
                 $registeredStyle = $rowStyle = $this->styleManager->registerStyle($rowStyle);
             }
@@ -172,7 +171,13 @@ class WorksheetManager implements WorksheetManagerInterface
             $mergedCellAndRowStyle = $this->styleMerger->merge($cell->getStyle(), $rowStyle);
             $cell->setStyle($mergedCellAndRowStyle);
 
-            $newCellStyle = $this->styleManager->applyExtraStylesIfNeeded($cell) ?: $mergedCellAndRowStyle;
+            $managedStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
+            if ($managedStyle->isUpdated()) {
+                $newCellStyle = $managedStyle->getStyle();
+            } else {
+                $newCellStyle = $mergedCellAndRowStyle;
+            }
+
             $registeredStyle = $this->styleManager->registerStyle($newCellStyle);
         }
 

src/Spout/Writer/XLSX/Manager/WorksheetManager.php

@@ -4,7 +4,6 @@ namespace Box\Spout\Writer\XLSX\Manager;
 
 use Box\Spout\Common\Entity\Cell;
 use Box\Spout\Common\Entity\Row;
-use Box\Spout\Common\Entity\Style\EmptyStyle;
 use Box\Spout\Common\Entity\Style\Style;
 use Box\Spout\Common\Exception\InvalidArgumentException;
 use Box\Spout\Common\Exception\IOException;
@@ -186,13 +185,13 @@ EOD;
      */
     private function applyStyleAndGetCellXML(Cell $cell, Style &$rowStyle, $rowIndexOneBased, $columnIndexZeroBased)
     {
-        if ($cell->getStyle() instanceof EmptyStyle) {
+        if ($cell->getStyle()->isEmpty()) {
             $cell->setStyle($rowStyle);
 
-            $extraStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
+            $managedStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
 
-            if ($extraStyle) {
+            if ($managedStyle->isUpdated()) {
-                $registeredStyle = $this->styleManager->registerStyle($extraStyle);
+                $registeredStyle = $this->styleManager->registerStyle($managedStyle->getStyle());
             } else {
                 $registeredStyle = $rowStyle = $this->styleManager->registerStyle($rowStyle);
             }
@@ -200,7 +199,14 @@ EOD;
             $mergedCellAndRowStyle = $this->styleMerger->merge($cell->getStyle(), $rowStyle);
             $cell->setStyle($mergedCellAndRowStyle);
 
-            $newCellStyle = $this->styleManager->applyExtraStylesIfNeeded($cell) ?: $mergedCellAndRowStyle;
+            $managedStyle = $this->styleManager->applyExtraStylesIfNeeded($cell);
+
+            if ($managedStyle->isUpdated()) {
+                $newCellStyle = $managedStyle->getStyle();
+            } else {
+                $newCellStyle = $mergedCellAndRowStyle;
+            }
+
             $registeredStyle = $this->styleManager->registerStyle($newCellStyle);
         }
 

tests/Spout/Writer/CSV/WriterPerfTest.php

@@ -61,7 +61,7 @@ class WriterPerfTest extends TestCase
      */
     private function getNumWrittenRows($resourcePath)
     {
-        $lineCountResult = `wc -l $resourcePath`;
+        $lineCountResult = shell_exec("wc -l $resourcePath");
 
         return (int) $lineCountResult;
     }

tests/Spout/Writer/Common/Manager/Style/StyleManagerTest.php

@@ -28,10 +28,10 @@ class StyleManagerTest extends TestCase
         $this->assertFalse($style->shouldWrapText());
 
         $styleManager = $this->getStyleManager();
-        $updatedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell("multi\nlines", $style));
+        $managedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell("multi\nlines", $style));
 
-        $this->assertNotNull($updatedStyle);
+        $this->assertTrue($managedStyle->isUpdated());
-        $this->assertTrue($updatedStyle->shouldWrapText());
+        $this->assertTrue($managedStyle->getStyle()->shouldWrapText());
     }
 
     public function testApplyExtraStylesIfNeededShouldReturnNullIfWrapTextNotNeeded() : void
@@ -40,9 +40,9 @@ class StyleManagerTest extends TestCase
         $this->assertFalse($style->shouldWrapText());
 
         $styleManager = $this->getStyleManager();
-        $updatedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell('oneline', $style));
+        $managedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell('oneline', $style));
 
-        $this->assertNull($updatedStyle);
+        $this->assertFalse($managedStyle->isUpdated());
     }
 
     public function testApplyExtraStylesIfNeededShouldReturnNullIfWrapTextAlreadyApplied() : void
@@ -51,8 +51,8 @@ class StyleManagerTest extends TestCase
         $this->assertTrue($style->shouldWrapText());
 
         $styleManager = $this->getStyleManager();
-        $updatedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell("multi\nlines", $style));
+        $managedStyle = $styleManager->applyExtraStylesIfNeeded(new Cell("multi\nlines", $style));
 
-        $this->assertNull($updatedStyle);
+        $this->assertFalse($managedStyle->isUpdated());
     }
 }

tests/Spout/Writer/ODS/WriterPerfTest.php

@@ -88,7 +88,7 @@ class WriterPerfTest extends TestCase
         copy($pathToContentXmlFile, $tmpFile);
 
         // Get the last 200 characters
-        $lastCharacters = `tail -c 200 $tmpFile`;
+        $lastCharacters = shell_exec("tail -c 200 $tmpFile");
 
         // remove the temporary file
         unlink($tmpFile);

tests/Spout/Writer/XLSX/WriterPerfTest.php

@@ -135,7 +135,7 @@ class WriterPerfTest extends TestCase
         copy($filePath, $tmpFile);
 
         // Get the last 200 characters
-        $lastCharacters = `tail -c $numCharacters $tmpFile`;
+        $lastCharacters = shell_exec("tail -c $numCharacters $tmpFile");
 
         // remove the temporary file
         unlink($tmpFile);