Fix Chart Problems and Memory Leak in Xlsx Writer (#2930)

This was supposed to be mopping up some longstanding chart issues. But one of the sample files exposed a memory leak in Xlsx Writer, unrelated to charts. Since that is my best sample file for this problem, I would like to fix both problems at the same time.

Xlsx Writer for Worksheets calls getRowDimension for all rows on the sheet. As it happens, the sample file had data in the last rows after a huge gap of rows without any data. It correctly did not write anything for the unused rows. However, the call to getRowDimension actually creates a new RowDimension object if it doesn't already exist, and so it wound up creating over a million totally unneeded objects. This caused it to run out of memory when I tried to make a copy of the 8K input file. The logic is changed to call getRowDimension if and only if (there is data in the row or the RowDimension object already exists). It still has to loop through a million rows, but it no longer allocates the unneeded storage.

As for the Chart problems - fix #1797. This is where the file that caused the memory leak originated. Many of its problems were already resolved by the earlier large set of changes to Charts. However, there were a few new properties that needed to be added to Layout to make things complete - numberFormat code and source-linked, and dLblPos (position for labels); and autoTitleDeleted needs to be added to Charts.

Also fix #2077, by allowing the format to be specified in the Layout rather than the DataSeriesValues constructor.

src/PhpSpreadsheet/Chart/Chart.php

@@ -141,6 +141,9 @@ class Chart
     /** @var bool */
     private $oneCellAnchor = false;
 
+    /** @var bool */
+    private $autoTitleDeleted = false;
+
     /**
      * Create a new Chart.
      * majorGridlines and minorGridlines are deprecated, moved to Axis.
@@ -732,4 +735,16 @@ class Chart
 
         return $this;
     }
+
+    public function getAutoTitleDeleted(): bool
+    {
+        return $this->autoTitleDeleted;
+    }
+
+    public function setAutoTitleDeleted(bool $autoTitleDeleted): self
+    {
+        $this->autoTitleDeleted = $autoTitleDeleted;
+
+        return $this;
+    }
 }

src/PhpSpreadsheet/Chart/Layout.php

@@ -53,6 +53,19 @@ class Layout
      */
     private $height;
 
+    /**
+     * Position - t=top.
+     *
+     * @var string
+     */
+    private $dLblPos = '';
+
+    /** @var string */
+    private $numFmtCode = '';
+
+    /** @var bool */
+    private $numFmtLinked = false;
+
     /**
      * show legend key
      * Specifies that legend keys should be shown in data labels.
@@ -143,6 +156,12 @@ class Layout
         if (isset($layout['h'])) {
             $this->height = (float) $layout['h'];
         }
+        if (isset($layout['dLblPos'])) {
+            $this->dLblPos = (string) $layout['dLblPos'];
+        }
+        if (isset($layout['numFmtCode'])) {
+            $this->numFmtCode = (string) $layout['numFmtCode'];
+        }
         $this->initBoolean($layout, 'showLegendKey');
         $this->initBoolean($layout, 'showVal');
         $this->initBoolean($layout, 'showCatName');
@@ -150,6 +169,7 @@ class Layout
         $this->initBoolean($layout, 'showPercent');
         $this->initBoolean($layout, 'showBubbleSize');
         $this->initBoolean($layout, 'showLeaderLines');
+        $this->initBoolean($layout, 'numFmtLinked');
         $this->initColor($layout, 'labelFillColor');
         $this->initColor($layout, 'labelBorderColor');
         $this->initColor($layout, 'labelFontColor');
@@ -484,4 +504,40 @@ class Layout
 
         return $this;
     }
+
+    public function getDLblPos(): string
+    {
+        return $this->dLblPos;
+    }
+
+    public function setDLblPos(string $dLblPos): self
+    {
+        $this->dLblPos = $dLblPos;
+
+        return $this;
+    }
+
+    public function getNumFmtCode(): string
+    {
+        return $this->numFmtCode;
+    }
+
+    public function setNumFmtCode(string $numFmtCode): self
+    {
+        $this->numFmtCode = $numFmtCode;
+
+        return $this;
+    }
+
+    public function getNumFmtLinked(): bool
+    {
+        return $this->numFmtLinked;
+    }
+
+    public function setNumFmtLinked(bool $numFmtLinked): self
+    {
+        $this->numFmtLinked = $numFmtLinked;
+
+        return $this;
+    }
 }

src/PhpSpreadsheet/Reader/Xlsx/Chart.php

@@ -72,12 +72,18 @@ class Chart
         $rotX = $rotY = $rAngAx = $perspective = null;
         $xAxis = new Axis();
         $yAxis = new Axis();
+        $autoTitleDeleted = null;
         foreach ($chartElementsC as $chartElementKey => $chartElement) {
             switch ($chartElementKey) {
                 case 'chart':
                     foreach ($chartElement as $chartDetailsKey => $chartDetails) {
                         $chartDetailsC = $chartDetails->children($this->cNamespace);
                         switch ($chartDetailsKey) {
+                            case 'autoTitleDeleted':
+                                /** @var bool */
+                                $autoTitleDeleted = self::getAttribute($chartElementsC->chart->autoTitleDeleted, 'val', 'boolean');
+
+                                break;
                             case 'view3D':
                                 $rotX = self::getAttribute($chartDetails->rotX, 'val', 'integer');
                                 $rotY = self::getAttribute($chartDetails->rotY, 'val', 'integer');
@@ -324,6 +330,9 @@ class Chart
             }
         }
         $chart = new \PhpOffice\PhpSpreadsheet\Chart\Chart($chartName, $title, $legend, $plotArea, $plotVisOnly, (string) $dispBlanksAs, $XaxisLabel, $YaxisLabel, $xAxis, $yAxis);
