Fix cell ranges causing coordinate merge error

Fixes #319
Closes #328

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Freeze Panes takes wrong coordinates for XLSX - [#322](https://github.com/PHPOffice/PhpSpreadsheet/issues/322)
 - `COLUMNS` and `ROWS` functions crashed in some cases - [#336](https://github.com/PHPOffice/PhpSpreadsheet/issues/336)
 - Support XML file without styles - [#331](https://github.com/PHPOffice/PhpSpreadsheet/pull/331)
+- Cell coordinates which are already a range cause an exception [#319](https://github.com/PHPOffice/PhpSpreadsheet/issues/319)
 
 ## [1.0.0] - 2017-12-25
 

src/PhpSpreadsheet/Cell/Coordinate.php

@@ -32,7 +32,7 @@ abstract class Coordinate
     {
         if (preg_match("/^([$]?[A-Z]{1,3})([$]?\d{1,7})$/", $pCoordinateString, $matches)) {
             return [$matches[1], $matches[2]];
-        } elseif ((strpos($pCoordinateString, ':') !== false) || (strpos($pCoordinateString, ',') !== false)) {
+        } elseif (self::coordinateIsRange($pCoordinateString)) {
             throw new Exception('Cell coordinate string can not be a range of cells');
         } elseif ($pCoordinateString == '') {
             throw new Exception('Cell coordinate can not be zero-length string');
@@ -41,6 +41,18 @@ abstract class Coordinate
         throw new Exception('Invalid cell coordinate ' . $pCoordinateString);
     }
 
+    /**
+     * Checks if a coordinate represents a range of cells.
+     *
+     * @param string $coord eg: 'A1' or 'A1:A2' or 'A1:A2,C1:C2'
+     *
+     * @return bool Whether the coordinate represents a range of cells
+     */
+    public static function coordinateIsRange($coord)
+    {
+        return (strpos($coord, ':') !== false) || (strpos($coord, ',') !== false);
+    }
+
     /**
      * Make string row, column or cell coordinate absolute.
      *
@@ -53,28 +65,28 @@ abstract class Coordinate
      */
     public static function absoluteReference($pCoordinateString)
     {
-        if (strpos($pCoordinateString, ':') === false && strpos($pCoordinateString, ',') === false) {
+        if (self::coordinateIsRange($pCoordinateString)) {
-            // Split out any worksheet name from the reference
+            throw new Exception('Cell coordinate string can not be a range of cells');
-            $worksheet = '';
-            $cellAddress = explode('!', $pCoordinateString);
-            if (count($cellAddress) > 1) {
-                list($worksheet, $pCoordinateString) = $cellAddress;
-            }
-            if ($worksheet > '') {
-                $worksheet .= '!';
-            }
-
-            // Create absolute coordinate
-            if (ctype_digit($pCoordinateString)) {
-                return $worksheet . '$' . $pCoordinateString;
-            } elseif (ctype_alpha($pCoordinateString)) {
-                return $worksheet . '$' . strtoupper($pCoordinateString);
-            }
-
-            return $worksheet . self::absoluteCoordinate($pCoordinateString);
         }
 
-        throw new Exception('Cell coordinate string can not be a range of cells');
+        // Split out any worksheet name from the reference
+        $worksheet = '';
+        $cellAddress = explode('!', $pCoordinateString);
+        if (count($cellAddress) > 1) {
+            list($worksheet, $pCoordinateString) = $cellAddress;
+        }
+        if ($worksheet > '') {
+            $worksheet .= '!';
+        }
+
+        // Create absolute coordinate
+        if (ctype_digit($pCoordinateString)) {
+            return $worksheet . '$' . $pCoordinateString;
+        } elseif (ctype_alpha($pCoordinateString)) {
+            return $worksheet . '$' . strtoupper($pCoordinateString);
+        }
+
+        return $worksheet . self::absoluteCoordinate($pCoordinateString);
     }
 
     /**
@@ -88,26 +100,26 @@ abstract class Coordinate
      */
     public static function absoluteCoordinate($pCoordinateString)
     {
-        if (strpos($pCoordinateString, ':') === false && strpos($pCoordinateString, ',') === false) {
+        if (self::coordinateIsRange($pCoordinateString)) {
-            // Split out any worksheet name from the coordinate
+            throw new Exception('Cell coordinate string can not be a range of cells');
-            $worksheet = '';
-            $cellAddress = explode('!', $pCoordinateString);
-            if (count($cellAddress) > 1) {
-                list($worksheet, $pCoordinateString) = $cellAddress;
-            }
-            if ($worksheet > '') {
-                $worksheet .= '!';
-            }
-
-            // Create absolute coordinate
-            list($column, $row) = self::coordinateFromString($pCoordinateString);
-            $column = ltrim($column, '$');
-            $row = ltrim($row, '$');
-
-            return $worksheet . '$' . $column . '$' . $row;
         }
 
-        throw new Exception('Cell coordinate string can not be a range of cells');
+        // Split out any worksheet name from the coordinate
+        $worksheet = '';
+        $cellAddress = explode('!', $pCoordinateString);
+        if (count($cellAddress) > 1) {
+            list($worksheet, $pCoordinateString) = $cellAddress;
+        }
+        if ($worksheet > '') {
+            $worksheet .= '!';
+        }
+
+        // Create absolute coordinate
+        list($column, $row) = self::coordinateFromString($pCoordinateString);
+        $column = ltrim($column, '$');
+        $row = ltrim($row, '$');
+
+        return $worksheet . '$' . $column . '$' . $row;
     }
 
     /**
@@ -330,7 +342,7 @@ abstract class Coordinate
         $cellBlocks = explode(' ', str_replace('$', '', strtoupper($pRange)));
         foreach ($cellBlocks as $cellBlock) {
             // Single cell?
-            if (strpos($cellBlock, ':') === false && strpos($cellBlock, ',') === false) {
+            if (!self::coordinateIsRange($cellBlock)) {
                 $returnValue[] = $cellBlock;
 
                 continue;
@@ -400,8 +412,15 @@ abstract class Coordinate
     public static function mergeRangesInCollection(array $pCoordCollection)
     {
         $hashedValues = [];
+        $mergedCoordCollection = [];
 
         foreach ($pCoordCollection as $coord => $value) {
+            if (self::coordinateIsRange($coord)) {
+                $mergedCoordCollection[$coord] = $value;
+
+                continue;
+            }
+
             list($column, $row) = self::coordinateFromString($coord);
             $row = (int) (ltrim($row, '$'));
             $hashCode = $column . '-' . (is_object($value) ? $value->getHashCode() : $value);
@@ -417,7 +436,6 @@ abstract class Coordinate
             }
         }
 
-        $mergedCoordCollection = [];
         ksort($hashedValues);
 
         foreach ($hashedValues as $hashedValue) {

src/PhpSpreadsheet/ReferenceHelper.php

@@ -432,7 +432,7 @@ class ReferenceHelper
                 if ($cell->getDataType() == DataType::TYPE_FORMULA) {
                     // Formula should be adjusted
                     $pSheet->getCell($newCoordinate)
-                            ->setValue($this->updateFormulaReferences($cell->getValue(), $pBefore, $pNumCols, $pNumRows, $pSheet->getTitle()));
+                        ->setValue($this->updateFormulaReferences($cell->getValue(), $pBefore, $pNumCols, $pNumRows, $pSheet->getTitle()));
                 } else {
                     // Formula should not be adjusted
                     $pSheet->getCell($newCoordinate)->setValue($cell->getValue());
@@ -760,7 +760,7 @@ class ReferenceHelper
      * Update cell reference.
      *
      * @param string $pCellRange Cell range
-     * @param int $pBefore Insert before this one
+     * @param string $pBefore Insert before this one
      * @param int $pNumCols Number of columns to increment
      * @param int $pNumRows Number of rows to increment
      *
@@ -774,13 +774,14 @@ class ReferenceHelper
         if (strpos($pCellRange, '!') !== false) {
             return $pCellRange;
             // Is it a range or a single cell?
-        } elseif (strpos($pCellRange, ':') === false && strpos($pCellRange, ',') === false) {
+        } elseif (!Coordinate::coordinateIsRange($pCellRange)) {
             // Single cell
             return $this->updateSingleCellReference($pCellRange, $pBefore, $pNumCols, $pNumRows);
-        } elseif (strpos($pCellRange, ':') !== false || strpos($pCellRange, ',') !== false) {
+        } elseif (Coordinate::coordinateIsRange($pCellRange)) {
             // Range
             return $this->updateCellRange($pCellRange, $pBefore, $pNumCols, $pNumRows);
         }
+
         // Return original
         return $pCellRange;
     }
@@ -817,7 +818,7 @@ class ReferenceHelper
      * Update cell range.
      *
      * @param string $pCellRange Cell range    (e.g. 'B2:D4', 'B:C' or '2:3')
-     * @param int $pBefore Insert before this one
+     * @param string $pBefore Insert before this one
      * @param int $pNumCols Number of columns to increment
      * @param int $pNumRows Number of rows to increment
      *
@@ -827,37 +828,37 @@ class ReferenceHelper
      */
     private function updateCellRange($pCellRange = 'A1:A1', $pBefore = 'A1', $pNumCols = 0, $pNumRows = 0)
     {
-        if (strpos($pCellRange, ':') !== false || strpos($pCellRange, ',') !== false) {
+        if (!Coordinate::coordinateIsRange($pCellRange)) {
-            // Update range
+            throw new Exception('Only cell ranges may be passed to this method.');
-            $range = Coordinate::splitRange($pCellRange);
-            $ic = count($range);
-            for ($i = 0; $i < $ic; ++$i) {
-                $jc = count($range[$i]);
-                for ($j = 0; $j < $jc; ++$j) {
-                    if (ctype_alpha($range[$i][$j])) {
-                        $r = Coordinate::coordinateFromString($this->updateSingleCellReference($range[$i][$j] . '1', $pBefore, $pNumCols, $pNumRows));
-                        $range[$i][$j] = $r[0];
-                    } elseif (ctype_digit($range[$i][$j])) {
-                        $r = Coordinate::coordinateFromString($this->updateSingleCellReference('A' . $range[$i][$j], $pBefore, $pNumCols, $pNumRows));
-                        $range[$i][$j] = $r[1];
-                    } else {
-                        $range[$i][$j] = $this->updateSingleCellReference($range[$i][$j], $pBefore, $pNumCols, $pNumRows);
-                    }
-                }
-            }
-
-            // Recreate range string
-            return Coordinate::buildRange($range);
         }
 
-        throw new Exception('Only cell ranges may be passed to this method.');
+        // Update range
+        $range = Coordinate::splitRange($pCellRange);
+        $ic = count($range);
+        for ($i = 0; $i < $ic; ++$i) {
+            $jc = count($range[$i]);
+            for ($j = 0; $j < $jc; ++$j) {
+                if (ctype_alpha($range[$i][$j])) {
+                    $r = Coordinate::coordinateFromString($this->updateSingleCellReference($range[$i][$j] . '1', $pBefore, $pNumCols, $pNumRows));
+                    $range[$i][$j] = $r[0];
+                } elseif (ctype_digit($range[$i][$j])) {
+                    $r = Coordinate::coordinateFromString($this->updateSingleCellReference('A' . $range[$i][$j], $pBefore, $pNumCols, $pNumRows));
+                    $range[$i][$j] = $r[1];
+                } else {
+                    $range[$i][$j] = $this->updateSingleCellReference($range[$i][$j], $pBefore, $pNumCols, $pNumRows);
+                }
+            }
+        }
+
+        // Recreate range string
+        return Coordinate::buildRange($range);
     }
 
     /**
      * Update single cell reference.
      *
      * @param string $pCellReference Single cell reference
-     * @param int $pBefore Insert before this one
+     * @param string $pBefore Insert before this one
      * @param int $pNumCols Number of columns to increment
      * @param int $pNumRows Number of rows to increment
      *
@@ -867,32 +868,32 @@ class ReferenceHelper
      */
     private function updateSingleCellReference($pCellReference = 'A1', $pBefore = 'A1', $pNumCols = 0, $pNumRows = 0)
     {
-        if (strpos($pCellReference, ':') === false && strpos($pCellReference, ',') === false) {
+        if (Coordinate::coordinateIsRange($pCellReference)) {
-            // Get coordinate of $pBefore
+            throw new Exception('Only single cell references may be passed to this method.');
-            list($beforeColumn, $beforeRow) = Coordinate::coordinateFromString($pBefore);
-
-            // Get coordinate of $pCellReference
-            list($newColumn, $newRow) = Coordinate::coordinateFromString($pCellReference);
-
-            // Verify which parts should be updated
-            $updateColumn = (($newColumn[0] != '$') && ($beforeColumn[0] != '$') && (Coordinate::columnIndexFromString($newColumn) >= Coordinate::columnIndexFromString($beforeColumn)));
-            $updateRow = (($newRow[0] != '$') && ($beforeRow[0] != '$') && $newRow >= $beforeRow);
-
-            // Create new column reference
-            if ($updateColumn) {
-                $newColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($newColumn) + $pNumCols);
-            }
-
-            // Create new row reference
-            if ($updateRow) {
-                $newRow = $newRow + $pNumRows;
-            }
-
-            // Return new reference
-            return $newColumn . $newRow;
         }
 
-        throw new Exception('Only single cell references may be passed to this method.');
+        // Get coordinate of $pBefore
+        list($beforeColumn, $beforeRow) = Coordinate::coordinateFromString($pBefore);
+
+        // Get coordinate of $pCellReference
+        list($newColumn, $newRow) = Coordinate::coordinateFromString($pCellReference);
+
+        // Verify which parts should be updated
+        $updateColumn = (($newColumn[0] != '$') && ($beforeColumn[0] != '$') && (Coordinate::columnIndexFromString($newColumn) >= Coordinate::columnIndexFromString($beforeColumn)));
+        $updateRow = (($newRow[0] != '$') && ($beforeRow[0] != '$') && $newRow >= $beforeRow);
+
+        // Create new column reference
+        if ($updateColumn) {
+            $newColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($newColumn) + $pNumCols);
+        }
+
+        // Create new row reference
+        if ($updateRow) {
+            $newRow = $newRow + $pNumRows;
+        }
+
+        // Return new reference
+        return $newColumn . $newRow;
     }
 
     /**

src/PhpSpreadsheet/Worksheet/Worksheet.php

@@ -1216,7 +1216,7 @@ class Worksheet implements IComparable
         // Uppercase coordinate
         $pCoordinate = strtoupper($pCoordinate);
 
-        if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
+        if (Coordinate::coordinateIsRange($pCoordinate)) {
             throw new Exception('Cell coordinate can not be a range of cells.');
         } elseif (strpos($pCoordinate, '$') !== false) {
             throw new Exception('Cell coordinate must not be absolute.');
@@ -1324,7 +1324,7 @@ class Worksheet implements IComparable
         // Uppercase coordinate
         $pCoordinate = strtoupper($pCoordinate);
 
-        if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
+        if (Coordinate::coordinateIsRange($pCoordinate)) {
             throw new Exception('Cell coordinate can not be a range of cells.');
         } elseif (strpos($pCoordinate, '$') !== false) {
             throw new Exception('Cell coordinate must not be absolute.');
@@ -1993,7 +1993,7 @@ class Worksheet implements IComparable
      */
     public function freezePane($cell, $topLeftCell = null)
     {
-        if (is_string($cell) && (strpos($cell, ':') !== false || strpos($cell, ',') !== false)) {
+        if (is_string($cell) && Coordinate::coordinateIsRange($cell)) {
             throw new Exception('Freeze pane can not be set on a range of cells.');
         }
 
@@ -2337,7 +2337,7 @@ class Worksheet implements IComparable
         // Uppercase coordinate
         $pCellCoordinate = strtoupper($pCellCoordinate);
 
-        if (strpos($pCellCoordinate, ':') !== false || strpos($pCellCoordinate, ',') !== false) {
+        if (Coordinate::coordinateIsRange($pCellCoordinate)) {
             throw new Exception('Cell coordinate string can not be a range of cells.');
         } elseif (strpos($pCellCoordinate, '$') !== false) {
             throw new Exception('Cell coordinate string must not be absolute.');
@@ -2428,7 +2428,7 @@ class Worksheet implements IComparable
         // Convert '1:3' to 'A1:XFD3'
         $pCoordinate = preg_replace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate);
 
-        if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
+        if (Coordinate::coordinateIsRange($pCoordinate)) {
             list($first) = Coordinate::splitRange($pCoordinate);
             $this->activeCell = $first[0];
         } else {

tests/PhpSpreadsheetTests/Cell/CoordinateTest.php

@@ -331,4 +331,20 @@ class CoordinateTest extends TestCase
     {
         return require 'data/CellMergeRangesInCollection.php';
     }
+
+    /**
+     * @dataProvider providerCoordinateIsRange
+     *
+     * @param mixed $expectedResult
+     */
+    public function testCoordinateIsRange($expectedResult, ...$args)
+    {
+        $result = Coordinate::coordinateIsRange(...$args);
+        self::assertEquals($expectedResult, $result);
+    }
+
+    public function providerCoordinateIsRange()
+    {
+        return require 'data/CoordinateIsRange.php';
+    }
 }

tests/data/CellMergeRangesInCollection.php

@@ -55,4 +55,16 @@ return [
             'A3' => 'z',
         ],
     ],
+    [
+        [
+            'C1' => 'x',
+            'A1:A3' => 'x',
+            'A1:A3,C1:C3' => 'y',
+        ],
+        [
+            'C1' => 'x',
+            'A1:A3' => 'x',
+            'A1:A3,C1:C3' => 'y',
+        ],
+    ],
 ];

tests/data/CoordinateIsRange.php

@@ -0,0 +1,24 @@
+<?php
+
+return [
+    [
+        false,
+        'A1',
+    ],
+    [
+        false,
+        '$A$1',
+    ],
+    [
+        true,
+        'A1,C3',
+    ],
+    [
+        true,
+        'A1:A10',
+    ],
+    [
+        true,
+        'A1:A10,C4',
+    ],
+];