Fix removing last row incorrect behavior

`$highestRow = $this->getHighestDataRow();` was calculated after `$this->getCellCollection()->removeRow($pRow + $r);` - this is the root reason for incorrect rows removal because removing last row will change '$this->getHighestDataRow()' value, but removing row from the middle will not change it. So, removing last row causes incorrect `$highestRow` value that is used for wiping out empty rows from the bottom of the table:
```php
for ($r = 0; $r < $pNumRows; ++$r) {
    $this->getCellCollection()->removeRow($highestRow);
    --$highestRow;
}
```
To prevent this incorrect behavior I've moved highest row calculation before row removal.
But this still doesn't solve another problem when trying remove non existing rows: in this case the code above will remove `$pNumRows` rows from below of the table, e.g. if `$highestRow=4` and `$pNumRows=6`, than rows 4, 3, 2, 1, 0, -1 will be deleted. Obviously, this is not good, that is why I've added `$removedRowsCounter` to fix this issue.
And finally, moved Exception to early if statement to get away from unnecessary 'if-else'.

Fixes #1364
Closes #1365

Author: TimGa

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 
 - Fix ROUNDUP and ROUNDDOWN for floating-point rounding error [#1404](https://github.com/PHPOffice/PhpSpreadsheet/pull/1404)
 - Fix loading styles from vmlDrawings when containing whitespace [#1347](https://github.com/PHPOffice/PhpSpreadsheet/issues/1347)
+- Fix incorrect behavior when removing last row [#1365](https://github.com/PHPOffice/PhpSpreadsheet/pull/1365)
 
 ## [1.11.0] - 2020-03-02
 

src/PhpSpreadsheet/Worksheet/Worksheet.php

@@ -2114,20 +2114,25 @@ public function insertNewColumnBeforeByIndex($beforeColumnIndex, $pNumCols = 1)
      */
     public function removeRow($pRow, $pNumRows = 1)
     {
-        if ($pRow >= 1) {
-            for ($r = 0; $r < $pNumRows; ++$r) {
+        if ($pRow < 1) {
+            throw new Exception('Rows to be deleted should at least start from row 1.');
+        }
+
+        $highestRow = $this->getHighestDataRow();
+        $removedRowsCounter = 0;
+
+        for ($r = 0; $r < $pNumRows; ++$r) {
+            if ($pRow + $r <= $highestRow) {
                 $this->getCellCollection()->removeRow($pRow + $r);
+                ++$removedRowsCounter;
             }
+        }
 
-            $highestRow = $this->getHighestDataRow();
-            $objReferenceHelper = ReferenceHelper::getInstance();
-            $objReferenceHelper->insertNewBefore('A' . ($pRow + $pNumRows), 0, -$pNumRows, $this);
-            for ($r = 0; $r < $pNumRows; ++$r) {
-                $this->getCellCollection()->removeRow($highestRow);
-                --$highestRow;
-            }
-        } else {
-            throw new Exception('Rows to be deleted should at least start from row 1.');
+        $objReferenceHelper = ReferenceHelper::getInstance();
+        $objReferenceHelper->insertNewBefore('A' . ($pRow + $pNumRows), 0, -$pNumRows, $this);
+        for ($r = 0; $r < $removedRowsCounter; ++$r) {
+            $this->getCellCollection()->removeRow($highestRow);
+            --$highestRow;
         }
 
         return $this;

tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php

@@ -272,4 +272,136 @@ public function testRemoveColumn(
         self::assertSame($expectedHighestColumn, $worksheet->getHighestColumn());
         self::assertSame($expectedData, $worksheet->toArray());
     }
+
+    public function removeRowsProvider()
+    {
+        return [
+            'Remove all rows except first one' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                2,
+                3,
+                [
+                    ['A1', 'B1', 'C1'],
+                ],
+                1,
+            ],
+            'Remove all rows except last one' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                1,
+                3,
+                [
+                    ['A4', 'B4', 'C4'],
+                ],
+                1,
+            ],
+            'Remove last row' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                4,
+                1,
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                ],
+                3,
+            ],
+            'Remove first row' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                1,
+                1,
+                [
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                3,
+            ],
+            'Remove all rows except first and last' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                2,
+                2,
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                2,
+            ],
+            'Remove non existing rows' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                2,
+                10,
+                [
+                    ['A1', 'B1', 'C1'],
+                ],
+                1,
+            ],
+            'Remove only non existing rows' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                5,
+                10,
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                    ['A3', 'B3', 'C3'],
+                    ['A4', 'B4', 'C4'],
+                ],
+                4,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider removeRowsProvider
+     */
+    public function testRemoveRows(
+        array $initialData,
+        int $rowToRemove,
+        int $rowsQtyToRemove,
+        array $expectedData,
+        int $expectedHighestRow
+    ) {
+        $workbook = new Spreadsheet();
+        $worksheet = $workbook->getActiveSheet();
+        $worksheet->fromArray($initialData);
+
+        $worksheet->removeRow($rowToRemove, $rowsQtyToRemove);
+
+        self::assertSame($expectedData, $worksheet->toArray());
+        self::assertSame($expectedHighestRow, $worksheet->getHighestRow());
+    }
 }