From 3dcc5ca7533180cba95ede7d2db5f9ce008ad082 Mon Sep 17 00:00:00 2001 From: Tim Gavryutenko <35343569+TimGa@users.noreply.github.com> Date: Sun, 26 Apr 2020 05:00:43 +0300 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + src/PhpSpreadsheet/Worksheet/Worksheet.php | 27 ++-- .../Worksheet/WorksheetTest.php | 132 ++++++++++++++++++ 3 files changed, 149 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e65a1a88c..391115a6c0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php index f3a5b4da78..9a3f96471d 100644 --- a/src/PhpSpreadsheet/Worksheet/Worksheet.php +++ b/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; diff --git a/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php b/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php index e1b4738b81..d1e19df429 100644 --- a/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php +++ b/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()); + } }