From 64b1b91962fa7d0062813f371d47888c0ace6de2 Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Wed, 30 May 2018 22:54:38 +0100 Subject: [PATCH 1/7] Prevent infinite loop when getting references from invalid range --- src/PhpSpreadsheet/Cell/Coordinate.php | 2 +- tests/PhpSpreadsheetTests/Cell/CoordinateTest.php | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index 1efcb627fc..54496a7765 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -369,7 +369,7 @@ public static function extractAllCellReferencesInRange($pRange) $currentRow = $startRow; // Loop cells - while ($currentCol != $endCol) { + while ($currentCol < $endCol) { while ($currentRow <= $endRow) { $returnValue[] = $currentCol . $currentRow; ++$currentRow; diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index 0b4263bf21..d9de39d735 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php @@ -315,6 +315,13 @@ public function providerExtractAllCellReferencesInRange() return require 'data/CellExtractAllCellReferencesInRange.php'; } + public function testExtractAllCellReferencesInRangeInvalidRange() + { + $result = Coordinate::extractAllCellReferencesInRange('Z1:A1'); + + self::assertSame([], $result); + } + /** * @dataProvider providerMergeRangesInCollection * From fbae1cf631393e810dbc87c1d89d1b1ce144f101 Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Wed, 30 May 2018 23:04:51 +0100 Subject: [PATCH 2/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00e7748c0e..2f2cc1687d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `Helper\Html` support UTF-8 HTML input - [#444](https://github.com/PHPOffice/PhpSpreadsheet/issues/444) - Xlsx loaded an extra empty comment for each real comment - [#375](https://github.com/PHPOffice/PhpSpreadsheet/issues/375) - Xlsx reader do not read rows and columns filtered out in readFilter at all - [#370](https://github.com/PHPOffice/PhpSpreadsheet/issues/370) +- `Coordinate::ExtractAllCellReferencesInRange()` returns empty array instead of looping when passed an invalid range – [#519](https://github.com/PHPOffice/PhpSpreadsheet/issues/519) ## [1.2.1] - 2018-04-10 From fc478662088bc0cadf71a522767c2f287a00a3fa Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Fri, 1 Jun 2018 23:09:22 +0100 Subject: [PATCH 3/7] Throw exception for invalid range --- CHANGELOG.md | 2 +- src/PhpSpreadsheet/Cell/Coordinate.php | 4 +++ .../Cell/CoordinateTest.php | 29 +++++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f2cc1687d..16c26a1762 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `Helper\Html` support UTF-8 HTML input - [#444](https://github.com/PHPOffice/PhpSpreadsheet/issues/444) - Xlsx loaded an extra empty comment for each real comment - [#375](https://github.com/PHPOffice/PhpSpreadsheet/issues/375) - Xlsx reader do not read rows and columns filtered out in readFilter at all - [#370](https://github.com/PHPOffice/PhpSpreadsheet/issues/370) -- `Coordinate::ExtractAllCellReferencesInRange()` returns empty array instead of looping when passed an invalid range – [#519](https://github.com/PHPOffice/PhpSpreadsheet/issues/519) +- `Coordinate::ExtractAllCellReferencesInRange()` throws an exception for an invalid range – [#519](https://github.com/PHPOffice/PhpSpreadsheet/issues/519) ## [1.2.1] - 2018-04-10 diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index 54496a7765..f0132c74f7 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -368,6 +368,10 @@ public static function extractAllCellReferencesInRange($pRange) $currentCol = $startCol; $currentRow = $startRow; + if ($startCol >= $endCol || $currentRow > $endRow) { + throw new Exception('Invalid range: "'. $pRange . '"'); + } + // Loop cells while ($currentCol < $endCol) { while ($currentRow <= $endRow) { diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index d9de39d735..bb8f0ff321 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php @@ -62,7 +62,7 @@ public function testColumnIndexFromStringTooShort() * @dataProvider providerColumnIndex * * @param mixed $expectedResult - * @param int $columnIndex + * @param int $columnIndex */ public function testStringFromColumnIndex($expectedResult, $columnIndex) { @@ -70,7 +70,8 @@ public function testStringFromColumnIndex($expectedResult, $columnIndex) self::assertEquals($expectedResult, $string); $columnIndexBack = Coordinate::columnIndexFromString($string); - self::assertEquals($columnIndexBack, $columnIndex, 'should be able to get the original input with opposite method'); + self::assertEquals($columnIndexBack, $columnIndex, + 'should be able to get the original input with opposite method'); } public function providerColumnIndex() @@ -210,7 +211,7 @@ public function testSplitRange($expectedResult, ...$args) { $result = Coordinate::splitRange(...$args); foreach ($result as $key => $split) { - if (!is_array($expectedResult[$key])) { + if (! is_array($expectedResult[$key])) { self::assertEquals($expectedResult[$key], $split[0]); } else { self::assertEquals($expectedResult[$key], $split); @@ -315,11 +316,27 @@ public function providerExtractAllCellReferencesInRange() return require 'data/CellExtractAllCellReferencesInRange.php'; } - public function testExtractAllCellReferencesInRangeInvalidRange() + /** + * @dataProvider providerInvalidRange + * + * @param string $range + */ + public function testExtractAllCellReferencesInRangeInvalidRange($range) { - $result = Coordinate::extractAllCellReferencesInRange('Z1:A1'); + try { + Coordinate::extractAllCellReferencesInRange($range); + } catch (Exception $e) { + self::assertSame('Invalid range: "' . $range . '"', $e->getMessage()); + + return; + } - self::assertSame([], $result); + self::fail('Exception not thrown for invalid range "' . $range . '".'); + } + + public function providerInvalidRange() + { + return [['Z1:A1'], ['A4:A1'], ['B1:A1'], ['AA1:Z1']]; } /** From 0d627dd211d4c88ecd41fa5417bad3e41165ff1f Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Fri, 1 Jun 2018 23:33:10 +0100 Subject: [PATCH 4/7] Fix code style --- src/PhpSpreadsheet/Cell/Coordinate.php | 2 +- tests/PhpSpreadsheetTests/Cell/CoordinateTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index f0132c74f7..ef36efa585 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -369,7 +369,7 @@ public static function extractAllCellReferencesInRange($pRange) $currentRow = $startRow; if ($startCol >= $endCol || $currentRow > $endRow) { - throw new Exception('Invalid range: "'. $pRange . '"'); + throw new Exception('Invalid range: "' . $pRange . '"'); } // Loop cells diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index bb8f0ff321..737c02615d 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php @@ -211,7 +211,7 @@ public function testSplitRange($expectedResult, ...$args) { $result = Coordinate::splitRange(...$args); foreach ($result as $key => $split) { - if (! is_array($expectedResult[$key])) { + if (!is_array($expectedResult[$key])) { self::assertEquals($expectedResult[$key], $split[0]); } else { self::assertEquals($expectedResult[$key], $split); From 38c0dcca0bec8ee090b89cfedd77ee359bad4595 Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Sun, 3 Jun 2018 21:35:23 +0100 Subject: [PATCH 5/7] Refactor Coordinate methods --- src/PhpSpreadsheet/Cell/Coordinate.php | 123 ++++++++++++++++--------- 1 file changed, 81 insertions(+), 42 deletions(-) diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index ef36efa585..be371a2de2 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -327,7 +327,7 @@ public static function stringFromColumnIndex($columnIndex) } /** - * Extract all cell references in range. + * Extract all cell references in range, which may be comprised of multiple cell ranges. * * @param string $pRange Range (e.g. A1 or A1:C10 or A1:E10 A20:E25) * @@ -335,65 +335,92 @@ public static function stringFromColumnIndex($columnIndex) */ public static function extractAllCellReferencesInRange($pRange) { - // Returnvalue $returnValue = []; // Explode spaces - $cellBlocks = explode(' ', str_replace('$', '', strtoupper($pRange))); + $cellBlocks = self::getCellBlocksFromRangeString($pRange); foreach ($cellBlocks as $cellBlock) { + $returnValue = array_merge($returnValue, static::getReferencesForCellBlock($cellBlock)); + } + + // Sort the result by column and row + $sortKeys = []; + foreach (array_unique($returnValue) as $coord) { + sscanf($coord, '%[A-Z]%d', $column, $row); + $sortKeys[sprintf('%3s%09d', $column, $row)] = $coord; + } + ksort($sortKeys); + + // Return value + return array_values($sortKeys); + } + + /** + * Get all cell references for an individual cell block. + * + * @param string $cellBlock A cell range e.g. A4:B5 + * + * @throws Exception + * + * @return array All individual cells in that range + */ + protected static function getReferencesForCellBlock($cellBlock) + { + $returnValue = []; + + // Single cell? + if (!self::coordinateIsRange($cellBlock)) { + return (array) $cellBlock; + } + + // Range... + $ranges = self::splitRange($cellBlock); + foreach ($ranges as $range) { // Single cell? - if (!self::coordinateIsRange($cellBlock)) { - $returnValue[] = $cellBlock; + if (!isset($range[1])) { + $returnValue[] = $range[0]; continue; } // Range... - $ranges = self::splitRange($cellBlock); - foreach ($ranges as $range) { - // Single cell? - if (!isset($range[1])) { - $returnValue[] = $range[0]; - - continue; - } + list($rangeStart, $rangeEnd) = $range; + list($startCol, $startRow) = static::extractColumnAndRow($rangeStart); + list($endCol, $endRow) = static::extractColumnAndRow($rangeEnd); + ++$endCol; - // Range... - list($rangeStart, $rangeEnd) = $range; - sscanf($rangeStart, '%[A-Z]%d', $startCol, $startRow); - sscanf($rangeEnd, '%[A-Z]%d', $endCol, $endRow); - ++$endCol; + // Current data + $currentCol = $startCol; + $currentRow = $startRow; - // Current data - $currentCol = $startCol; - $currentRow = $startRow; + if ($startCol >= $endCol || $currentRow > $endRow) { + throw new Exception('Invalid range: "' . $cellBlock . '"'); + } - if ($startCol >= $endCol || $currentRow > $endRow) { - throw new Exception('Invalid range: "' . $pRange . '"'); - } - - // Loop cells - while ($currentCol < $endCol) { - while ($currentRow <= $endRow) { - $returnValue[] = $currentCol . $currentRow; - ++$currentRow; - } - ++$currentCol; - $currentRow = $startRow; + // Loop cells + while ($currentCol < $endCol) { + while ($currentRow <= $endRow) { + $returnValue[] = $currentCol . $currentRow; + ++$currentRow; } + ++$currentCol; + $currentRow = $startRow; } } - // Sort the result by column and row - $sortKeys = []; - foreach (array_unique($returnValue) as $coord) { - sscanf($coord, '%[A-Z]%d', $column, $row); - $sortKeys[sprintf('%3s%09d', $column, $row)] = $coord; - } - ksort($sortKeys); + return $returnValue; + } - // Return value - return array_values($sortKeys); + /** + * Extract the column and row from a cell reference in the format [$column, $row]. + * + * @param string $cell + * + * @return array + */ + protected static function extractColumnAndRow($cell) + { + return sscanf($cell, '%[A-Z]%d'); } /** @@ -481,4 +508,16 @@ public static function mergeRangesInCollection(array $pCoordCollection) return $mergedCoordCollection; } + + /** + * Get the individual cell blocks from a range string, splitting by space and removing any $ characters. + * + * @param string $pRange + * + * @return string[] + */ + protected static function getCellBlocksFromRangeString($pRange) + { + return explode(' ', str_replace('$', '', strtoupper($pRange))); + } } From 3bfbbe5cb8da18b6f0cc9af3c4aa1717ec8f10cf Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Sun, 3 Jun 2018 22:04:05 +0100 Subject: [PATCH 6/7] Fix multiline function call style --- tests/PhpSpreadsheetTests/Cell/CoordinateTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index 737c02615d..ff31e1e372 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php @@ -62,7 +62,7 @@ public function testColumnIndexFromStringTooShort() * @dataProvider providerColumnIndex * * @param mixed $expectedResult - * @param int $columnIndex + * @param int $columnIndex */ public function testStringFromColumnIndex($expectedResult, $columnIndex) { @@ -70,8 +70,7 @@ public function testStringFromColumnIndex($expectedResult, $columnIndex) self::assertEquals($expectedResult, $string); $columnIndexBack = Coordinate::columnIndexFromString($string); - self::assertEquals($columnIndexBack, $columnIndex, - 'should be able to get the original input with opposite method'); + self::assertEquals($columnIndexBack, $columnIndex, 'should be able to get the original input with opposite method'); } public function providerColumnIndex() From c1fcee3884fe3fc323232b2bd2ab7cbc43c17bdb Mon Sep 17 00:00:00 2001 From: Robin D'Arcy Date: Sun, 3 Jun 2018 23:06:45 +0100 Subject: [PATCH 7/7] Extract range validation to method --- src/PhpSpreadsheet/Cell/Coordinate.php | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index be371a2de2..81b46bed2d 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -393,9 +393,7 @@ protected static function getReferencesForCellBlock($cellBlock) $currentCol = $startCol; $currentRow = $startRow; - if ($startCol >= $endCol || $currentRow > $endRow) { - throw new Exception('Invalid range: "' . $cellBlock . '"'); - } + static::validateRange($cellBlock, $startCol, $endCol, $currentRow, $endRow); // Loop cells while ($currentCol < $endCol) { @@ -520,4 +518,23 @@ protected static function getCellBlocksFromRangeString($pRange) { return explode(' ', str_replace('$', '', strtoupper($pRange))); } + + /** + * Check that the given range is valid, i.e. that the start column and row are not greater than the end column and + * row. + * + * @param string $cellBlock The original range, for displaying a meaningful error message + * @param string $startCol + * @param string $endCol + * @param int $currentRow + * @param int $endRow + * + * @throws Exception + */ + protected static function validateRange($cellBlock, $startCol, $endCol, $currentRow, $endRow) + { + if ($startCol >= $endCol || $currentRow > $endRow) { + throw new Exception('Invalid range: "' . $cellBlock . '"'); + } + } }