From 49775bd9721fd31b162a7840514bd69902eff77c Mon Sep 17 00:00:00 2001 From: MaxTingle Date: Tue, 9 Jan 2018 21:31:06 +0000 Subject: [PATCH] Fix cell ranges causing coordinate merge error Fixes #319 Closes #328 --- CHANGELOG.md | 1 + src/PhpSpreadsheet/Cell/Coordinate.php | 100 ++++++++++------- src/PhpSpreadsheet/ReferenceHelper.php | 105 +++++++++--------- src/PhpSpreadsheet/Worksheet/Worksheet.php | 10 +- .../Cell/CoordinateTest.php | 16 +++ tests/data/CellMergeRangesInCollection.php | 12 ++ tests/data/CoordinateIsRange.php | 24 ++++ 7 files changed, 170 insertions(+), 98 deletions(-) create mode 100644 tests/data/CoordinateIsRange.php diff --git a/CHANGELOG.md b/CHANGELOG.md index bc988426..9ef9d064 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index 82ba5a98..9a3c4b6c 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/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) { - // 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); + if (self::coordinateIsRange($pCoordinateString)) { + throw new Exception('Cell coordinate string can not be a range of cells'); } - 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) { - // 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; + if (self::coordinateIsRange($pCoordinateString)) { + throw new Exception('Cell coordinate string can not be a range of cells'); } - 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) { diff --git a/src/PhpSpreadsheet/ReferenceHelper.php b/src/PhpSpreadsheet/ReferenceHelper.php index bd74f0f1..2735de2a 100644 --- a/src/PhpSpreadsheet/ReferenceHelper.php +++ b/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) { - // 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); + if (!Coordinate::coordinateIsRange($pCellRange)) { + throw new Exception('Only cell ranges may be passed to this method.'); } - 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) { - // 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; + if (Coordinate::coordinateIsRange($pCellReference)) { + throw new Exception('Only single cell references may be passed to this method.'); } - 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; } /** diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php index 31ddc62d..4b349322 100644 --- a/src/PhpSpreadsheet/Worksheet/Worksheet.php +++ b/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 { diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index e6efa88e..9b56209e 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/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'; + } } diff --git a/tests/data/CellMergeRangesInCollection.php b/tests/data/CellMergeRangesInCollection.php index 9cb71125..0a7723ee 100644 --- a/tests/data/CellMergeRangesInCollection.php +++ b/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', + ], + ], ]; diff --git a/tests/data/CoordinateIsRange.php b/tests/data/CoordinateIsRange.php new file mode 100644 index 00000000..4718bdce --- /dev/null +++ b/tests/data/CoordinateIsRange.php @@ -0,0 +1,24 @@ +