Skip to content

Commit 49775bd

Browse files
MaxTinglePowerKiKi
MaxTingle
authored and
PowerKiKi
committed
Fix cell ranges causing coordinate merge error
Fixes #319 Closes #328
1 parent 4e0344c commit 49775bd

File tree

7 files changed

+160
-88
lines changed

7 files changed

+160
-88
lines changed

‎CHANGELOG.md

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

2425
## [1.0.0] - 2017-12-25
2526

‎src/PhpSpreadsheet/Cell/Coordinate.php

+56-38
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public static function coordinateFromString($pCoordinateString)
3232
{
3333
if (preg_match("/^([$]?[A-Z]{1,3})([$]?\d{1,7})$/", $pCoordinateString, $matches)) {
3434
return [$matches[1], $matches[2]];
35-
} elseif ((strpos($pCoordinateString, ':') !== false) || (strpos($pCoordinateString, ',') !== false)) {
35+
} elseif (self::coordinateIsRange($pCoordinateString)) {
3636
throw new Exception('Cell coordinate string can not be a range of cells');
3737
} elseif ($pCoordinateString == '') {
3838
throw new Exception('Cell coordinate can not be zero-length string');
@@ -41,6 +41,18 @@ public static function coordinateFromString($pCoordinateString)
4141
throw new Exception('Invalid cell coordinate ' . $pCoordinateString);
4242
}
4343

44+
/**
45+
* Checks if a coordinate represents a range of cells.
46+
*
47+
* @param string $coord eg: 'A1' or 'A1:A2' or 'A1:A2,C1:C2'
48+
*
49+
* @return bool Whether the coordinate represents a range of cells
50+
*/
51+
public static function coordinateIsRange($coord)
52+
{
53+
return (strpos($coord, ':') !== false) || (strpos($coord, ',') !== false);
54+
}
55+
4456
/**
4557
* Make string row, column or cell coordinate absolute.
4658
*
@@ -53,28 +65,28 @@ public static function coordinateFromString($pCoordinateString)
5365
*/
5466
public static function absoluteReference($pCoordinateString)
5567
{
56-
if (strpos($pCoordinateString, ':') === false && strpos($pCoordinateString, ',') === false) {
57-
// Split out any worksheet name from the reference
58-
$worksheet = '';
59-
$cellAddress = explode('!', $pCoordinateString);
60-
if (count($cellAddress) > 1) {
61-
list($worksheet, $pCoordinateString) = $cellAddress;
62-
}
63-
if ($worksheet > '') {
64-
$worksheet .= '!';
65-
}
68+
if (self::coordinateIsRange($pCoordinateString)) {
69+
throw new Exception('Cell coordinate string can not be a range of cells');
70+
}
6671

67-
// Create absolute coordinate
68-
if (ctype_digit($pCoordinateString)) {
69-
return $worksheet . '$' . $pCoordinateString;
70-
} elseif (ctype_alpha($pCoordinateString)) {
71-
return $worksheet . '$' . strtoupper($pCoordinateString);
72-
}
72+
// Split out any worksheet name from the reference
73+
$worksheet = '';
74+
$cellAddress = explode('!', $pCoordinateString);
75+
if (count($cellAddress) > 1) {
76+
list($worksheet, $pCoordinateString) = $cellAddress;
77+
}
78+
if ($worksheet > '') {
79+
$worksheet .= '!';
80+
}
7381

74-
return $worksheet . self::absoluteCoordinate($pCoordinateString);
82+
// Create absolute coordinate
83+
if (ctype_digit($pCoordinateString)) {
84+
return $worksheet . '$' . $pCoordinateString;
85+
} elseif (ctype_alpha($pCoordinateString)) {
86+
return $worksheet . '$' . strtoupper($pCoordinateString);
7587
}
7688

77-
throw new Exception('Cell coordinate string can not be a range of cells');
89+
return $worksheet . self::absoluteCoordinate($pCoordinateString);
7890
}
7991

8092
/**
@@ -88,26 +100,26 @@ public static function absoluteReference($pCoordinateString)
88100
*/
89101
public static function absoluteCoordinate($pCoordinateString)
90102
{
91-
if (strpos($pCoordinateString, ':') === false && strpos($pCoordinateString, ',') === false) {
92-
// Split out any worksheet name from the coordinate
93-
$worksheet = '';
94-
$cellAddress = explode('!', $pCoordinateString);
95-
if (count($cellAddress) > 1) {
96-
list($worksheet, $pCoordinateString) = $cellAddress;
97-
}
98-
if ($worksheet > '') {
99-
$worksheet .= '!';
100-
}
101-
102-
// Create absolute coordinate
103-
list($column, $row) = self::coordinateFromString($pCoordinateString);
104-
$column = ltrim($column, '$');
105-
$row = ltrim($row, '$');
103+
if (self::coordinateIsRange($pCoordinateString)) {
104+
throw new Exception('Cell coordinate string can not be a range of cells');
105+
}
106106

107-
return $worksheet . '$' . $column . '$' . $row;
107+
// Split out any worksheet name from the coordinate
108+
$worksheet = '';
109+
$cellAddress = explode('!', $pCoordinateString);
110+
if (count($cellAddress) > 1) {
111+
list($worksheet, $pCoordinateString) = $cellAddress;
112+
}
113+
if ($worksheet > '') {
114+
$worksheet .= '!';
108115
}
109116

110-
throw new Exception('Cell coordinate string can not be a range of cells');
117+
// Create absolute coordinate
118+
list($column, $row) = self::coordinateFromString($pCoordinateString);
119+
$column = ltrim($column, '$');
120+
$row = ltrim($row, '$');
121+
122+
return $worksheet . '$' . $column . '$' . $row;
111123
}
112124

113125
/**
@@ -330,7 +342,7 @@ public static function extractAllCellReferencesInRange($pRange)
330342
$cellBlocks = explode(' ', str_replace('$', '', strtoupper($pRange)));
331343
foreach ($cellBlocks as $cellBlock) {
332344
// Single cell?
333-
if (strpos($cellBlock, ':') === false && strpos($cellBlock, ',') === false) {
345+
if (!self::coordinateIsRange($cellBlock)) {
334346
$returnValue[] = $cellBlock;
335347

336348
continue;
@@ -400,8 +412,15 @@ public static function extractAllCellReferencesInRange($pRange)
400412
public static function mergeRangesInCollection(array $pCoordCollection)
401413
{
402414
$hashedValues = [];
415+
$mergedCoordCollection = [];
403416

404417
foreach ($pCoordCollection as $coord => $value) {
418+
if (self::coordinateIsRange($coord)) {
419+
$mergedCoordCollection[$coord] = $value;
420+
421+
continue;
422+
}
423+
405424
list($column, $row) = self::coordinateFromString($coord);
406425
$row = (int) (ltrim($row, '$'));
407426
$hashCode = $column . '-' . (is_object($value) ? $value->getHashCode() : $value);
@@ -417,7 +436,6 @@ public static function mergeRangesInCollection(array $pCoordCollection)
417436
}
418437
}
419438

420-
$mergedCoordCollection = [];
421439
ksort($hashedValues);
422440

423441
foreach ($hashedValues as $hashedValue) {

‎src/PhpSpreadsheet/ReferenceHelper.php

+46-45
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ public function insertNewBefore($pBefore, $pNumCols, $pNumRows, Worksheet $pShee
432432
if ($cell->getDataType() == DataType::TYPE_FORMULA) {
433433
// Formula should be adjusted
434434
$pSheet->getCell($newCoordinate)
435-
->setValue($this->updateFormulaReferences($cell->getValue(), $pBefore, $pNumCols, $pNumRows, $pSheet->getTitle()));
435+
->setValue($this->updateFormulaReferences($cell->getValue(), $pBefore, $pNumCols, $pNumRows, $pSheet->getTitle()));
436436
} else {
437437
// Formula should not be adjusted
438438
$pSheet->getCell($newCoordinate)->setValue($cell->getValue());
@@ -760,7 +760,7 @@ public function updateFormulaReferences($pFormula = '', $pBefore = 'A1', $pNumCo
760760
* Update cell reference.
761761
*
762762
* @param string $pCellRange Cell range
763-
* @param int $pBefore Insert before this one
763+
* @param string $pBefore Insert before this one
764764
* @param int $pNumCols Number of columns to increment
765765
* @param int $pNumRows Number of rows to increment
766766
*
@@ -774,13 +774,14 @@ public function updateCellReference($pCellRange = 'A1', $pBefore = 'A1', $pNumCo
774774
if (strpos($pCellRange, '!') !== false) {
775775
return $pCellRange;
776776
// Is it a range or a single cell?
777-
} elseif (strpos($pCellRange, ':') === false && strpos($pCellRange, ',') === false) {
777+
} elseif (!Coordinate::coordinateIsRange($pCellRange)) {
778778
// Single cell
779779
return $this->updateSingleCellReference($pCellRange, $pBefore, $pNumCols, $pNumRows);
780-
} elseif (strpos($pCellRange, ':') !== false || strpos($pCellRange, ',') !== false) {
780+
} elseif (Coordinate::coordinateIsRange($pCellRange)) {
781781
// Range
782782
return $this->updateCellRange($pCellRange, $pBefore, $pNumCols, $pNumRows);
783783
}
784+
784785
// Return original
785786
return $pCellRange;
786787
}
@@ -817,7 +818,7 @@ public function updateNamedFormulas(Spreadsheet $spreadsheet, $oldName = '', $ne
817818
* Update cell range.
818819
*
819820
* @param string $pCellRange Cell range (e.g. 'B2:D4', 'B:C' or '2:3')
820-
* @param int $pBefore Insert before this one
821+
* @param string $pBefore Insert before this one
821822
* @param int $pNumCols Number of columns to increment
822823
* @param int $pNumRows Number of rows to increment
823824
*
@@ -827,37 +828,37 @@ public function updateNamedFormulas(Spreadsheet $spreadsheet, $oldName = '', $ne
827828
*/
828829
private function updateCellRange($pCellRange = 'A1:A1', $pBefore = 'A1', $pNumCols = 0, $pNumRows = 0)
829830
{
830-
if (strpos($pCellRange, ':') !== false || strpos($pCellRange, ',') !== false) {
831-
// Update range
832-
$range = Coordinate::splitRange($pCellRange);
833-
$ic = count($range);
834-
for ($i = 0; $i < $ic; ++$i) {
835-
$jc = count($range[$i]);
836-
for ($j = 0; $j < $jc; ++$j) {
837-
if (ctype_alpha($range[$i][$j])) {
838-
$r = Coordinate::coordinateFromString($this->updateSingleCellReference($range[$i][$j] . '1', $pBefore, $pNumCols, $pNumRows));
839-
$range[$i][$j] = $r[0];
840-
} elseif (ctype_digit($range[$i][$j])) {
841-
$r = Coordinate::coordinateFromString($this->updateSingleCellReference('A' . $range[$i][$j], $pBefore, $pNumCols, $pNumRows));
842-
$range[$i][$j] = $r[1];
843-
} else {
844-
$range[$i][$j] = $this->updateSingleCellReference($range[$i][$j], $pBefore, $pNumCols, $pNumRows);
845-
}
831+
if (!Coordinate::coordinateIsRange($pCellRange)) {
832+
throw new Exception('Only cell ranges may be passed to this method.');
833+
}
834+
835+
// Update range
836+
$range = Coordinate::splitRange($pCellRange);
837+
$ic = count($range);
838+
for ($i = 0; $i < $ic; ++$i) {
839+
$jc = count($range[$i]);
840+
for ($j = 0; $j < $jc; ++$j) {
841+
if (ctype_alpha($range[$i][$j])) {
842+
$r = Coordinate::coordinateFromString($this->updateSingleCellReference($range[$i][$j] . '1', $pBefore, $pNumCols, $pNumRows));
843+
$range[$i][$j] = $r[0];
844+
} elseif (ctype_digit($range[$i][$j])) {
845+
$r = Coordinate::coordinateFromString($this->updateSingleCellReference('A' . $range[$i][$j], $pBefore, $pNumCols, $pNumRows));
846+
$range[$i][$j] = $r[1];
847+
} else {
848+
$range[$i][$j] = $this->updateSingleCellReference($range[$i][$j], $pBefore, $pNumCols, $pNumRows);
846849
}
847850
}
848-
849-
// Recreate range string
850-
return Coordinate::buildRange($range);
851851
}
852852

853-
throw new Exception('Only cell ranges may be passed to this method.');
853+
// Recreate range string
854+
return Coordinate::buildRange($range);
854855
}
855856

856857
/**
857858
* Update single cell reference.
858859
*
859860
* @param string $pCellReference Single cell reference
860-
* @param int $pBefore Insert before this one
861+
* @param string $pBefore Insert before this one
861862
* @param int $pNumCols Number of columns to increment
862863
* @param int $pNumRows Number of rows to increment
863864
*
@@ -867,32 +868,32 @@ private function updateCellRange($pCellRange = 'A1:A1', $pBefore = 'A1', $pNumCo
867868
*/
868869
private function updateSingleCellReference($pCellReference = 'A1', $pBefore = 'A1', $pNumCols = 0, $pNumRows = 0)
869870
{
870-
if (strpos($pCellReference, ':') === false && strpos($pCellReference, ',') === false) {
871-
// Get coordinate of $pBefore
872-
list($beforeColumn, $beforeRow) = Coordinate::coordinateFromString($pBefore);
871+
if (Coordinate::coordinateIsRange($pCellReference)) {
872+
throw new Exception('Only single cell references may be passed to this method.');
873+
}
873874

874-
// Get coordinate of $pCellReference
875-
list($newColumn, $newRow) = Coordinate::coordinateFromString($pCellReference);
875+
// Get coordinate of $pBefore
876+
list($beforeColumn, $beforeRow) = Coordinate::coordinateFromString($pBefore);
876877

877-
// Verify which parts should be updated
878-
$updateColumn = (($newColumn[0] != '$') && ($beforeColumn[0] != '$') && (Coordinate::columnIndexFromString($newColumn) >= Coordinate::columnIndexFromString($beforeColumn)));
879-
$updateRow = (($newRow[0] != '$') && ($beforeRow[0] != '$') && $newRow >= $beforeRow);
878+
// Get coordinate of $pCellReference
879+
list($newColumn, $newRow) = Coordinate::coordinateFromString($pCellReference);
880880

881-
// Create new column reference
882-
if ($updateColumn) {
883-
$newColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($newColumn) + $pNumCols);
884-
}
881+
// Verify which parts should be updated
882+
$updateColumn = (($newColumn[0] != '$') && ($beforeColumn[0] != '$') && (Coordinate::columnIndexFromString($newColumn) >= Coordinate::columnIndexFromString($beforeColumn)));
883+
$updateRow = (($newRow[0] != '$') && ($beforeRow[0] != '$') && $newRow >= $beforeRow);
885884

886-
// Create new row reference
887-
if ($updateRow) {
888-
$newRow = $newRow + $pNumRows;
889-
}
885+
// Create new column reference
886+
if ($updateColumn) {
887+
$newColumn = Coordinate::stringFromColumnIndex(Coordinate::columnIndexFromString($newColumn) + $pNumCols);
888+
}
890889

891-
// Return new reference
892-
return $newColumn . $newRow;
890+
// Create new row reference
891+
if ($updateRow) {
892+
$newRow = $newRow + $pNumRows;
893893
}
894894

895-
throw new Exception('Only single cell references may be passed to this method.');
895+
// Return new reference
896+
return $newColumn . $newRow;
896897
}
897898

898899
/**

‎src/PhpSpreadsheet/Worksheet/Worksheet.php

+5-5
Original file line numberDiff line numberDiff line change
@@ -1216,7 +1216,7 @@ public function getCell($pCoordinate, $createIfNotExists = true)
12161216
// Uppercase coordinate
12171217
$pCoordinate = strtoupper($pCoordinate);
12181218

1219-
if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
1219+
if (Coordinate::coordinateIsRange($pCoordinate)) {
12201220
throw new Exception('Cell coordinate can not be a range of cells.');
12211221
} elseif (strpos($pCoordinate, '$') !== false) {
12221222
throw new Exception('Cell coordinate must not be absolute.');
@@ -1324,7 +1324,7 @@ public function cellExists($pCoordinate)
13241324
// Uppercase coordinate
13251325
$pCoordinate = strtoupper($pCoordinate);
13261326

1327-
if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
1327+
if (Coordinate::coordinateIsRange($pCoordinate)) {
13281328
throw new Exception('Cell coordinate can not be a range of cells.');
13291329
} elseif (strpos($pCoordinate, '$') !== false) {
13301330
throw new Exception('Cell coordinate must not be absolute.');
@@ -1993,7 +1993,7 @@ public function getFreezePane()
19931993
*/
19941994
public function freezePane($cell, $topLeftCell = null)
19951995
{
1996-
if (is_string($cell) && (strpos($cell, ':') !== false || strpos($cell, ',') !== false)) {
1996+
if (is_string($cell) && Coordinate::coordinateIsRange($cell)) {
19971997
throw new Exception('Freeze pane can not be set on a range of cells.');
19981998
}
19991999

@@ -2337,7 +2337,7 @@ public function getComment($pCellCoordinate)
23372337
// Uppercase coordinate
23382338
$pCellCoordinate = strtoupper($pCellCoordinate);
23392339

2340-
if (strpos($pCellCoordinate, ':') !== false || strpos($pCellCoordinate, ',') !== false) {
2340+
if (Coordinate::coordinateIsRange($pCellCoordinate)) {
23412341
throw new Exception('Cell coordinate string can not be a range of cells.');
23422342
} elseif (strpos($pCellCoordinate, '$') !== false) {
23432343
throw new Exception('Cell coordinate string must not be absolute.');
@@ -2428,7 +2428,7 @@ public function setSelectedCells($pCoordinate)
24282428
// Convert '1:3' to 'A1:XFD3'
24292429
$pCoordinate = preg_replace('/^(\d+):(\d+)$/', 'A${1}:XFD${2}', $pCoordinate);
24302430

2431-
if (strpos($pCoordinate, ':') !== false || strpos($pCoordinate, ',') !== false) {
2431+
if (Coordinate::coordinateIsRange($pCoordinate)) {
24322432
list($first) = Coordinate::splitRange($pCoordinate);
24332433
$this->activeCell = $first[0];
24342434
} else {

‎tests/PhpSpreadsheetTests/Cell/CoordinateTest.php

+16
Original file line numberDiff line numberDiff line change
@@ -331,4 +331,20 @@ public function providerMergeRangesInCollection()
331331
{
332332
return require 'data/CellMergeRangesInCollection.php';
333333
}
334+
335+
/**
336+
* @dataProvider providerCoordinateIsRange
337+
*
338+
* @param mixed $expectedResult
339+
*/
340+
public function testCoordinateIsRange($expectedResult, ...$args)
341+
{
342+
$result = Coordinate::coordinateIsRange(...$args);
343+
self::assertEquals($expectedResult, $result);
344+
}
345+
346+
public function providerCoordinateIsRange()
347+
{
348+
return require 'data/CoordinateIsRange.php';
349+
}
334350
}

‎tests/data/CellMergeRangesInCollection.php

+12
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,16 @@
5555
'A3' => 'z',
5656
],
5757
],
58+
[
59+
[
60+
'C1' => 'x',
61+
'A1:A3' => 'x',
62+
'A1:A3,C1:C3' => 'y',
63+
],
64+
[
65+
'C1' => 'x',
66+
'A1:A3' => 'x',
67+
'A1:A3,C1:C3' => 'y',
68+
],
69+
],
5870
];

0 commit comments

Comments
 (0)