Fix #276, some general refinement (#289)

* Fix #276, some general refinement

* Failing test #267

* Fixed shared border definitions across different styles #267

* Fix finding the correct borderId

src/Spout/Writer/XLSX/Helper/StyleHelper.php

@@ -32,6 +32,16 @@ class StyleHelper extends AbstractStyleHelper
      */
     protected $fillIndex = 2;
 
+    /**
+     * @var array
+     */
+    protected $registeredBorders = [];
+
+    /**
+     * @var array [STYLE_ID] => [BORDER_ID] maps a style to a border declaration
+     */
+    protected $styleIdToBorderMappingTable = [];
+
     /**
      * XLSX specific operations on the registered styles
      *
@@ -42,6 +52,7 @@ class StyleHelper extends AbstractStyleHelper
     {
         $registeredStyle = parent::registerStyle($style);
         $this->registerFill($registeredStyle);
+        $this->registerBorder($registeredStyle);
         return $registeredStyle;
     }
 
@@ -71,6 +82,36 @@ class StyleHelper extends AbstractStyleHelper
         }
     }
 
+    /**
+     * Register a border definition
+     *
+     * @param $style
+     */
+    protected function registerBorder($style)
+    {
+        $styleId = $style->getId();
+
+        if ($style->shouldApplyBorder()) {
+            $border = $style->getBorder();
+            $serializedBorder = serialize($border);
+
+            $isBorderAlreadyRegistered = isset($this->registeredBorders[$serializedBorder]);
+
+            if ($isBorderAlreadyRegistered) {
+                $registeredStyleId = $this->registeredBorders[$serializedBorder];
+                $registeredBorderId = $this->styleIdToBorderMappingTable[$registeredStyleId];
+                $this->styleIdToBorderMappingTable[$styleId] = $registeredBorderId;
+            } else {
+                $this->registeredBorders[$serializedBorder] = $styleId;
+                $this->styleIdToBorderMappingTable[$styleId] = count($this->registeredBorders);
+            }
+
+        } else { 
+            // If no border should be applied - the mapping is the default border: 0
+            $this->styleIdToBorderMappingTable[$styleId] = 0;
+        }
+    }
+
 
     /**
      * Returns the content of the "styles.xml" file, given a list of styles.
@@ -152,7 +193,6 @@ EOD;
 
         // The other fills are actually registered by setting a background color
         foreach ($this->registeredFills as $styleId) {
-
             /** @var Style $style */
             $style = $this->styleIdToStyleMappingTable[$styleId];
 
@@ -175,15 +215,19 @@ EOD;
      */
     protected function getBordersSectionContent()
     {
-        $registeredStyles = $this->getRegisteredStyles();
-        $registeredStylesCount = count($registeredStyles);
 
-        $content = '<borders count="' . $registeredStylesCount . '">';
+        // There is one default border with index 0
+        $borderCount = count($this->registeredBorders) + 1;
 
+        $content = '<borders count="' . $borderCount . '">';
+
+        // Default border starting at index 0
+        $content .= '<border><left/><right/><top/><bottom/></border>';
+
+        foreach ($this->registeredBorders as $styleId) {
             /** @var \Box\Spout\Writer\Style\Style $style */
-        foreach ($registeredStyles as $style) {
+            $style = $this->styleIdToStyleMappingTable[$styleId];
             $border = $style->getBorder();
-            if ($border) {
             $content .= '<border>';
 
             // @link https://github.com/box/spout/issues/271
@@ -195,13 +239,10 @@ EOD;
                     $part = $border->getPart($partName);
                     $content .= BorderHelper::serializeBorderPart($part);
                 }
+
             }
 
             $content .= '</border>';
-
-            } else {
-                $content .= '<border><left/><right/><top/><bottom/></border>';
-            }
         }
 
         $content .= '</borders>';