+        if (is_bool($autoTitleDeleted)) {
+            $chart->setAutoTitleDeleted($autoTitleDeleted);
+        }
         if (is_int($rotX)) {
             $chart->setRotX($rotX);
         }
@@ -967,6 +976,13 @@ class Chart
     {
         $plotAttributes = [];
         if (isset($chartDetail->dLbls)) {
+            if (isset($chartDetail->dLbls->dLblPos)) {
+                $plotAttributes['dLblPos'] = self::getAttribute($chartDetail->dLbls->dLblPos, 'val', 'string');
+            }
+            if (isset($chartDetail->dLbls->numFmt)) {
+                $plotAttributes['numFmtCode'] = self::getAttribute($chartDetail->dLbls->numFmt, 'formatCode', 'string');
+                $plotAttributes['numFmtLinked'] = self::getAttribute($chartDetail->dLbls->numFmt, 'sourceLinked', 'boolean');
+            }
             if (isset($chartDetail->dLbls->showLegendKey)) {
                 $plotAttributes['showLegendKey'] = self::getAttribute($chartDetail->dLbls->showLegendKey, 'val', 'string');
             }

src/PhpSpreadsheet/Worksheet/Worksheet.php

@@ -1413,6 +1413,11 @@ class Worksheet implements IComparable
         return $this->rowDimensions[$row];
     }
 
+    public function rowDimensionExists(int $row): bool
+    {
+        return isset($this->rowDimensions[$row]);
+    }
+
     /**
      * Get column dimension at a specific column.
      *

src/PhpSpreadsheet/Writer/Xlsx/Chart.php

@@ -68,7 +68,7 @@ class Chart extends WriterPart
         $this->writeTitle($objWriter, $chart->getTitle());
 
         $objWriter->startElement('c:autoTitleDeleted');
-        $objWriter->writeAttribute('val', '0');
+        $objWriter->writeAttribute('val', (string) (int) $chart->getAutoTitleDeleted());
         $objWriter->endElement();
 
         $objWriter->startElement('c:view3D');
@@ -420,6 +420,17 @@ class Chart extends WriterPart
             $objWriter->endElement(); // c:txPr
         }
 
+        if ($chartLayout->getNumFmtCode() !== '') {
+            $objWriter->startElement('c:numFmt');
+            $objWriter->writeAttribute('formatCode', $chartLayout->getnumFmtCode());
+            $objWriter->writeAttribute('sourceLinked', (string) (int) $chartLayout->getnumFmtLinked());
+            $objWriter->endElement(); // c:numFmt
+        }
+        if ($chartLayout->getDLblPos() !== '') {
+            $objWriter->startElement('c:dLblPos');
+            $objWriter->writeAttribute('val', $chartLayout->getDLblPos());
+            $objWriter->endElement(); // c:dLblPos
+        }
         $this->writeDataLabelsBool($objWriter, 'showLegendKey', $chartLayout->getShowLegendKey());
         $this->writeDataLabelsBool($objWriter, 'showVal', $chartLayout->getShowVal());
         $this->writeDataLabelsBool($objWriter, 'showCatName', $chartLayout->getShowCatName());

src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php

@@ -1155,11 +1155,13 @@ class Worksheet extends WriterPart
 
         $currentRow = 0;
         while ($currentRow++ < $highestRow) {
+            $isRowSet = isset($cellsByRow[$currentRow]);
+            if ($isRowSet || $worksheet->rowDimensionExists($currentRow)) {
                 // Get row dimension
                 $rowDimension = $worksheet->getRowDimension($currentRow);
 
                 // Write current row?
-            $writeCurrentRow = isset($cellsByRow[$currentRow]) || $rowDimension->getRowHeight() >= 0 || $rowDimension->getVisible() === false || $rowDimension->getCollapsed() === true || $rowDimension->getOutlineLevel() > 0 || $rowDimension->getXfIndex() !== null;
+                $writeCurrentRow = $isRowSet || $rowDimension->getRowHeight() >= 0 || $rowDimension->getVisible() === false || $rowDimension->getCollapsed() === true || $rowDimension->getOutlineLevel() > 0 || $rowDimension->getXfIndex() !== null;
 
                 if ($writeCurrentRow) {
                     // Start a new row
@@ -1209,6 +1211,7 @@ class Worksheet extends WriterPart
                     $objWriter->endElement();
                 }
             }
+        }
 
         $objWriter->endElement();
     }

tests/PhpSpreadsheetTests/Chart/Issue2077Test.php

@@ -0,0 +1,108 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Chart;
+
+use PhpOffice\PhpSpreadsheet\Chart\Chart;
+use PhpOffice\PhpSpreadsheet\Chart\DataSeries;
+use PhpOffice\PhpSpreadsheet\Chart\DataSeriesValues;
+use PhpOffice\PhpSpreadsheet\Chart\Layout;
+use PhpOffice\PhpSpreadsheet\Chart\Legend as ChartLegend;
+use PhpOffice\PhpSpreadsheet\Chart\PlotArea;
+use PhpOffice\PhpSpreadsheet\Chart\Title;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Style\NumberFormat;
+use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;
+use PHPUnit\Framework\TestCase;
+
+class Issue2077Test extends TestCase
+{
+    public function testPercentLabels(): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $worksheet = $spreadsheet->getActiveSheet();
+        $worksheet->fromArray(
+            [
+                ['', '2010', '2011', '2012'],
+                ['Q1', 12, 15, 21],
+                ['Q2', 56, 73, 86],
+                ['Q3', 52, 61, 69],
+                ['Q4', 30, 32, 60],
+            ]
+        );
+
+        // Set the Labels for each data series we want to plot
+        $dataSeriesLabels1 = [
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_STRING, 'Worksheet!$B$1', null, 1), // 2011
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_STRING, 'Worksheet!$C$1', null, 1), // 2012
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_STRING, 'Worksheet!$D$1', null, 1), // 2013
+        ];
+
+        // Set the X-Axis Labels
+        $xAxisTickValues1 = [
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_STRING, 'Worksheet!$A$2:$A$5', null, 4), // Q1 to Q4
+        ];
+
+        // Set the Data values for each data series we want to plot
+        // TODO  I think the third parameter can be set，but I didn't succeed
+        $dataSeriesValues1 = [
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_NUMBER, 'Worksheet!$B$2:$B$5', null, 4),
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_NUMBER, 'Worksheet!$C$2:$C$5', NumberFormat::FORMAT_NUMBER_00, 4),
+            new DataSeriesValues(DataSeriesValues::DATASERIES_TYPE_NUMBER, 'Worksheet!$D$2:$D$5', NumberFormat::FORMAT_PERCENTAGE_00, 4),
+        ];
+
+        // Build the dataseries
+        $series1 = [
+            new DataSeries(
+                DataSeries::TYPE_PIECHART, // plotType
+                null, // plotGrouping (Pie charts don't have any grouping)
+                range(0, count($dataSeriesValues1) - 1), // plotOrder
+                $dataSeriesLabels1, // plotLabel
+                $xAxisTickValues1, // plotCategory
+                $dataSeriesValues1 // plotValues
+            ),
+        ];
+
+        // Set up a layout object for the Pie chart
+        $layout1 = new Layout();
+        $layout1->setShowVal(true);
+        // Set the layout to show percentage with 2 decimal points
+        $layout1->setShowPercent(true);
+        $layout1->setNumFmtCode(NumberFormat::FORMAT_PERCENTAGE_00);
+
+        // Set the series in the plot area
+        $plotArea1 = new PlotArea($layout1, $series1);
+
+        // Set the chart legend
+        $legend1 = new ChartLegend(ChartLegend::POSITION_RIGHT, null, false);
+
+        $title1 = new Title('Test Pie Chart');
+
+        $yAxisLabel = new Title('Value ($k)');
+        // Create the chart
+        $chart1 = new Chart(
+            'chart1', // name
+            $title1, // title
+            $legend1, // legend
+            $plotArea1, // plotArea
+            true, // plotVisibleOnly
+            'gap', // displayBlanksAs
+            null, // xAxisLabel
+            $yAxisLabel
+        );
+
+        // Set the position where the chart should appear in the worksheet
+        $chart1->setTopLeftPosition('A7');
+        $chart1->setBottomRightPosition('H20');
+
+        // Add the chart to the worksheet
+        $worksheet->addChart($chart1);
+
+        $writer = new XlsxWriter($spreadsheet);
+        $writer->setIncludeCharts(true);
+        $writerChart = new XlsxWriter\Chart($writer);
+        $data = $writerChart->writeChart($chart1);
+        self::assertStringContainsString('<c:dLbls><c:numFmt formatCode="0.00%" sourceLinked="0"/><c:showVal val="1"/><c:showPercent val="1"/></c:dLbls>', $data);
+
+        $spreadsheet->disconnectWorksheets();
+    }
+}

tests/PhpSpreadsheetTests/Chart/LayoutTest.php

@@ -42,6 +42,9 @@ class LayoutTest extends TestCase
             'w' => 3.0,
             'h' => 4.0,
             'showVal' => true,
+            'dLblPos' => 't',
+            'numFmtCode' => '0.00%',
+            'numFmtLinked' => true,
             'labelFillColor' => $fillColor,
             'labelBorderColor' => $borderColor,
             'labelFontColor' => $fontColor,
@@ -56,6 +59,9 @@ class LayoutTest extends TestCase
             ->setWidth(3.0)
             ->setHeight(4.0)
             ->setShowVal(true)
+            ->setDLblPos('t')
+            ->setNumFmtCode('0.00%')
+            ->setNumFmtLinked(true)
             ->setLabelFillColor($fillColor)
             ->setLabelBorderColor($borderColor)
             ->setLabelFontColor($fontColor);