From 5c18bb5798be3c186e89b534de691a20beab21a7 Mon Sep 17 00:00:00 2001 From: Mark Baker Date: Tue, 2 Jun 2020 07:38:35 +0200 Subject: [PATCH] Range operator tests (#1501) * Improved handling of named ranges, although there are still some issues (names ranges using a union type with an overlap don't handle the overlap twice, which as the MS Excel approach to set overlaps as opposed to the mathematical approach which only applies overlap values once) * Fix tests that misused space and comma as simple separators in cell ranges --- .../Calculation/Calculation.php | 12 ++- src/PhpSpreadsheet/Cell/Coordinate.php | 73 ++++++++++++----- .../Calculation/Engine/RangeTest.php | 80 +++++++++++++------ .../Cell/CoordinateTest.php | 49 +++++++----- .../CellExtractAllCellReferencesInRange.php | 25 +----- 5 files changed, 151 insertions(+), 88 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 5aa309c5..2d67a134 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -3625,7 +3625,6 @@ class Calculation $expectingOperand = false; $val = $match[1]; $length = strlen($val); - if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $val, $matches)) { $val = preg_replace('/\s/u', '', $val); if (isset(self::$phpSpreadsheetFunctions[strtoupper($matches[1])]) || isset(self::$controlFunctions[strtoupper($matches[1])])) { // it's a function @@ -3660,7 +3659,6 @@ class Calculation } elseif (preg_match('/^' . self::CALCULATION_REGEXP_CELLREF . '$/i', $val, $matches)) { // Watch for this case-change when modifying to allow cell references in different worksheets... // Should only be applied to the actual cell column, not the worksheet name - // If the last entry on the stack was a : operator, then we have a cell range reference $testPrevOp = $stack->last(1); if ($testPrevOp !== null && $testPrevOp['value'] == ':') { @@ -3717,6 +3715,8 @@ class Calculation } $localeConstant = false; + $stackItemType = 'Value'; + $stackItemReference = null; if ($opCharacter == self::FORMULA_STRING_QUOTE) { // UnEscape any quotes within the string $val = self::wrapResult(str_replace('""', self::FORMULA_STRING_QUOTE, self::unwrapResult($val))); @@ -3727,12 +3727,17 @@ class Calculation $val = (int) $val; } } elseif (isset(self::$excelConstants[trim(strtoupper($val))])) { + $stackItemType = 'Constant'; $excelConstant = trim(strtoupper($val)); $val = self::$excelConstants[$excelConstant]; } elseif (($localeConstant = array_search(trim(strtoupper($val)), self::$localeBoolean)) !== false) { + $stackItemType = 'Constant'; $val = self::$excelConstants[$localeConstant]; + } elseif (preg_match('/^' . self::CALCULATION_REGEXP_NAMEDRANGE . '.*/Ui', $val, $match)) { + $stackItemType = 'Named Range'; + $stackItemReference = $val; } - $details = $stack->getStackItem('Value', $val, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); + $details = $stack->getStackItem($stackItemType, $val, $stackItemReference, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); if ($localeConstant) { $details['localeValue'] = $localeConstant; } @@ -3842,7 +3847,6 @@ class Calculation $fakedForBranchPruning = []; // help us to know when pruning ['branchTestId' => true/false] $branchStore = []; - // Loop through each token in turn foreach ($tokens as $tokenData) { $token = $tokenData['value']; diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php index 8c679913..2afeebe9 100644 --- a/src/PhpSpreadsheet/Cell/Coordinate.php +++ b/src/PhpSpreadsheet/Cell/Coordinate.php @@ -312,32 +312,59 @@ abstract class Coordinate /** * 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) + * @param string $cellRange Range: e.g. 'A1' or 'A1:C10' or 'A1:E10,A20:E25' or 'A1:E5 C3:G7' or 'A1:C1,A3:C3 B1:C3' * * @return array Array containing single cell references */ - public static function extractAllCellReferencesInRange($pRange) + public static function extractAllCellReferencesInRange($cellRange): array { - $returnValue = []; + [$ranges, $operators] = self::getCellBlocksFromRangeString($cellRange); - // Explode spaces - $cellBlocks = self::getCellBlocksFromRangeString($pRange); - foreach ($cellBlocks as $cellBlock) { - $returnValue = array_merge($returnValue, self::getReferencesForCellBlock($cellBlock)); + $cells = []; + foreach ($ranges as $range) { + $cells[] = self::getReferencesForCellBlock($range); } + $cells = self::processRangeSetOperators($operators, $cells); + + if (empty($cells)) { + return []; + } + + $cellList = array_merge(...$cells); + $cellList = self::sortCellReferenceArray($cellList); + + return $cellList; + } + + private static function processRangeSetOperators(array $operators, array $cells): array + { + for ($offset = 0; $offset < count($operators); ++$offset) { + $operator = $operators[$offset]; + if ($operator !== ' ') { + continue; + } + + $cells[$offset] = array_intersect($cells[$offset], $cells[$offset + 1]); + unset($operators[$offset], $cells[$offset + 1]); + $operators = array_values($operators); + $cells = array_values($cells); + --$offset; + } + + return $cells; + } + + private static function sortCellReferenceArray(array $cellList): array + { // Sort the result by column and row $sortKeys = []; - foreach (array_unique($returnValue) as $coord) { - $column = ''; - $row = 0; - - sscanf($coord, '%[A-Z]%d', $column, $row); + foreach ($cellList as $coord) { + [$column, $row] = sscanf($coord, '%[A-Z]%d'); $sortKeys[sprintf('%3s%09d', $column, $row)] = $coord; } ksort($sortKeys); - // Return value return array_values($sortKeys); } @@ -482,15 +509,25 @@ abstract class Coordinate } /** - * Get the individual cell blocks from a range string, splitting by space and removing any $ characters. + * Get the individual cell blocks from a range string, removing any $ characters. + * then splitting by operators and returning an array with ranges and operators. * - * @param string $pRange + * @param string $rangeString * - * @return string[] + * @return array[] */ - private static function getCellBlocksFromRangeString($pRange) + private static function getCellBlocksFromRangeString($rangeString) { - return explode(' ', str_replace('$', '', strtoupper($pRange))); + $rangeString = str_replace('$', '', strtoupper($rangeString)); + + // split range sets on intersection (space) or union (,) operators + $tokens = preg_split('/([ ,])/', $rangeString, -1, PREG_SPLIT_DELIM_CAPTURE); + // separate the range sets and the operators into arrays + $split = array_chunk($tokens, 2); + $ranges = array_column($split, 0); + $operators = array_column($split, 1); + + return [$ranges, $operators]; } /** diff --git a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php index d1ad229b..ee566db2 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php @@ -45,11 +45,21 @@ class RangeTest extends TestCase { return[ ['=SUM(A1:B3,A1:C2)', 48], + ['=COUNT(A1:B3,A1:C2)', 12], ['=SUM(A1:B3 A1:C2)', 12], + ['=COUNT(A1:B3 A1:C2)', 4], ['=SUM(A1:A3,C1:C3)', 30], + ['=COUNT(A1:A3,C1:C3)', 6], ['=SUM(A1:A3 C1:C3)', Functions::null()], + ['=COUNT(A1:A3 C1:C3)', 0], ['=SUM(A1:B2,B2:C3)', 40], + ['=COUNT(A1:B2,B2:C3)', 8], ['=SUM(A1:B2 B2:C3)', 5], + ['=COUNT(A1:B2 B2:C3)', 1], + ['=SUM(A1:C1,A3:C3,B1:C3)', 63], + ['=COUNT(A1:C1,A3:C3,B1:C3)', 12], + ['=SUM(A1:C1,A3:C3 B1:C3)', 23], + ['=COUNT(A1:C1,A3:C3 B1:C3)', 5], ]; } @@ -69,35 +79,57 @@ class RangeTest extends TestCase $workSheet->setCellValue('E1', $formula); - $actualRresult = $workSheet->getCell('E1')->getCalculatedValue(); - self::assertSame($expectedResult, $actualRresult); + $sumRresult = $workSheet->getCell('E1')->getCalculatedValue(); + self::assertSame($expectedResult, $sumRresult); } public function providerNamedRangeEvaluation() { return[ - [ - 'A1:B3', - 'A1:C2', - '=SUM(GROUP1,GROUP2)', - 48, - ], - [ - 'A1:B3', - 'A1:C2', - '=SUM(GROUP1 GROUP2)', - 12, - ], - [ - 'A1:B2', - 'B2:C3', - '=SUM(GROUP1,GROUP2)', - 40, - ], - [ - 'A1:B2', - 'B2:C3', - '=SUM(GROUP1 GROUP2)', + ['A1:B3', 'A1:C2', '=SUM(GROUP1,GROUP2)', 48], + ['A1:B3', 'A1:C2', '=COUNT(GROUP1,GROUP2)', 12], + ['A1:B3', 'A1:C2', '=SUM(GROUP1 GROUP2)', 12], + ['A1:B3', 'A1:C2', '=COUNT(GROUP1 GROUP2)', 4], + ['A1:B2', 'B2:C3', '=SUM(GROUP1,GROUP2)', 40], + ['A1:B2', 'B2:C3', '=COUNT(GROUP1,GROUP2)', 8], + ['A1:B2', 'B2:C3', '=SUM(GROUP1 GROUP2)', 5], + ['A1:B2', 'B2:C3', '=COUNT(GROUP1 GROUP2)', 1], + ]; + } + + /** + * @dataProvider providerCompositeNamedRangeEvaluation + * + * @param string $composite + * @param int $expectedSum + * @param int $expectedCount + */ + public function testCompositeNamedRangeEvaluation($composite, $expectedSum, $expectedCount): void + { + $workSheet = $this->spreadSheet->getActiveSheet(); + $this->spreadSheet->addNamedRange(new NamedRange('COMPOSITE', $workSheet, $composite)); + + $workSheet->setCellValue('E1', '=SUM(COMPOSITE)'); + $workSheet->setCellValue('E2', '=COUNT(COMPOSITE)'); + + $actualSum = $workSheet->getCell('E1')->getCalculatedValue(); + self::assertSame($expectedSum, $actualSum); + $actualCount = $workSheet->getCell('E2')->getCalculatedValue(); + self::assertSame($expectedCount, $actualCount); + } + + public function providerCompositeNamedRangeEvaluation() + { + return[ + // Calculation engine doesn't yet handle union ranges with overlap + // 'Union with overlap' => [ + // 'A1:C1,A3:C3,B1:C3', + // 63, + // 12, + // ], + 'Intersection' => [ + 'A1:C1,A3:C3 B1:C3', + 23, 5, ], ]; diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php index 37579e80..8e0e98a9 100644 --- a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php +++ b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php @@ -83,10 +83,11 @@ class CoordinateTest extends TestCase * @dataProvider providerCoordinates * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testCoordinateFromString($expectedResult, ...$args): void + public function testCoordinateFromString($expectedResult, $rangeSet): void { - $result = Coordinate::coordinateFromString(...$args); + $result = Coordinate::coordinateFromString($rangeSet); self::assertEquals($expectedResult, $result); } @@ -143,11 +144,12 @@ class CoordinateTest extends TestCase /** * @dataProvider providerAbsoluteCoordinates * - * @param mixed $expectedResult + * @param string $expectedResult + * @param string $rangeSet */ - public function testAbsoluteCoordinateFromString($expectedResult, ...$args): void + public function testAbsoluteCoordinateFromString($expectedResult, $rangeSet): void { - $result = Coordinate::absoluteCoordinate(...$args); + $result = Coordinate::absoluteCoordinate($rangeSet); self::assertEquals($expectedResult, $result); } @@ -175,10 +177,11 @@ class CoordinateTest extends TestCase * @dataProvider providerAbsoluteReferences * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testAbsoluteReferenceFromString($expectedResult, ...$args): void + public function testAbsoluteReferenceFromString($expectedResult, $rangeSet): void { - $result = Coordinate::absoluteReference(...$args); + $result = Coordinate::absoluteReference($rangeSet); self::assertEquals($expectedResult, $result); } @@ -206,10 +209,11 @@ class CoordinateTest extends TestCase * @dataProvider providerSplitRange * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testSplitRange($expectedResult, ...$args): void + public function testSplitRange($expectedResult, $rangeSet): void { - $result = Coordinate::splitRange(...$args); + $result = Coordinate::splitRange($rangeSet); foreach ($result as $key => $split) { if (!is_array($expectedResult[$key])) { self::assertEquals($expectedResult[$key], $split[0]); @@ -252,10 +256,11 @@ class CoordinateTest extends TestCase * @dataProvider providerRangeBoundaries * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testRangeBoundaries($expectedResult, ...$args): void + public function testRangeBoundaries($expectedResult, $rangeSet): void { - $result = Coordinate::rangeBoundaries(...$args); + $result = Coordinate::rangeBoundaries($rangeSet); self::assertEquals($expectedResult, $result); } @@ -268,10 +273,11 @@ class CoordinateTest extends TestCase * @dataProvider providerRangeDimension * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testRangeDimension($expectedResult, ...$args): void + public function testRangeDimension($expectedResult, $rangeSet): void { - $result = Coordinate::rangeDimension(...$args); + $result = Coordinate::rangeDimension($rangeSet); self::assertEquals($expectedResult, $result); } @@ -284,10 +290,11 @@ class CoordinateTest extends TestCase * @dataProvider providerGetRangeBoundaries * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testGetRangeBoundaries($expectedResult, ...$args): void + public function testGetRangeBoundaries($expectedResult, $rangeSet): void { - $result = Coordinate::getRangeBoundaries(...$args); + $result = Coordinate::getRangeBoundaries($rangeSet); self::assertEquals($expectedResult, $result); } @@ -299,11 +306,12 @@ class CoordinateTest extends TestCase /** * @dataProvider providerExtractAllCellReferencesInRange * - * @param mixed $expectedResult + * @param array $expectedResult + * @param string $rangeSet */ - public function testExtractAllCellReferencesInRange($expectedResult, ...$args): void + public function testExtractAllCellReferencesInRange($expectedResult, $rangeSet): void { - $result = Coordinate::extractAllCellReferencesInRange(...$args); + $result = Coordinate::extractAllCellReferencesInRange($rangeSet); self::assertEquals($expectedResult, $result); } @@ -350,10 +358,11 @@ class CoordinateTest extends TestCase * @dataProvider providerCoordinateIsRange * * @param mixed $expectedResult + * @param string $rangeSet */ - public function testCoordinateIsRange($expectedResult, ...$args): void + public function testCoordinateIsRange($expectedResult, $rangeSet): void { - $result = Coordinate::coordinateIsRange(...$args); + $result = Coordinate::coordinateIsRange($rangeSet); self::assertEquals($expectedResult, $result); } diff --git a/tests/data/CellExtractAllCellReferencesInRange.php b/tests/data/CellExtractAllCellReferencesInRange.php index cf093289..b005b1fe 100644 --- a/tests/data/CellExtractAllCellReferencesInRange.php +++ b/tests/data/CellExtractAllCellReferencesInRange.php @@ -22,12 +22,6 @@ return [ ], [ [ - 'B4', - 'B5', - 'B6', - 'D4', - 'D5', - 'D6', ], 'B4:B6 D4:D6', ], @@ -66,20 +60,10 @@ return [ ], [ [ - 'B4', - 'B5', - 'B6', - 'C4', 'C5', 'C6', - 'C7', - 'D4', 'D5', 'D6', - 'D7', - 'E5', - 'E6', - 'E7', ], 'B4:D6 C5:E7', ], @@ -105,7 +89,7 @@ return [ 'F5', 'F6', ], - 'B2:D4 C5:D5 E3:E5 D6:E6 F4:F6', + 'B2:D4,C5:D5,E3:E5,D6:E6,F4:F6', ], [ [ @@ -129,16 +113,13 @@ return [ 'F5', 'F6', ], - 'B2:D4 C3:E5 D4:F6', + 'B2:D4,C3:E5,D4:F6', ], [ [ - 'B4', 'B5', - 'B6', - 'B8', ], - 'B4:B6 B8', + 'B4:B6 B5', ], [ [