Fill Pattern Start and End Colors (#2444)

* Fill Pattern Start and End Colors

Fix #2441. The Fill constructor sets start color to white and end color to black and the Xlsx writer writes these values to the output file. This appears to be the wrong setting for all 7 LIGHT* pattern types, 2 of the 7 DARK* patterns (DARKGRAY and DARKTRELLIS), and 1 of the 3 GRAY patterns (GRAY0625). When the wrong colors are written at save time, those patterns are not as expected. Xls writer does not appear to have the same problem.

The XML does not require either a start or end color, and the omission of these colors in the file being read was responsible for the problem. The code is changed to mimic that behavior by omitting the color tags at write time if they have not changed from when they were created by the Fill constructor (they will be written for gradient or solid patterns regardless).

This is another change which is easier to confirm via samples rather than tests. There are separate samples for Xlsx and Xls; as Excel will be quick to warn you, Xls is not as fully functional as Xlsx with respect to fill patterns. The samples do include a cell where one of the cells (LightGrid in C11) explicitly specifies the "default" colors.

* Scrutinizer

It somehow ascribed to me a problem in code which was unchanged by this PR. Correct it anyhow, along with some Phpstan fixes (errors now ignored because of change).

* Added Tests

Also corrected some docBlock problems with Style/*/parent and getSharedComponent.

* Create 2 Abstract Methods

Scrutinizer complained that 2 methods found in all Supervisor sub-types were not defined in Supervisor. Add abstract methods to satisfy it.

* Scrutinizer Ignoring Typehints

Try this instead.

* Slight Improvement

Better handling of Style->getParent().

phpstan-baseline.neon

@@ -5510,66 +5510,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Spreadsheet.php
 
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Alignment.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Border.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getStyleArray\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Border.php
-
-		-
-			message: "#^Parameter \\#1 \\$parent of method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Supervisor\\:\\:bindParent\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style, \\$this\\(PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Border\\) given\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Border.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Borders.php
-
-		-
-			message: "#^Parameter \\#1 \\$parent of method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Supervisor\\:\\:bindParent\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style, \\$this\\(PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Borders\\) given\\.$#"
-			count: 10
-			path: src/PhpSpreadsheet/Style/Borders.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getStyleArray\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Border\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Fill\\:\\:getColor\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Border\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Fill\\:\\:getEndColor\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Border\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Fill\\:\\:getStartColor\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Color\\:\\:getColourComponent\\(\\) should return int\\|string but returns float\\|int\\|string\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Color.php
-
 		-
 			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Conditional\\:\\:\\$condition \\(array\\<string\\>\\) does not accept array\\<bool\\|float\\|int\\|string\\>\\.$#"
 			count: 1
@@ -5740,36 +5680,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Style/ConditionalFormatting/ConditionalFormattingRuleExtension.php
 
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Fill.php
-
-		-
-			message: "#^Parameter \\#1 \\$parent of method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Supervisor\\:\\:bindParent\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style, \\$this\\(PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Fill\\) given\\.$#"
-			count: 2
-			path: src/PhpSpreadsheet/Style/Fill.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Font.php
-
-		-
-			message: "#^Parameter \\#1 \\$parent of method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Supervisor\\:\\:bindParent\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style, \\$this\\(PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Font\\) given\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Font.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat.php
-
-		-
-			message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\:\\:\\$builtInFormatCode \\(int\\|false\\) does not accept bool\\|int\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/NumberFormat.php
-
 		-
 			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\\\DateFormatter\\:\\:escapeQuotesCallback\\(\\) has parameter \\$matches with no type specified\\.$#"
 			count: 1
@@ -5900,21 +5810,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Style/NumberFormat/PercentageFormatter.php
 
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getSharedComponent\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Protection.php
-
-		-
-			message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getCellXfByIndex\\(\\)\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Style.php
-
-		-
-			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\:\\:getParent\\(\\) should return PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet but returns PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\|PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\Style\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Style/Style.php
-
 		-
 			message: "#^Argument of an invalid type mixed supplied for foreach, only iterables are supported\\.$#"
 			count: 1

samples/Basic/47_xlsfill.php

@@ -0,0 +1,13 @@
+<?php
+
+require __DIR__ . '/../Header.php';
+
+use PhpOffice\PhpSpreadsheet\Reader\Xls;
+
+$helper->log('Read spreadsheet');
+$reader = new Xls();
+$spreadsheet = $reader->load(__DIR__ . '/../templates/47_xlsfill.xls');
+
+// Save
+$helper->write($spreadsheet, __FILE__, ['Xls']);
+$spreadsheet->disconnectWorksheets();

samples/Basic/47_xlsxfill.php

@@ -0,0 +1,13 @@
+<?php
+
+require __DIR__ . '/../Header.php';
+
+use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
+
+$helper->log('Read spreadsheet');
+$reader = new Xlsx();
+$spreadsheet = $reader->load(__DIR__ . '/../templates/47_xlsxfill.xlsx');
+
+// Save
+$helper->write($spreadsheet, __FILE__, ['Xlsx']);
+$spreadsheet->disconnectWorksheets();

src/PhpSpreadsheet/Style/Alignment.php

@@ -111,7 +111,10 @@ public function __construct($isSupervisor = false, $isConditional = false)
      */
     public function getSharedComponent()
     {
-        return $this->parent->getSharedComponent()->getAlignment();
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getSharedComponent()->getAlignment();
     }
 
     /**

src/PhpSpreadsheet/Style/Border.php

@@ -70,8 +70,11 @@ public function __construct($isSupervisor = false)
      */
     public function getSharedComponent()
     {
+        /** @var Style */
+        $parent = $this->parent;
+
         /** @var Borders $sharedComponent */
-        $sharedComponent = $this->parent->getSharedComponent();
+        $sharedComponent = $parent->getSharedComponent();
         switch ($this->parentPropertyName) {
             case 'bottom':
                 return $sharedComponent->getBottom();
@@ -97,7 +100,10 @@ public function getSharedComponent()
      */
     public function getStyleArray($array)
     {
-        return $this->parent->getStyleArray([$this->parentPropertyName => $array]);
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getStyleArray([$this->parentPropertyName => $array]);
     }
 
     /**

src/PhpSpreadsheet/Style/Borders.php

@@ -140,7 +140,10 @@ public function __construct($isSupervisor = false)
      */
     public function getSharedComponent()
     {
-        return $this->parent->getSharedComponent()->getBorders();
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getSharedComponent()->getBorders();
     }
 
     /**

src/PhpSpreadsheet/Style/Color.php

@@ -45,6 +45,9 @@ class Color extends Supervisor
      */
     protected $argb;
 
+    /** @var bool */
+    private $hasChanged = false;
+
     /**
      * Create a new Color.
      *
@@ -75,12 +78,15 @@ public function __construct($colorValue = self::COLOR_BLACK, $isSupervisor = fal
      */
     public function getSharedComponent()
     {
+        /** @var Style */
+        $parent = $this->parent;
         /** @var Border|Fill $sharedComponent */
-        $sharedComponent = $this->parent->getSharedComponent();
-        if ($this->parentPropertyName === 'endColor') {
-            return $sharedComponent->getEndColor();
-        }
-        if ($this->parentPropertyName === 'startColor') {
+        $sharedComponent = $parent->getSharedComponent();
+        if ($sharedComponent instanceof Fill) {
+            if ($this->parentPropertyName === 'endColor') {
+                return $sharedComponent->getEndColor();
+            }
+
             return $sharedComponent->getStartColor();
         }
 
@@ -96,7 +102,10 @@ public function getSharedComponent()
      */
     public function getStyleArray($array)
     {
-        return $this->parent->getStyleArray([$this->parentPropertyName => $array]);
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getStyleArray([$this->parentPropertyName => $array]);
     }
 
     /**
@@ -153,6 +162,7 @@ public function getARGB(): ?string
      */
     public function setARGB(?string $colorValue = self::COLOR_BLACK)
     {
+        $this->hasChanged = true;
         if ($colorValue === '' || $colorValue === null) {
             $colorValue = self::COLOR_BLACK;
         } elseif (!$this->validateColor($colorValue, self::VALIDATE_ARGB_SIZE)) {
@@ -190,6 +200,7 @@ public function getRGB(): string
      */
     public function setRGB(?string $colorValue = self::COLOR_BLACK)
     {
+        $this->hasChanged = true;
         if ($colorValue === '' || $colorValue === null) {
             $colorValue = '000000';
         } elseif (!$this->validateColor($colorValue, self::VALIDATE_RGB_SIZE)) {
@@ -220,7 +231,7 @@ private static function getColourComponent($rgbValue, $offset, $hex = true)
     {
         $colour = substr($rgbValue, $offset, 2);
 
-        return ($hex) ? $colour : hexdec($colour);
+        return ($hex) ? $colour : (int) hexdec($colour);
     }
 
     /**
@@ -410,4 +421,13 @@ protected function exportArray1(): array
 
         return $exportedArray;
     }
+
+    public function getHasChanged(): bool
+    {
+        if ($this->isSupervisor) {
+            return $this->getSharedComponent()->hasChanged;
+        }
+
+        return $this->hasChanged;
+    }
 }

src/PhpSpreadsheet/Style/Fill.php

@@ -49,7 +49,7 @@ class Fill extends Supervisor
      *
      * @var float
      */
-    protected $rotation = 0;
+    protected $rotation = 0.0;
 
     /**
      * Start color.
@@ -65,6 +65,9 @@ class Fill extends Supervisor
      */
     protected $endColor;
 
+    /** @var bool */
+    private $colorChanged = false;
+
     /**
      * Create a new Fill.
      *
@@ -102,7 +105,10 @@ public function __construct($isSupervisor = false, $isConditional = false)
      */
     public function getSharedComponent()
     {
-        return $this->parent->getSharedComponent()->getFill();
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getSharedComponent()->getFill();
     }
 
     /**
@@ -124,7 +130,7 @@ public function getStyleArray($array)
      * $spreadsheet->getActiveSheet()->getStyle('B2')->getFill()->applyFromArray(
      *     [
      *         'fillType' => Fill::FILL_GRADIENT_LINEAR,
-     *         'rotation' => 0,
+     *         'rotation' => 0.0,
      *         'startColor' => [
      *             'rgb' => '000000'
      *         ],
@@ -248,6 +254,7 @@ public function getStartColor()
      */
     public function setStartColor(Color $color)
     {
+        $this->colorChanged = true;
         // make sure parameter is a real color and not a supervisor
         $color = $color->getIsSupervisor() ? $color->getSharedComponent() : $color;
 
@@ -278,6 +285,7 @@ public function getEndColor()
      */
     public function setEndColor(Color $color)
     {
+        $this->colorChanged = true;
         // make sure parameter is a real color and not a supervisor
         $color = $color->getIsSupervisor() ? $color->getSharedComponent() : $color;
 
@@ -291,6 +299,17 @@ public function setEndColor(Color $color)
         return $this;
     }
 
+    public function getColorsChanged(): bool
+    {
+        if ($this->isSupervisor) {
+            $changed = $this->getSharedComponent()->colorChanged;
+        } else {
+            $changed = $this->colorChanged;
+        }
+
+        return $changed || $this->startColor->getHasChanged() || $this->endColor->getHasChanged();
+    }
+
     /**
      * Get hash code.
      *
@@ -308,17 +327,20 @@ public function getHashCode()
             $this->getRotation() .
             ($this->getFillType() !== self::FILL_NONE ? $this->getStartColor()->getHashCode() : '') .
             ($this->getFillType() !== self::FILL_NONE ? $this->getEndColor()->getHashCode() : '') .
+            ((string) $this->getColorsChanged()) .
             __CLASS__
         );
     }
 
     protected function exportArray1(): array
     {
         $exportedArray = [];
-        $this->exportArray2($exportedArray, 'endColor', $this->getEndColor());
         $this->exportArray2($exportedArray, 'fillType', $this->getFillType());
         $this->exportArray2($exportedArray, 'rotation', $this->getRotation());
-        $this->exportArray2($exportedArray, 'startColor', $this->getStartColor());
+        if ($this->getColorsChanged()) {
+            $this->exportArray2($exportedArray, 'endColor', $this->getEndColor());
+            $this->exportArray2($exportedArray, 'startColor', $this->getStartColor());
+        }
 
         return $exportedArray;
     }

src/PhpSpreadsheet/Style/Font.php

@@ -122,7 +122,10 @@ public function __construct($isSupervisor = false, $isConditional = false)
      */
     public function getSharedComponent()
     {
-        return $this->parent->getSharedComponent()->getFont();
+        /** @var Style */
+        $parent = $this->parent;
+
+        return $parent->getSharedComponent()->getFont();
     }
 
     /**