WIP Removing Rows or Columns that Include Edge Ranges

oleibman · oleibman · commit 99d49554e2cd · 2023-04-20T14:26:38.000-07:00
This PR will probably remain in draft status for some time. It needs updates, and I need more time to understand exactly what it is doing. It is a replacement for PR PHPOffice#2096, intended as a fix for issue PHPOffice#1449. That PR has been stuck, with a good deal of acrimony, for over 2 years. In the interim, all the parameters for the functions involved were renamed, the functions were refactored to have fewer parameters with new class properties replacing the missing parameters, special code was added for absolute references, and code was refactored putting some of the methods involved in a new class under a new method name. In addition, the original PR was not Php8-compliant (one of the sticking points regarding implementing it), and there were a great many formatting problems (another sticking point). This has made it very difficult to figure out exactly what was intended in the original PR. This PR (a) addresses some of the issues associated with the original issue (deleting columns/rows at the end of a range passed to a formula *on the same sheet*), and (b) does not cause any existing tests to fail. Many more unit tests are needed (another sticking point); in particular, there are changes involving data validations, comments, defined names, and other elements which are not adequately tested. Aside from those, PhpSpreadsheet does not adjust formulas on a different sheet which involve deleted rows/columns, and it should, as Excel does. Similarly, it appears that Excel doesn't do anything extraordinary when the right/bottom part of the range is deleted, but when the left/top part is deleted, it converts the range to #REF! at least some of the time. This needs more research; if confirmed, it needs to be added to PhpSpreadsheet.
diff --git a/src/PhpSpreadsheet/CellReferenceHelper.php b/src/PhpSpreadsheet/CellReferenceHelper.php
@@ -17,6 +17,15 @@ class CellReferenceHelper
      */
     protected $beforeColumn;
 
+    /** @var string */
+    protected $beforeColumnString;
+
+    /** @var bool */
+    protected $beforeColumnAbsolute = false;
+
+    /** @var bool */
+    protected $beforeRowAbsolute = false;
+
     /**
      * @var int
      */
@@ -34,12 +43,15 @@ class CellReferenceHelper
 
     public function __construct(string $beforeCellAddress = 'A1', int $numberOfColumns = 0, int $numberOfRows = 0)
     {
+        $this->beforeColumnAbsolute = $beforeCellAddress[0] === '$';
+        $this->beforeRowAbsolute = strpos($beforeCellAddress, '$', 1) !== false;
         $this->beforeCellAddress = str_replace('$', '', $beforeCellAddress);
         $this->numberOfColumns = $numberOfColumns;
         $this->numberOfRows = $numberOfRows;
 
         // Get coordinate of $beforeCellAddress
         [$beforeColumn, $beforeRow] = Coordinate::coordinateFromString($beforeCellAddress);
+        $this->beforeColumnString = $beforeColumn;
         $this->beforeColumn = (int) Coordinate::columnIndexFromString($beforeColumn);
         $this->beforeRow = (int) $beforeRow;
     }
@@ -56,7 +68,7 @@ public function refreshRequired(string $beforeCellAddress, int $numberOfColumns,
             $this->numberOfRows !== $numberOfRows;
     }
 
