Restore RGB/ARGB Interchangeability

Fix #2494. Apparently EPPlus outputs fill colors as `<fgColor rgb="BFBFBF">` while most output fill colors as `<fgColor rgb="FFBFBFBF">`. EPPlus actually makes more sense. Regardless, validating length of rgb/argb is a recent development for PhpSpreadsheet, under the assumption that an incorrect length is a user error. This development invalidates that assumption, so restore the previous behavior.

In addition, a comment in Colors.php says that the supplied color is "the ARGB value for the colour, or named colour". However, although named colors are accepted, nothing sensible is done with them - they are passed unchanged to the ARGB value, where Excel treats them as black. The routine should either reject the named color, or convert it to the appropriate ARGB value. This change implements the latter.

Author: oleibman

src/PhpSpreadsheet/Style/Color.php

@@ -26,10 +26,24 @@ class Color extends Supervisor
     const COLOR_DARKGREEN = 'FF008000';
     const COLOR_YELLOW = 'FFFFFF00';
     const COLOR_DARKYELLOW = 'FF808000';
+    const COLOR_MAGENTA = 'FFFF00FF';
+    const COLOR_CYAN = 'FF00FFFF';
+
+    const NAMED_COLOR_TRANSLATIONS = [
+        'Black' => self::COLOR_BLACK,
+        'White' => self::COLOR_WHITE,
+        'Red' => self::COLOR_RED,
+        'Green' => self::COLOR_GREEN,
+        'Blue' => self::COLOR_BLUE,
+        'Yellow' => self::COLOR_YELLOW,
+        'Magenta' => self::COLOR_MAGENTA,
+        'Cyan' => self::COLOR_CYAN,
+    ];
 
     const VALIDATE_ARGB_SIZE = 8;
     const VALIDATE_RGB_SIZE = 6;
-    const VALIDATE_COLOR_VALUE = '/^[A-F0-9]{%d}$/i';
+    const VALIDATE_COLOR_6 = '/^[A-F0-9]{6}$/i';
+    const VALIDATE_COLOR_8 = '/^[A-F0-9]{8}$/i';
 
     /**
      * Indexed colors array.
@@ -66,7 +80,7 @@ public function __construct($colorValue = self::COLOR_BLACK, $isSupervisor = fal
 
         //    Initialise values
         if (!$isConditional) {
-            $this->argb = $this->validateColor($colorValue, self::VALIDATE_ARGB_SIZE) ? $colorValue : self::COLOR_BLACK;
+            $this->argb = $this->validateColor($colorValue) ?: self::COLOR_BLACK;
         }
     }
 
@@ -135,10 +149,23 @@ public function applyFromArray(array $styleArray)
         return $this;
     }
 
-    private function validateColor(string $colorValue, int $size): bool
+    private function validateColor(?string $colorValue): string
     {
-        return in_array(ucfirst(strtolower($colorValue)), self::NAMED_COLORS) ||
-            preg_match(sprintf(self::VALIDATE_COLOR_VALUE, $size), $colorValue);
+        if ($colorValue === null || $colorValue === '') {
+            return self::COLOR_BLACK;
+        }
+        $named = ucfirst(strtolower($colorValue));
+        if (array_key_exists($named, self::NAMED_COLOR_TRANSLATIONS)) {
+            return self::NAMED_COLOR_TRANSLATIONS[$named];
+        }
+        if (preg_match(self::VALIDATE_COLOR_8, $colorValue) === 1) {
+            return $colorValue;
+        }
+        if (preg_match(self::VALIDATE_COLOR_6, $colorValue) === 1) {
+            return 'FF' . $colorValue;
+        }
+
+        return '';
     }
 
     /**
@@ -163,9 +190,8 @@ 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)) {
+        $colorValue = $this->validateColor($colorValue);
+        if ($colorValue === '') {
             return $this;
         }
 
@@ -200,21 +226,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)) {
-            return $this;
-        }
-
-        if ($this->isSupervisor) {
-            $styleArray = $this->getStyleArray(['argb' => 'FF' . $colorValue]);
-            $this->getActiveSheet()->getStyle($this->getSelectedCells())->applyFromArray($styleArray);
-        } else {
-            $this->argb = 'FF' . $colorValue;
-        }
-
-        return $this;
+        return $this->setARGB($colorValue);
     }
 
     /**

tests/PhpSpreadsheetTests/CommentTest.php

@@ -65,7 +65,7 @@ public function testSetFillColor(): void
     {
         $comment = new Comment();
         $comment->setFillColor(new Color('RED'));
-        self::assertEquals('RED', $comment->getFillColor()->getARGB());
+        self::assertEquals(Color::COLOR_RED, $comment->getFillColor()->getARGB());
     }
 
     public function testSetAlignment(): void

tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2494Test.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
+
+use PhpOffice\PhpSpreadsheet\IOFactory;
+use PHPUnit\Framework\TestCase;
+
+class Issue2494Test extends TestCase
+{
+    public function testIssue2494(): void
+    {
+        // Fill style incorrect.
+        $filename = 'tests/data/Reader/XLSX/issue.2494.xlsx';
+        $reader = IOFactory::createReader('Xlsx');
+        $spreadsheet = $reader->load($filename);
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertTrue($sheet->getCell('C3')->getStyle()->getFont()->getBold());
+        self::assertSame('FFBFBFBF', $sheet->getCell('D8')->getStyle()->getFill()->getStartColor()->getArgb());
+        $spreadsheet->disconnectWorksheets();
+    }
+}

tests/PhpSpreadsheetTests/Style/ColorTest.php

@@ -157,4 +157,31 @@ public function testDefaultColor(): void
         self::assertEquals(Color::COLOR_BLACK, $color->getARGB());
         self::assertEquals('000000', $color->getRGB());
     }
+
+    public function testNamedColors(): void
+    {
+        $color = new Color();
+        $color->setARGB('Blue');
+        self::assertEquals(Color::COLOR_BLUE, $color->getARGB());
+        $color->setARGB('black');
+        self::assertEquals(Color::COLOR_BLACK, $color->getARGB());
+        $color->setARGB('wHite');
+        self::assertEquals(Color::COLOR_WHITE, $color->getARGB());
+        $color->setRGB('reD');
+        self::assertEquals(Color::COLOR_RED, $color->getARGB());
+        $color->setRGB('GREEN');
+        self::assertEquals(Color::COLOR_GREEN, $color->getARGB());
+        $color->setRGB('magenta');
+        self::assertEquals(Color::COLOR_MAGENTA, $color->getARGB());
+        $color->setRGB('YeLlOw');
+        self::assertEquals(Color::COLOR_YELLOW, $color->getARGB());
+        $color->setRGB('CYAN');
+        self::assertEquals(Color::COLOR_CYAN, $color->getARGB());
+        $color->setRGB('123456ab');
+        self::assertEquals('123456ab', $color->getARGB());
+        self::assertEquals('3456ab', $color->getRGB());
+        $color->setARGB('3456cd');
+        self::assertEquals('FF3456cd', $color->getARGB());
+        self::assertEquals('3456cd', $color->getRGB());
+    }
 }