From 16a2f91a222bcb195c002567443253cbf3136b3b Mon Sep 17 00:00:00 2001 From: Adrien Loison Date: Sat, 28 Sep 2019 15:42:19 +0200 Subject: [PATCH] Cell indexes not being respected when rendering row Fixes #682 When calling `Row::setCellIndex`, it's possible to create a Row with holes. Instead of iterating over existing cells of a Row, we should instead use the cell indexes (from 0 to max cell index). --- src/Spout/Common/Entity/Row.php | 10 ++++-- .../Reader/Common/Manager/RowManager.php | 6 ++-- src/Spout/Writer/Common/Helper/CellHelper.php | 29 ++++++++-------- .../Writer/XLSX/Manager/WorksheetManager.php | 34 +++++++++---------- tests/Spout/Common/Entity/RowTest.php | 14 ++++++++ .../Writer/Common/Helper/CellHelperTest.php | 13 +++---- 6 files changed, 65 insertions(+), 41 deletions(-) diff --git a/src/Spout/Common/Entity/Row.php b/src/Spout/Common/Entity/Row.php index ccb6bf5..c54cbc0 100644 --- a/src/Spout/Common/Entity/Row.php +++ b/src/Spout/Common/Entity/Row.php @@ -70,7 +70,7 @@ class Row */ public function getCellAtIndex($cellIndex) { - return isset($this->cells[$cellIndex]) ? $this->cells[$cellIndex] : null; + return $this->cells[$cellIndex] ?? null; } /** @@ -89,7 +89,13 @@ class Row */ public function getNumCells() { - return count($this->cells); + // When using "setCellAtIndex", it's possible to + // have "$this->cells" contain holes. + if (empty($this->cells)) { + return 0; + } + + return max(array_keys($this->cells)) + 1; } /** diff --git a/src/Spout/Reader/Common/Manager/RowManager.php b/src/Spout/Reader/Common/Manager/RowManager.php index ea76255..623c8d8 100644 --- a/src/Spout/Reader/Common/Manager/RowManager.php +++ b/src/Spout/Reader/Common/Manager/RowManager.php @@ -47,12 +47,14 @@ class RowManager */ public function fillMissingIndexesWithEmptyCells(Row $row) { - if ($row->getNumCells() === 0) { + $numCells = $row->getNumCells(); + + if ($numCells === 0) { return $row; } $rowCells = $row->getCells(); - $maxCellIndex = max(array_keys($rowCells)); + $maxCellIndex = $numCells; for ($cellIndex = 0; $cellIndex < $maxCellIndex; $cellIndex++) { if (!isset($rowCells[$cellIndex])) { diff --git a/src/Spout/Writer/Common/Helper/CellHelper.php b/src/Spout/Writer/Common/Helper/CellHelper.php index 472d310..87623d8 100644 --- a/src/Spout/Writer/Common/Helper/CellHelper.php +++ b/src/Spout/Writer/Common/Helper/CellHelper.php @@ -8,38 +8,39 @@ namespace Box\Spout\Writer\Common\Helper; */ class CellHelper { - /** @var array Cache containing the mapping column index => cell index */ - private static $columnIndexToCellIndexCache = []; + /** @var array Cache containing the mapping column index => column letters */ + private static $columnIndexToColumnLettersCache = []; /** - * Returns the cell index (base 26) associated to the base 10 column index. + * Returns the column letters (base 26) associated to the base 10 column index. * Excel uses A to Z letters for column indexing, where A is the 1st column, * Z is the 26th and AA is the 27th. * The mapping is zero based, so that 0 maps to A, B maps to 1, Z to 25 and AA to 26. * - * @param int $columnIndex The Excel column index (0, 42, ...) + * @param int $columnIndexZeroBased The Excel column index (0, 42, ...) + * * @return string The associated cell index ('A', 'BC', ...) */ - public static function getCellIndexFromColumnIndex($columnIndex) + public static function getColumnLettersFromColumnIndex($columnIndexZeroBased) { - $originalColumnIndex = $columnIndex; + $originalColumnIndex = $columnIndexZeroBased; // Using isset here because it is way faster than array_key_exists... - if (!isset(self::$columnIndexToCellIndexCache[$originalColumnIndex])) { - $cellIndex = ''; + if (!isset(self::$columnIndexToColumnLettersCache[$originalColumnIndex])) { + $columnLetters = ''; $capitalAAsciiValue = ord('A'); do { - $modulus = $columnIndex % 26; - $cellIndex = chr($capitalAAsciiValue + $modulus) . $cellIndex; + $modulus = $columnIndexZeroBased % 26; + $columnLetters = chr($capitalAAsciiValue + $modulus) . $columnLetters; // substracting 1 because it's zero-based - $columnIndex = (int) ($columnIndex / 26) - 1; - } while ($columnIndex >= 0); + $columnIndexZeroBased = (int) ($columnIndexZeroBased / 26) - 1; + } while ($columnIndexZeroBased >= 0); - self::$columnIndexToCellIndexCache[$originalColumnIndex] = $cellIndex; + self::$columnIndexToColumnLettersCache[$originalColumnIndex] = $columnLetters; } - return self::$columnIndexToCellIndexCache[$originalColumnIndex]; + return self::$columnIndexToColumnLettersCache[$originalColumnIndex]; } } diff --git a/src/Spout/Writer/XLSX/Manager/WorksheetManager.php b/src/Spout/Writer/XLSX/Manager/WorksheetManager.php index b0e458f..e02e9e6 100644 --- a/src/Spout/Writer/XLSX/Manager/WorksheetManager.php +++ b/src/Spout/Writer/XLSX/Manager/WorksheetManager.php @@ -153,16 +153,14 @@ EOD; */ private function addNonEmptyRow(Worksheet $worksheet, Row $row) { - $cellIndex = 0; $rowStyle = $row->getStyle(); - $rowIndex = $worksheet->getLastWrittenRowIndex() + 1; + $rowIndexOneBased = $worksheet->getLastWrittenRowIndex() + 1; $numCells = $row->getNumCells(); - $rowXML = ''; + $rowXML = ''; - foreach ($row->getCells() as $cell) { - $rowXML .= $this->applyStyleAndGetCellXML($cell, $rowStyle, $rowIndex, $cellIndex); - $cellIndex++; + foreach ($row->getCells() as $columnIndexZeroBased => $cell) { + $rowXML .= $this->applyStyleAndGetCellXML($cell, $rowStyle, $rowIndexOneBased, $columnIndexZeroBased); } $rowXML .= ''; @@ -177,14 +175,15 @@ EOD; * Applies styles to the given style, merging the cell's style with its row's style * Then builds and returns xml for the cell. * - * @param Cell $cell + * @param Cell $cell * @param Style $rowStyle - * @param int $rowIndex - * @param int $cellIndex + * @param int $rowIndexOneBased + * @param int $columnIndexZeroBased + * * @throws InvalidArgumentException If the given value cannot be processed * @return string */ - private function applyStyleAndGetCellXML(Cell $cell, Style $rowStyle, $rowIndex, $cellIndex) + private function applyStyleAndGetCellXML(Cell $cell, Style $rowStyle, $rowIndexOneBased, $columnIndexZeroBased) { // Apply row and extra styles $mergedCellAndRowStyle = $this->styleMerger->merge($cell->getStyle(), $rowStyle); @@ -193,23 +192,24 @@ EOD; $registeredStyle = $this->styleManager->registerStyle($newCellStyle); - return $this->getCellXML($rowIndex, $cellIndex, $cell, $registeredStyle->getId()); + return $this->getCellXML($rowIndexOneBased, $columnIndexZeroBased, $cell, $registeredStyle->getId()); } /** * Builds and returns xml for a single cell. * - * @param int $rowIndex - * @param int $cellNumber + * @param int $rowIndexOneBased + * @param int $columnIndexZeroBased * @param Cell $cell - * @param int $styleId + * @param int $styleId + * * @throws InvalidArgumentException If the given value cannot be processed * @return string */ - private function getCellXML($rowIndex, $cellNumber, Cell $cell, $styleId) + private function getCellXML($rowIndexOneBased, $columnIndexZeroBased, Cell $cell, $styleId) { - $columnIndex = CellHelper::getCellIndexFromColumnIndex($cellNumber); - $cellXML = 'isString()) { diff --git a/tests/Spout/Common/Entity/RowTest.php b/tests/Spout/Common/Entity/RowTest.php index 2325412..095025c 100644 --- a/tests/Spout/Common/Entity/RowTest.php +++ b/tests/Spout/Common/Entity/RowTest.php @@ -78,10 +78,24 @@ class RowTest extends \PHPUnit\Framework\TestCase $row = new Row([], null); $cellMock = $this->getCellMock(); $row->setCellAtIndex($cellMock, 3); + $this->assertEquals($cellMock, $row->getCellAtIndex(3)); $this->assertNull($row->getCellAtIndex(10)); } + /** + * @return void + */ + public function testSetCellAtIndex() + { + $row = new Row([], null); + $cellMock = $this->getCellMock(); + $row->setCellAtIndex($cellMock, 1); + + $this->assertEquals(2, $row->getNumCells()); + $this->assertNull($row->getCellAtIndex(0)); + } + /** * @return void */ diff --git a/tests/Spout/Writer/Common/Helper/CellHelperTest.php b/tests/Spout/Writer/Common/Helper/CellHelperTest.php index d699bc0..3ab9a79 100644 --- a/tests/Spout/Writer/Common/Helper/CellHelperTest.php +++ b/tests/Spout/Writer/Common/Helper/CellHelperTest.php @@ -12,7 +12,7 @@ class CellHelperTest extends TestCase /** * @return array */ - public function dataProviderForTestGetCellIndexFromColumnIndex() + public function dataProviderForTestGetColumnLettersFromColumnIndex() { return [ [0, 'A'], @@ -24,14 +24,15 @@ class CellHelperTest extends TestCase } /** - * @dataProvider dataProviderForTestGetCellIndexFromColumnIndex + * @dataProvider dataProviderForTestGetColumnLettersFromColumnIndex + * + * @param int $columnIndex + * @param string $expectedColumnLetters * - * @param int $columnIndex - * @param string $expectedCellIndex * @return void */ - public function testGetCellIndexFromColumnIndex($columnIndex, $expectedCellIndex) + public function testGetColumnLettersFromColumnIndex($columnIndex, $expectedColumnLetters) { - $this->assertEquals($expectedCellIndex, CellHelper::getCellIndexFromColumnIndex($columnIndex)); + $this->assertEquals($expectedColumnLetters, CellHelper::getColumnLettersFromColumnIndex($columnIndex)); } }