Skip to content

Commit

Permalink
Fix removing last row incorrect behavior
Browse files Browse the repository at this point in the history 
`$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
  • Loading branch information
@TimGa
TimGa authored Apr 26, 2020
1 parent 6e76d4e commit 3dcc5ca
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 11 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 16 additions & 11 deletions src/PhpSpreadsheet/Worksheet/Worksheet.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
132 changes: 132 additions & 0 deletions tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

0 comments on commit 3dcc5ca

Please sign in to comment.