From 89066d256868efea7b8b2051b7a1ca4a52f6d2b4 Mon Sep 17 00:00:00 2001
From: David Arenas <darenas31415@users.noreply.github.com>
Date: Mon, 28 Oct 2019 17:52:06 +0000
Subject: [PATCH] Bugfix/remove column out of range (#1197)

* Call garbage collector after removing a column

Otherwise callers of getHighestColumn get stale values

* Update changelog

* Fix remove a column out of range removes the last column

Given:
+---+---+
| A | B |
+---+---+
Attempting to remove 'D', should not alter the worksheet

* Avoid side effects when trying to remove more columns than exists
---
 CHANGELOG.md                                  |  1 +
 src/PhpSpreadsheet/Worksheet/Worksheet.php    | 33 ++++++++++++-------
 .../Worksheet/WorksheetTest.php               | 32 +++++++++++++++++-
 3 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 56989e4b..00465650 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 ### Fixed
 
 - Call garbage collector after removing a column to prevent stale cached values
+- Trying to remove a column that doesn't exist deletes the latest column
 
 ## [1.9.0] - 2019-08-17
 
diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php
index 2a7c529d..7777199c 100644
--- a/src/PhpSpreadsheet/Worksheet/Worksheet.php
+++ b/src/PhpSpreadsheet/Worksheet/Worksheet.php
@@ -2145,20 +2145,31 @@ class Worksheet implements IComparable
      */
     public function removeColumn($pColumn, $pNumCols = 1)
     {
-        if (!is_numeric($pColumn)) {
-            $highestColumn = $this->getHighestDataColumn();
-            $pColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($pColumn) + $pNumCols);
-            $objReferenceHelper = ReferenceHelper::getInstance();
-            $objReferenceHelper->insertNewBefore($pColumn . '1', -$pNumCols, 0, $this);
-            for ($c = 0; $c < $pNumCols; ++$c) {
-                $this->getCellCollection()->removeColumn($highestColumn);
-                $highestColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($highestColumn) - 1);
-            }
-            $this->garbageCollect();
-        } else {
+        if (is_numeric($pColumn)) {
             throw new Exception('Column references should not be numeric.');
         }
 
+        $highestColumn = $this->getHighestDataColumn();
+        $highestColumnIndex = Coordinate::columnIndexFromString($highestColumn);
+        $pColumnIndex = Coordinate::columnIndexFromString($pColumn);
+
+        if ($pColumnIndex > $highestColumnIndex) {
+            return $this;
+        }
+
+        $pColumn = Coordinate::stringFromColumnIndex($pColumnIndex + $pNumCols);
+        $objReferenceHelper = ReferenceHelper::getInstance();
+        $objReferenceHelper->insertNewBefore($pColumn . '1', -$pNumCols, 0, $this);
+
+        $maxPossibleColumnsToBeRemoved = $highestColumnIndex - $pColumnIndex + 1;
+
+        for ($c = 0, $n = min($maxPossibleColumnsToBeRemoved, $pNumCols); $c < $n; ++$c) {
+            $this->getCellCollection()->removeColumn($highestColumn);
+            $highestColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($highestColumn) - 1);
+        }
+
+        $this->garbageCollect();
+
         return $this;
     }
 
diff --git a/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php b/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php
index b6fcb57c..e1b4738b 100644
--- a/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php
+++ b/tests/PhpSpreadsheetTests/Worksheet/WorksheetTest.php
@@ -191,6 +191,7 @@ class WorksheetTest extends TestCase
                     ['A2', 'B2', 'C2'],
                 ],
                 'A',
+                1,
                 [
                     ['B1', 'C1'],
                     ['B2', 'C2'],
@@ -203,6 +204,7 @@ class WorksheetTest extends TestCase
                     ['A2', 'B2', 'C2'],
                 ],
                 'B',
+                1,
                 [
                     ['A1', 'C1'],
                     ['A2', 'C2'],
@@ -215,12 +217,39 @@ class WorksheetTest extends TestCase
                     ['A2', 'B2', 'C2'],
                 ],
                 'C',
+                1,
                 [
                     ['A1', 'B1'],
                     ['A2', 'B2'],
                 ],
                 'B',
             ],
+            'Remove a column out of range' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                ],
+                'D',
+                1,
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                ],
+                'C',
+            ],
+            'Remove multiple columns' => [
+                [
+                    ['A1', 'B1', 'C1'],
+                    ['A2', 'B2', 'C2'],
+                ],
+                'B',
+                5,
+                [
+                    ['A1'],
+                    ['A2'],
+                ],
+                'A',
+            ],
         ];
     }
 
@@ -230,6 +259,7 @@ class WorksheetTest extends TestCase
     public function testRemoveColumn(
         array $initialData,
         string $columnToBeRemoved,
+        int $columnsToBeRemoved,
         array $expectedData,
         string $expectedHighestColumn
     ) {
@@ -237,7 +267,7 @@ class WorksheetTest extends TestCase
         $worksheet = $spreadsheet->getActiveSheet();
         $worksheet->fromArray($initialData);
 
-        $worksheet->removeColumn($columnToBeRemoved);
+        $worksheet->removeColumn($columnToBeRemoved, $columnsToBeRemoved);
 
         self::assertSame($expectedHighestColumn, $worksheet->getHighestColumn());
         self::assertSame($expectedData, $worksheet->toArray());