@@ -235,19 +276,17 @@ EOD;
         $content = '<cellXfs count="' . count($registeredStyles) . '">';
 
         foreach ($registeredStyles as $style) {
-
             $styleId = $style->getId();
             $fillId = $this->styleIdToFillMappingTable[$styleId];
+            $borderId = $this->styleIdToBorderMappingTable[$styleId];
 
-            $content .= '<xf numFmtId="0" fontId="' . $styleId . '" fillId="' . $fillId . '" borderId="' . $styleId . '" xfId="0"';
+            $content .= '<xf numFmtId="0" fontId="' . $styleId . '" fillId="' . $fillId . '" borderId="' . $borderId . '" xfId="0"';
 
             if ($style->shouldApplyFont()) {
                 $content .= ' applyFont="1"';
             }
 
-            if ($style->shouldApplyBorder()) {
+            $content .= sprintf(' applyBorder="%d"', $style->shouldApplyBorder() ? 1 : 0);
-                $content .= ' applyBorder="1"';
-            }
 
             if ($style->shouldWrapText()) {
                 $content .= ' applyAlignment="1">';

tests/Spout/Writer/XLSX/WriterWithStyleTest.php

@@ -359,6 +359,79 @@ class WriterWithStyleTest extends \PHPUnit_Framework_TestCase
         $this->assertFirstChildHasAttributeEquals((string) $defaultFontSize, $defaultFontElement, 'sz', 'val');
     }
 
+    /**
+     * @return void
+     */
+    public function testReUseBorders()
+    {
+
+        $fileName = 'test_reuse_borders.xlsx';
+
+        $borderLeft = (new BorderBuilder())
+            ->setBorderLeft()
+            ->build();
+        $borderLeftStyle = (new StyleBuilder())->setBorder($borderLeft)->build();
+
+        $borderRight = (new BorderBuilder())
+            ->setBorderRight(Color::RED, Border::WIDTH_THICK)
+            ->build();
+        $borderRightStyle = (new StyleBuilder())->setBorder($borderRight)->build();
+
+        $fontStyle = (new StyleBuilder())->setFontBold()->build();
+        $emptyStyle = (new StyleBuilder())->build();
+
+        $borderRightFontBoldStyle = $borderRightStyle->mergeWith($fontStyle);
+
+        $dataRows = [
+            ['Border-Left'],
+            ['Empty'],
+            ['Font-Bold'],
+            ['Border-Right'],
+            ['Border-Right-Font-Bold'],
+        ];
+
+        $styles = [
+            $borderLeftStyle,
+            $emptyStyle,
+            $fontStyle,
+            $borderRightStyle,
+            $borderRightFontBoldStyle
+        ];
+        
+        $this->writeToXLSXFileWithMultipleStyles($dataRows, $fileName, $styles);
+        $borderElements = $this->getXmlSectionFromStylesXmlFile($fileName, 'borders');
+
+        $this->assertEquals(3, $borderElements->getAttribute('count'), '3 borders in count attribute');
+        $this->assertEquals(3, $borderElements->childNodes->length, '3 border childnodes present');
+
+        /** @var \DOMElement $firstBorder */
+        $firstBorder = $borderElements->childNodes->item(1); // 0  = default border
+        $leftStyle = $firstBorder->getElementsByTagName('left')->item(0)->getAttribute('style');
+        $this->assertEquals('medium', $leftStyle, 'Style is medium');
+
+        /** @var \DOMElement $secondBorder */
+        $secondBorder = $borderElements->childNodes->item(2);
+        $rightStyle = $secondBorder->getElementsByTagName('right')->item(0)->getAttribute('style');
+        $this->assertEquals('thick', $rightStyle, 'Style is thick');
+
+        $styleXfsElements = $this->getXmlSectionFromStylesXmlFile($fileName, 'cellXfs');
+
+        // A rather relaxed test
+        // Where a border is applied - the borderId attribute has to be greater than 0
+        $bordersApplied = 0;
+        /** @var \DOMElement $node */
+        foreach ($styleXfsElements->childNodes as $node) {
+            if ($node->getAttribute('applyBorder') == 1) {
+                $bordersApplied++;
+                $this->assertTrue((int)$node->getAttribute('borderId') > 0, 'BorderId is greater than 0');
+            } else {
+                $this->assertTrue((int)$node->getAttribute('borderId') === 0, 'BorderId is 0');
+            }
+        }
+
+        $this->assertEquals(3, $bordersApplied, 'Three borders have been applied');
+    }
+
     /**
      * @param array $allRows
      * @param string $fileName