Permit Max Column for Row Breaks (PHPOffice#3345)

* Permit Max Column for Row Breaks

Fix PHPOffice#3143. Page break was dropped. Difference between good and bad was the use of attribute `max` in `brk` tag in the good spreadsheet. However, `max` was *not* required in a similar spreadsheet. So the reason for the problem isn't completely explained. Nevertheless, it can't really hurt to capture the `max` value on read (if present) and generate it on write. This resolves the issue. User is also permitted to specify max column when setting a row break programatically. I am not yet in position to document when that might be a good idea.

* Case-sensitive Directory Name

Not a problem on my Windows system.

* Update Documentation and Add Tests

Change is necessitated by probable Excel bug.

* Unhappy With Initial Implementation

I kind of shoe-horned it in. Better to create a new PageBreak class, which will make it easier to accomodate any future surprises about page break handling. The only difficulty with the new approach is making sure getBreaks maintains backwards compatibility. New tests will ensure that.

docs/topics/recipes.md

@@ -479,6 +479,15 @@ row 10.
 $spreadsheet->getActiveSheet()->setBreak('A10', \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_ROW);
 ```
 
+If your print break is inside a defined print area, it may be necessary to add an extra parameter to specify the max column (and this probably won't hurt if the break is not inside a defined print area):
+
+```php
+$spreadsheet->getActiveSheet()
+    ->setBreak('A10',
+        \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_ROW,
+        \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_ROW_MAX_COLUMN);
+```
+
 The following line of code sets a print break on column D:
 
 ```php

src/PhpSpreadsheet/Reader/Xlsx/PageSetup.php

@@ -151,8 +151,9 @@ private function pageBreaks(SimpleXMLElement $xmlSheet, Worksheet $worksheet): v
     private function rowBreaks(SimpleXMLElement $xmlSheet, Worksheet $worksheet): void
     {
         foreach ($xmlSheet->rowBreaks->brk as $brk) {
+            $rowBreakMax = isset($brk['max']) ? ((int) $brk['max']) : -1;
             if ($brk['man']) {
-                $worksheet->setBreak("A{$brk['id']}", Worksheet::BREAK_ROW);
+                $worksheet->setBreak("A{$brk['id']}", Worksheet::BREAK_ROW, $rowBreakMax);
             }
         }
     }

src/PhpSpreadsheet/Worksheet/PageBreak.php

@@ -0,0 +1,58 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheet\Worksheet;
+
+use PhpOffice\PhpSpreadsheet\Calculation\Functions;
+use PhpOffice\PhpSpreadsheet\Cell\CellAddress;
+use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
+
+class PageBreak
+{
+    /** @var int */
+    private $breakType;
+
+    /** @var string */
+    private $coordinate;
+
+    /** @var int */
+    private $maxColOrRow;
+
+    /** @param array|CellAddress|string $coordinate */
+    public function __construct(int $breakType, $coordinate, int $maxColOrRow = -1)
+    {
+        $coordinate = Functions::trimSheetFromCellReference(Validations::validateCellAddress($coordinate));
+        $this->breakType = $breakType;
+        $this->coordinate = $coordinate;
+        $this->maxColOrRow = $maxColOrRow;
+    }
+
+    public function getBreakType(): int
+    {
+        return $this->breakType;
+    }
+
+    public function getCoordinate(): string
+    {
+        return $this->coordinate;
+    }
+
+    public function getMaxColOrRow(): int
+    {
+        return $this->maxColOrRow;
+    }
+
+    public function getColumnInt(): int
+    {
+        return Coordinate::indexesFromString($this->coordinate)[0];
+    }
+
+    public function getRow(): int
+    {
+        return Coordinate::indexesFromString($this->coordinate)[1];
+    }
+
+    public function getColumnString(): string
+    {
+        return Coordinate::indexesFromString($this->coordinate)[2];
+    }
+}

src/PhpSpreadsheet/Worksheet/Worksheet.php

@@ -35,6 +35,8 @@ class Worksheet implements IComparable
     public const BREAK_NONE = 0;
     public const BREAK_ROW = 1;
     public const BREAK_COLUMN = 2;
+    // Maximum column for row break
+    public const BREAK_ROW_MAX_COLUMN = 16383;
 
     // Sheet state
     public const SHEETSTATE_VISIBLE = 'visible';
@@ -188,11 +190,18 @@ class Worksheet implements IComparable
     private $conditionalStylesCollection = [];
 
     /**
-     * Collection of breaks.
+     * Collection of row breaks.
      *
-     * @var int[]
+     * @var PageBreak[]
      */
-    private $breaks = [];
+    private $rowBreaks = [];
+
+    /**
+     * Collection of column breaks.
+     *
+     * @var PageBreak[]
+     */
+    private $columnBreaks = [];
 
     /**
      * Collection of merged cell ranges.
@@ -1748,16 +1757,16 @@ public function duplicateConditionalStyle(array $styles, $range = '')
      *
      * @return $this
      */
-    public function setBreak($coordinate, $break)
+    public function setBreak($coordinate, $break, int $max = -1)
     {
         $cellAddress = Functions::trimSheetFromCellReference(Validations::validateCellAddress($coordinate));
 
         if ($break === self::BREAK_NONE) {
-            if (isset($this->breaks[$cellAddress])) {
-                unset($this->breaks[$cellAddress]);
-            }
-        } else {
-            $this->breaks[$cellAddress] = $break;
+            unset($this->rowBreaks[$cellAddress], $this->columnBreaks[$cellAddress]);
+        } elseif ($break === self::BREAK_ROW) {
+            $this->rowBreaks[$cellAddress] = new PageBreak($break, $cellAddress, $max);
+        } elseif ($break === self::BREAK_COLUMN) {
+            $this->columnBreaks[$cellAddress] = new PageBreak($break, $cellAddress, $max);
         }
 
         return $this;
@@ -1789,7 +1798,35 @@ public function setBreakByColumnAndRow($columnIndex, $row, $break)
      */
     public function getBreaks()
     {
-        return $this->breaks;
+        $breaks = [];
+        foreach ($this->rowBreaks as $break) {
+            $breaks[$break->getCoordinate()] = self::BREAK_ROW;
+        }
+        foreach ($this->columnBreaks as $break) {
+            $breaks[$break->getCoordinate()] = self::BREAK_COLUMN;
+        }
+
+        return $breaks;
+    }
+
+    /**
+     * Get row breaks.
+     *
+     * @return PageBreak[]
+     */
+    public function getRowBreaks()
+    {
+        return $this->rowBreaks;
+    }
+
+    /**
+     * Get row breaks.
+     *
+     * @return PageBreak[]
+     */
+    public function getColumnBreaks()
+    {
+        return $this->columnBreaks;
     }
 
     /**

src/PhpSpreadsheet/Writer/Html.php

@@ -1193,7 +1193,7 @@ private function generateRowStart(Worksheet $worksheet, $sheetIndex, $row)
     {
         $html = '';
         if (count($worksheet->getBreaks()) > 0) {
-            $breaks = $worksheet->getBreaks();
+            $breaks = $worksheet->getRowBreaks();
 
             // check if a break is needed before this row
             if (isset($breaks['A' . $row])) {

src/PhpSpreadsheet/Writer/Xls/Worksheet.php

@@ -2022,27 +2022,15 @@ private function writeBreaks(): void
         $vbreaks = [];
         $hbreaks = [];
 
-        foreach ($this->phpSheet->getBreaks() as $cell => $breakType) {
+        foreach ($this->phpSheet->getRowBreaks() as $cell => $break) {
             // Fetch coordinates
             $coordinates = Coordinate::coordinateFromString($cell);
-
-            // Decide what to do by the type of break
-            switch ($breakType) {
-                case \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_COLUMN:
-                    // Add to list of vertical breaks
-                    $vbreaks[] = Coordinate::columnIndexFromString($coordinates[0]) - 1;
-
-                    break;
-                case \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_ROW:
-                    // Add to list of horizontal breaks
-                    $hbreaks[] = $coordinates[1];
-
-                    break;
-                case \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::BREAK_NONE:
-                default:
-                    // Nothing to do
-                    break;
-            }
+            $hbreaks[] = $coordinates[1];
+        }
+        foreach ($this->phpSheet->getColumnBreaks() as $cell => $break) {
+            // Fetch coordinates
+            $coordinates = Coordinate::indexesFromString($cell);
+            $vbreaks[] = $coordinates[0] - 1;
         }
 
         //horizontal page breaks

src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php

@@ -1006,12 +1006,11 @@ private function writeBreaks(XMLWriter $objWriter, PhpspreadsheetWorksheet $work
         // Get row and column breaks
         $aRowBreaks = [];
         $aColumnBreaks = [];
-        foreach ($worksheet->getBreaks() as $cell => $breakType) {
-            if ($breakType == PhpspreadsheetWorksheet::BREAK_ROW) {
-                $aRowBreaks[] = $cell;
-            } elseif ($breakType == PhpspreadsheetWorksheet::BREAK_COLUMN) {
-                $aColumnBreaks[] = $cell;
-            }
+        foreach ($worksheet->getRowBreaks() as $cell => $break) {
+            $aRowBreaks[$cell] = $break;
+        }
+        foreach ($worksheet->getColumnBreaks() as $cell => $break) {
+            $aColumnBreaks[$cell] = $break;
         }
 
         // rowBreaks
@@ -1020,12 +1019,16 @@ private function writeBreaks(XMLWriter $objWriter, PhpspreadsheetWorksheet $work
             $objWriter->writeAttribute('count', (string) count($aRowBreaks));
             $objWriter->writeAttribute('manualBreakCount', (string) count($aRowBreaks));
 
-            foreach ($aRowBreaks as $cell) {
+            foreach ($aRowBreaks as $cell => $break) {
                 $coords = Coordinate::coordinateFromString($cell);
 
                 $objWriter->startElement('brk');
                 $objWriter->writeAttribute('id', $coords[1]);
                 $objWriter->writeAttribute('man', '1');
+                $rowBreakMax = $break->getMaxColOrRow();
+                if ($rowBreakMax >= 0) {
+                    $objWriter->writeAttribute('max', "$rowBreakMax");
+                }
                 $objWriter->endElement();
             }
 
@@ -1038,11 +1041,11 @@ private function writeBreaks(XMLWriter $objWriter, PhpspreadsheetWorksheet $work
             $objWriter->writeAttribute('count', (string) count($aColumnBreaks));
             $objWriter->writeAttribute('manualBreakCount', (string) count($aColumnBreaks));
 
-            foreach ($aColumnBreaks as $cell) {
+            foreach ($aColumnBreaks as $cell => $break) {
                 $coords = Coordinate::coordinateFromString($cell);
 
                 $objWriter->startElement('brk');
-                $objWriter->writeAttribute('id', (string) (Coordinate::columnIndexFromString($coords[0]) - 1));
+                $objWriter->writeAttribute('id', (string) ((int) $coords[0] - 1));
                 $objWriter->writeAttribute('man', '1');
                 $objWriter->endElement();
             }

tests/PhpSpreadsheetTests/Reader/Xlsx/RowBreakTest.php

@@ -0,0 +1,69 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
+
+use PhpOffice\PhpSpreadsheet\Reader\Xlsx as XlsxReader;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
+use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;
+use PHPUnit\Framework\TestCase;
+
+class RowBreakTest extends TestCase
+{
+    public function testReadAndWriteRowBreak(): void
+    {
+        $file = 'tests/data/Reader/XLSX/issue.3143a.xlsx';
+        $reader = new XlsxReader();
+        $spreadsheet = $reader->load($file);
+        $sheet = $spreadsheet->getActiveSheet();
+        $writer = new XlsxWriter($spreadsheet);
+        $writerWorksheet = new XlsxWriter\Worksheet($writer);
+        $data = $writerWorksheet->writeWorksheet($sheet, []);
+        $expected = '<rowBreaks count="1" manualBreakCount="1"><brk id="25" man="1" max="16383"/></rowBreaks>';
+        self::assertStringContainsString($expected, $data);
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public function testWriteRowBreakInPrintAreaWithMax(): void
+    {
+        // This test specifies max for setBreak and appears correct.
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        for ($row = 1; $row < 60; ++$row) {
+            for ($column = 'A'; $column !== 'L'; ++$column) {
+                $cell = $column . $row;
+                $sheet->getCell($cell)->setValue($cell);
+            }
+        }
+        $sheet->getPageSetup()->setPrintArea('B2:J55');
+        $sheet->setBreak('A25', Worksheet::BREAK_ROW, Worksheet::BREAK_ROW_MAX_COLUMN);
+        $writer = new XlsxWriter($spreadsheet);
+        $writerWorksheet = new XlsxWriter\Worksheet($writer);
+        $data = $writerWorksheet->writeWorksheet($sheet, []);
+        $expected = '<rowBreaks count="1" manualBreakCount="1"><brk id="25" man="1" max="16383"/></rowBreaks>';
+        self::assertStringContainsString($expected, $data);
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public function testWriteRowBreakInPrintAreaWithoutMax(): void
+    {
+        // This test does not specify max for setBreak,
+        // and appears incorrect. Probable Excel bug.
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        for ($row = 1; $row < 60; ++$row) {
+            for ($column = 'A'; $column !== 'L'; ++$column) {
+                $cell = $column . $row;
+                $sheet->getCell($cell)->setValue($cell);
+            }
+        }
+        $sheet->getPageSetup()->setPrintArea('B2:J55');
+        $sheet->setBreak('A25', Worksheet::BREAK_ROW);
+        $writer = new XlsxWriter($spreadsheet);
+        $writerWorksheet = new XlsxWriter\Worksheet($writer);
+        $data = $writerWorksheet->writeWorksheet($sheet, []);
+        $expected = '<rowBreaks count="1" manualBreakCount="1"><brk id="25" man="1"/></rowBreaks>';
+        self::assertStringContainsString($expected, $data);
+        $spreadsheet->disconnectWorksheets();
+    }
+}