-    public function updateCellReference(string $cellReference = 'A1', bool $includeAbsoluteReferences = false): string
+    public function updateCellReference(string $cellReference = 'A1', bool $includeAbsoluteReferences = false, ?bool $topLeft = null): string
     {
         if (Coordinate::coordinateIsRange($cellReference)) {
             throw new Exception('Only single cell references may be passed to this method.');
@@ -74,8 +86,46 @@ public function updateCellReference(string $cellReference = 'A1', bool $includeA
             $updateColumn = (($absoluteColumn !== '$') && $newColumnIndex >= $this->beforeColumn);
             $updateRow = (($absoluteRow !== '$') && $newRowIndex >= $this->beforeRow);
         } else {
-            $updateColumn = ($newColumnIndex >= $this->beforeColumn);
-            $updateRow = ($newRowIndex >= $this->beforeRow);
+            // A special case is removing the left/top or bottom/right edge of a range
+            // $topLeft is null if we aren't adjusting a range at all.
+            if (
+                $topLeft !== null
+                && $this->numberOfColumns < 0
+                && $newColumnIndex >= $this->beforeColumn + $this->numberOfColumns
+                && $newColumnIndex <= $this->beforeColumn - 1
+            ) {
+                if ($topLeft) {
+                    $newColumnIndex = $this->beforeColumn + $this->numberOfColumns;
+                    $newColumn = Coordinate::stringFromColumnIndex($newColumnIndex);
+                } else {
+                    $newColumnIndex = $this->beforeColumn + $this->numberOfColumns - 1;
+                }
+            } elseif ($newColumnIndex >= $this->beforeColumn) {
+                // Create new column reference
+                $newColumnIndex += $this->numberOfColumns;
+            }
+            $newColumn = $absoluteColumn . Coordinate::stringFromColumnIndex($newColumnIndex);
+            //$updateColumn = ($newColumnIndex >= $this->beforeColumn);
+            $updateColumn = false;
+            // A special case is removing the left/top or bottom/right edge of a range
+            // $topLeft is null if we aren't adjusting a range at all.
+            if (
+                $topLeft !== null
+                && $this->numberOfRows < 0
+                && $newRowIndex >= $this->beforeRow + $this->numberOfRows
+                && $newRowIndex <= $this->beforeRow - 1
+            ) {
+                if ($topLeft) {
+                    $newRowIndex = $this->beforeRow + $this->numberOfRows;
+                } else {
+                    $newRowIndex = $this->beforeRow + $this->numberOfRows - 1;
+                }
+            } elseif ($newRowIndex >= $this->beforeRow) {
+                $newRowIndex = $newRowIndex + $this->numberOfRows;
+            }
+            $newRow = $absoluteRow . $newRowIndex;
+            //$updateRow = ($newRowIndex >= $this->beforeRow);
+            $updateRow = false;
         }
 
         // Create new column reference
diff --git a/src/PhpSpreadsheet/ReferenceHelper.php b/src/PhpSpreadsheet/ReferenceHelper.php
@@ -154,7 +154,7 @@ protected function adjustPageBreaks(Worksheet $worksheet, int $numberOfColumns,
             } else {
                 //    Otherwise update any affected breaks by inserting a new break at the appropriate point
                 //        and removing the old affected break
-                $newReference = $this->updateCellReference($cellAddress);
+                $newReference = $this->updateCellReference($cellAddress, false, null);
                 if ($cellAddress !== $newReference) {
                     $worksheet->setBreak($newReference, $value)
                         ->setBreak($cellAddress, Worksheet::BREAK_NONE);
@@ -177,7 +177,7 @@ protected function adjustComments(Worksheet $worksheet): void
             // Any comments inside a deleted range will be ignored
             if ($this->cellReferenceHelper->cellAddressInDeleteRange($cellAddress) === false) {
                 // Otherwise build a new array of comments indexed by the adjusted cell reference
-                $newReference = $this->updateCellReference($cellAddress);
+                $newReference = $this->updateCellReference($cellAddress, false, null);
                 $aNewComments[$newReference] = $value;
             }
         }
@@ -200,12 +200,14 @@ protected function adjustHyperlinks(Worksheet $worksheet, int $numberOfColumns,
             : uksort($aHyperlinkCollection, [self::class, 'cellSort']);
 
         foreach ($aHyperlinkCollection as $cellAddress => $value) {
-            $newReference = $this->updateCellReference($cellAddress);
+            $newReference = $this->updateCellReference($cellAddress, false, null);
             if ($this->cellReferenceHelper->cellAddressInDeleteRange($cellAddress) === true) {
                 $worksheet->setHyperlink($cellAddress, null);
             } elseif ($cellAddress !== $newReference) {
-                $worksheet->setHyperlink($newReference, $value);
                 $worksheet->setHyperlink($cellAddress, null);
+                if ($newReference) {
+                    $worksheet->setHyperlink($newReference, $value);
+                }
             }
         }
     }
@@ -226,7 +228,7 @@ protected function adjustConditionalFormatting(Worksheet $worksheet, int $number
 
         foreach ($aStyles as $cellAddress => $cfRules) {
             $worksheet->removeConditionalStyles($cellAddress);
-            $newReference = $this->updateCellReference($cellAddress);
+            $newReference = $this->updateCellReference($cellAddress, false, null);
 
             foreach ($cfRules as &$cfRule) {
                 /** @var Conditional $cfRule */
@@ -264,11 +266,13 @@ protected function adjustDataValidations(Worksheet $worksheet, int $numberOfColu
             : uksort($aDataValidationCollection, [self::class, 'cellSort']);
 
         foreach ($aDataValidationCollection as $cellAddress => $dataValidation) {
-            $newReference = $this->updateCellReference($cellAddress);
+            $newReference = $this->updateCellReference($cellAddress, false, null);
             if ($cellAddress !== $newReference) {
                 $dataValidation->setSqref($newReference);
-                $worksheet->setDataValidation($newReference, $dataValidation);
                 $worksheet->setDataValidation($cellAddress, null);
+                if ($newReference) {
+                    $worksheet->setDataValidation($newReference, $dataValidation);
+                }
             }
         }
     }
@@ -283,8 +287,10 @@ protected function adjustMergeCells(Worksheet $worksheet): void
         $aMergeCells = $worksheet->getMergeCells();
         $aNewMergeCells = []; // the new array of all merge cells
         foreach ($aMergeCells as $cellAddress => &$value) {
-            $newReference = $this->updateCellReference($cellAddress);
-            $aNewMergeCells[$newReference] = $newReference;
+            $newReference = $this->updateCellReference($cellAddress, false, null);
+            if ($newReference) {
+                $aNewMergeCells[$newReference] = $newReference;
+            }
         }
         $worksheet->setMergeCells($aNewMergeCells); // replace the merge cells array
     }
@@ -303,10 +309,12 @@ protected function adjustProtectedCells(Worksheet $worksheet, int $numberOfColum
             ? uksort($aProtectedCells, [self::class, 'cellReverseSort'])
             : uksort($aProtectedCells, [self::class, 'cellSort']);
         foreach ($aProtectedCells as $cellAddress => $value) {
-            $newReference = $this->updateCellReference($cellAddress);
+            $newReference = $this->updateCellReference($cellAddress, false, null);
             if ($cellAddress !== $newReference) {
-                $worksheet->protectCells($newReference, $value, true);
                 $worksheet->unprotectCells($cellAddress);
+                if ($newReference) {
+                    $worksheet->protectCells($newReference, $value, true);
+                }
             }
         }
     }
@@ -321,7 +329,7 @@ protected function adjustColumnDimensions(Worksheet $worksheet): void
         $aColumnDimensions = array_reverse($worksheet->getColumnDimensions(), true);
         if (!empty($aColumnDimensions)) {
             foreach ($aColumnDimensions as $objColumnDimension) {
-                $newReference = $this->updateCellReference($objColumnDimension->getColumnIndex() . '1');
+                $newReference = $this->updateCellReference($objColumnDimension->getColumnIndex() . '1', false, null);
                 [$newReference] = Coordinate::coordinateFromString($newReference);
                 if ($objColumnDimension->getColumnIndex() !== $newReference) {
                     $objColumnDimension->setColumnIndex($newReference);
@@ -344,7 +352,7 @@ protected function adjustRowDimensions(Worksheet $worksheet, $beforeRow, $number
         $aRowDimensions = array_reverse($worksheet->getRowDimensions(), true);
         if (!empty($aRowDimensions)) {
             foreach ($aRowDimensions as $objRowDimension) {
-                $newReference = $this->updateCellReference('A' . $objRowDimension->getRowIndex());
+                $newReference = $this->updateCellReference('A' . $objRowDimension->getRowIndex(), false, null);
                 [, $newReference] = Coordinate::coordinateFromString($newReference);
                 $newRoweference = (int) $newReference;
                 if ($objRowDimension->getRowIndex() !== $newRoweference) {
@@ -434,7 +442,8 @@ function ($coordinate) use ($cellCollection) {
             $cell = $worksheet->getCell($coordinate);
             $cellIndex = Coordinate::columnIndexFromString($cell->getColumn());
 
-            if ($cellIndex - 1 + $numberOfColumns < 0) {
+            // Don't update cells that are being removed
+            if ($numberOfColumns < 0 && $cellIndex >= $beforeColumn + $numberOfColumns && $cellIndex < $beforeColumn) {
                 continue;
             }
 
@@ -518,28 +527,28 @@ function ($coordinate) use ($cellCollection) {
             $splitCell = $worksheet->getFreezePane();
             $topLeftCell = $worksheet->getTopLeftCell() ?? '';
 
-            $splitCell = $this->updateCellReference($splitCell);
-            $topLeftCell = $this->updateCellReference($topLeftCell);
+            $splitCell = $this->updateCellReference($splitCell, false, null);
+            $topLeftCell = $this->updateCellReference($topLeftCell, false, null);
 
             $worksheet->freezePane($splitCell, $topLeftCell);
         }
 
         // Page setup
         if ($worksheet->getPageSetup()->isPrintAreaSet()) {
             $worksheet->getPageSetup()->setPrintArea(
-                $this->updateCellReference($worksheet->getPageSetup()->getPrintArea())
+                $this->updateCellReference($worksheet->getPageSetup()->getPrintArea(), false, null)
             );
         }
 
         // Update worksheet: drawings
         $aDrawings = $worksheet->getDrawingCollection();
         foreach ($aDrawings as $objDrawing) {
-            $newReference = $this->updateCellReference($objDrawing->getCoordinates());
+            $newReference = $this->updateCellReference($objDrawing->getCoordinates(), false, null);
             if ($objDrawing->getCoordinates() != $newReference) {
                 $objDrawing->setCoordinates($newReference);
             }
             if ($objDrawing->getCoordinates2() !== '') {
-                $newReference = $this->updateCellReference($objDrawing->getCoordinates2());
+                $newReference = $this->updateCellReference($objDrawing->getCoordinates2(), false, null);
                 if ($objDrawing->getCoordinates2() != $newReference) {
                     $objDrawing->setCoordinates2($newReference);
                 }
@@ -596,8 +605,8 @@ public function updateFormulaReferences(
                     foreach ($matches as $match) {
                         $fromString = ($match[2] > '') ? $match[2] . '!' : '';
                         $fromString .= $match[3] . ':' . $match[4];
-                        $modified3 = substr($this->updateCellReference('$A' . $match[3], $includeAbsoluteReferences), 2);
-                        $modified4 = substr($this->updateCellReference('$A' . $match[4], $includeAbsoluteReferences), 2);
+                        $modified3 = substr($this->updateCellReference('$A' . $match[3], $includeAbsoluteReferences, true), 2);
+                        $modified4 = substr($this->updateCellReference('$A' . $match[4], $includeAbsoluteReferences, false), 2);
 
                         if ($match[3] . ':' . $match[4] !== $modified3 . ':' . $modified4) {
                             if (($match[2] == '') || (trim($match[2], "'") == $worksheetName)) {
@@ -621,8 +630,8 @@ public function updateFormulaReferences(
                     foreach ($matches as $match) {
                         $fromString = ($match[2] > '') ? $match[2] . '!' : '';
                         $fromString .= $match[3] . ':' . $match[4];
-                        $modified3 = substr($this->updateCellReference($match[3] . '$1', $includeAbsoluteReferences), 0, -2);
-                        $modified4 = substr($this->updateCellReference($match[4] . '$1', $includeAbsoluteReferences), 0, -2);
+                        $modified3 = substr($this->updateCellReference($match[3] . '$1', $includeAbsoluteReferences, true), 0, -2);
+                        $modified4 = substr($this->updateCellReference($match[4] . '$1', $includeAbsoluteReferences, false), 0, -2);
 
                         if ($match[3] . ':' . $match[4] !== $modified3 . ':' . $modified4) {
                             if (($match[2] == '') || (trim($match[2], "'") == $worksheetName)) {
@@ -646,8 +655,8 @@ public function updateFormulaReferences(
                     foreach ($matches as $match) {
                         $fromString = ($match[2] > '') ? $match[2] . '!' : '';
                         $fromString .= $match[3] . ':' . $match[4];
-                        $modified3 = $this->updateCellReference($match[3], $includeAbsoluteReferences);
-                        $modified4 = $this->updateCellReference($match[4], $includeAbsoluteReferences);
+                        $modified3 = $this->updateCellReference($match[3], $includeAbsoluteReferences, true);
+                        $modified4 = $this->updateCellReference($match[4], $includeAbsoluteReferences, false);
 
                         if ($match[3] . $match[4] !== $modified3 . $modified4) {
                             if (($match[2] == '') || (trim($match[2], "'") == $worksheetName)) {
@@ -674,7 +683,7 @@ public function updateFormulaReferences(
                         $fromString = ($match[2] > '') ? $match[2] . '!' : '';
                         $fromString .= $match[3];
 
-                        $modified3 = $this->updateCellReference($match[3], $includeAbsoluteReferences);
+                        $modified3 = $this->updateCellReference($match[3], $includeAbsoluteReferences, null);
                         if ($match[3] !== $modified3) {
                             if (($match[2] == '') || (trim($match[2], "'") == $worksheetName)) {
                                 $toString = ($match[2] > '') ? $match[2] . '!' : '';
@@ -854,7 +863,7 @@ private function updateRowRangesAllWorksheets(string $formula, int $numberOfRows
      *
      * @return string Updated cell range
      */
-    private function updateCellReference($cellReference = 'A1', bool $includeAbsoluteReferences = false)
+    private function updateCellReference($cellReference = 'A1', bool $includeAbsoluteReferences = false, ?bool $topLeft = null)
     {
         // Is it in another worksheet? Will not have to update anything.
         if (strpos($cellReference, '!') !== false) {
@@ -863,7 +872,7 @@ private function updateCellReference($cellReference = 'A1', bool $includeAbsolut
         // Is it a range or a single cell?
         if (!Coordinate::coordinateIsRange($cellReference)) {
             // Single cell
-            return $this->cellReferenceHelper->updateCellReference($cellReference, $includeAbsoluteReferences);
+            return $this->cellReferenceHelper->updateCellReference($cellReference, $includeAbsoluteReferences, $topLeft);
         }
 
         // Range
@@ -925,7 +934,7 @@ private function updateNamedRange(DefinedName $definedName, Worksheet $worksheet
                 $formula = $this->updateFormulaReferences($cellAddress, $beforeCellAddress, $numberOfColumns, $numberOfRows, $worksheet->getTitle(), true);
                 $definedName->setValue($formula);
             } else {
-                $definedName->setValue($this->updateCellReference(ltrim($cellAddress, '='), true));
+                $definedName->setValue($this->updateCellReference(ltrim($cellAddress, '='), true, null));
             }
         }
     }
@@ -967,14 +976,14 @@ private function updateCellRange(string $cellRange = 'A1:A1', bool $includeAbsol
             for ($j = 0; $j < $jc; ++$j) {
                 if (ctype_alpha($range[$i][$j])) {
                     $range[$i][$j] = Coordinate::coordinateFromString(
-                        $this->cellReferenceHelper->updateCellReference($range[$i][$j] . '1', $includeAbsoluteReferences)
+                        $this->cellReferenceHelper->updateCellReference($range[$i][$j] . '1', $includeAbsoluteReferences, null)
                     )[0];
                 } elseif (ctype_digit($range[$i][$j])) {
                     $range[$i][$j] = Coordinate::coordinateFromString(
-                        $this->cellReferenceHelper->updateCellReference('A' . $range[$i][$j], $includeAbsoluteReferences)
+                        $this->cellReferenceHelper->updateCellReference('A' . $range[$i][$j], $includeAbsoluteReferences, null)
                     )[1];
                 } else {
-                    $range[$i][$j] = $this->cellReferenceHelper->updateCellReference($range[$i][$j], $includeAbsoluteReferences);
+                    $range[$i][$j] = $this->cellReferenceHelper->updateCellReference($range[$i][$j], $includeAbsoluteReferences, null);
                 }
             }
         }
@@ -1052,7 +1061,7 @@ private function adjustAutoFilter(Worksheet $worksheet, string $beforeCellAddres
             }
 
             $worksheet->setAutoFilter(
-                $this->updateCellReference($autoFilterRange)
+                $this->updateCellReference($autoFilterRange, false, null)
             );
         }
     }
@@ -1131,7 +1140,7 @@ private function adjustTable(Worksheet $worksheet, string $beforeCellAddress, in
                     }
                 }
 
-                $table->setRange($this->updateCellReference($tableRange));
+                $table->setRange($this->updateCellReference($tableRange, false, null));
             }
         }
     }
diff --git a/tests/PhpSpreadsheetTests/Issue1449Test.php b/tests/PhpSpreadsheetTests/Issue1449Test.php
