Apply Column and Row Styles to Existing Cells (#1721)

* Apply Column and Row Styles to Existing Cells

This is a fix for issue #1712.
When a style is applied to an entire row or column, it is currently
only effective for cells which don't already contain a value.
The code needs to iterate through existing cells in the row/column
in order to apply the style to them.
This could be considered a breaking change, however, I believe that
the change makes things operate as users would expect, and that the
existing implementation is incomplete.

The change also removes protected element conditionalStyles from
the Style class. That element is an unused remnant, and can no longer be
set or retrieved - methods getConditionalStyles and setConditionalStyles
actually act on an element in the Worksheet class.

Finally, additional tests are added so that Style, and in fact the
entire Style directory, now has 100% test coverage.

* Scrutinizer Changes

Scrutinizer flagged 6 statements. 5 can be easily corrected.
One is absolutely wrong (it thinks iterating through cells in column
can return null). Let's see if we can satisfy it.

* Remove Exception For CellIterator on Empty Row/Column

For my first attempt at this change, which corrects a bug by updating styles
for non-empty cells when a style is set on a row or column, I wished to make things
more efficient by using setIterateOnlyExistingCells, something which the
existing documentation recommends. This caused an exception to be generated
when the row or column is empty. So I removed that part of the change while I
researched what was going on.

I have completed that research. The existing code does throw an exception
when the row/column is empty and iterateOnlyExistingCells is true. However,
that does not seem like a reasonable action. This situation is analagous to
iterating over an empty array, and that action is legal and does not throw.
The same should apply here. There were no tests for this situation,
and now there are.

I have added additional tests, and coverage for all of RowCellIterator,
ColumnCellIterator, and CellIterator are all now 100%. Some of my new tests
were added in new members, because the existing tests all relied on mocking,
which was not the best choice for the new tests. One of the existing tests
for RowCellIteratorTest (testSeekOutOfRange) was wrong; it issued the expected
exception, but for the wrong reason. I have added an additional test to
ensure that it fails "correctly".

The existing documentation says that the default value for
IterateOnlyExistingCells is true. In fact, the default value is false.
I have corrected the documentation.

* More Scrutinizer

I believe its analysis is incorrect, but this should silence it.

* DocBlock Correction

ColumnCellIterator DocBlock for current indicated it could return null
or Cell, but it can really return only Cell. This had caused Scrutinizer
to complain earlier.

* PHP8 Environment Appears to be Fixed

Cosmetic change to Doc member. I suspect there is a way to rerun all
the tests without another push, but I have been unable to figure out how.
This commit is contained in:
oleibman 2020-12-10 09:19:56 -08:00 committed by GitHub
parent 0861370c4d
commit a8462f3864
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 342 additions and 36 deletions

View File

@ -422,8 +422,10 @@ foreach ($worksheet->getRowIterator() as $row) {
$cellIterator = $row->getCellIterator();
$cellIterator->setIterateOnlyExistingCells(FALSE); // This loops through all cells,
// even if a cell value is not set.
// By default, only cells that have a value
// set will be iterated.
// For 'TRUE', we loop through cells
// only when their value is set.
// If this method is not called,
// the default value is 'false'.
foreach ($cellIterator as $cell) {
echo '<td>' .
$cell->getValue() .

View File

@ -42,13 +42,6 @@ class Style extends Supervisor
*/
protected $numberFormat;
/**
* Conditional styles.
*
* @var Conditional[]
*/
protected $conditionalStyles;
/**
* Protection.
*
@ -85,7 +78,6 @@ class Style extends Supervisor
parent::__construct($isSupervisor);
// Initialise values
$this->conditionalStyles = [];
$this->font = new Font($isSupervisor, $isConditional);
$this->fill = new Fill($isSupervisor, $isConditional);
$this->borders = new Borders($isSupervisor, $isConditional);
@ -212,6 +204,8 @@ class Style extends Supervisor
$rangeEnd = Coordinate::coordinateFromString($rangeB);
// Translate column into index
$rangeStart0 = $rangeStart[0];
$rangeEnd0 = $rangeEnd[0];
$rangeStart[0] = Coordinate::columnIndexFromString($rangeStart[0]);
$rangeEnd[0] = Coordinate::columnIndexFromString($rangeEnd[0]);
@ -361,6 +355,13 @@ class Style extends Supervisor
for ($col = $rangeStart[0]; $col <= $rangeEnd[0]; ++$col) {
$oldXfIndexes[$this->getActiveSheet()->getColumnDimensionByColumn($col)->getXfIndex()] = true;
}
foreach ($this->getActiveSheet()->getColumnIterator($rangeStart0, $rangeEnd0) as $columnIterator) {
$cellIterator = $columnIterator->getCellIterator();
$cellIterator->setIterateOnlyExistingCells(true);
foreach ($cellIterator as $columnCell) {
$columnCell->getStyle()->applyFromArray($pStyles);
}
}
break;
case 'ROW':
@ -372,6 +373,13 @@ class Style extends Supervisor
$oldXfIndexes[$this->getActiveSheet()->getRowDimension($row)->getXfIndex()] = true;
}
}
foreach ($this->getActiveSheet()->getRowIterator((int) $rangeStart[1], (int) $rangeEnd[1]) as $rowIterator) {
$cellIterator = $rowIterator->getCellIterator();
$cellIterator->setIterateOnlyExistingCells(true);
foreach ($cellIterator as $rowCell) {
$rowCell->getStyle()->applyFromArray($pStyles);
}
}
break;
case 'CELL':
@ -599,18 +607,12 @@ class Style extends Supervisor
*/
public function getHashCode()
{
$hashConditionals = '';
foreach ($this->conditionalStyles as $conditional) {
$hashConditionals .= $conditional->getHashCode();
}
return md5(
$this->fill->getHashCode() .
$this->font->getHashCode() .
$this->borders->getHashCode() .
$this->alignment->getHashCode() .
$this->numberFormat->getHashCode() .
$hashConditionals .
$this->protection->getHashCode() .
($this->quotePrefix ? 't' : 'f') .
__CLASS__

View File

@ -92,10 +92,11 @@ class ColumnCellIterator extends CellIterator
*/
public function seek($row = 1)
{
if ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $row))) {
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
}
if (($row < $this->startRow) || ($row > $this->endRow)) {
throw new PhpSpreadsheetException("Row $row is out of range ({$this->startRow} - {$this->endRow})");
} elseif ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $row))) {
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
}
$this->currentRow = $row;
@ -113,7 +114,7 @@ class ColumnCellIterator extends CellIterator
/**
* Return the current cell in this worksheet column.
*
* @return null|\PhpOffice\PhpSpreadsheet\Cell\Cell
* @return \PhpOffice\PhpSpreadsheet\Cell\Cell
*/
public function current()
{
@ -180,18 +181,12 @@ class ColumnCellIterator extends CellIterator
) {
++$this->startRow;
}
if ($this->startRow > $this->endRow) {
throw new PhpSpreadsheetException('No cells exist within the specified range');
}
while (
(!$this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $this->endRow)) &&
($this->endRow >= $this->startRow)
) {
--$this->endRow;
}
if ($this->endRow < $this->startRow) {
throw new PhpSpreadsheetException('No cells exist within the specified range');
}
}
}
}

View File

@ -93,12 +93,14 @@ class RowCellIterator extends CellIterator
*/
public function seek($column = 'A')
{
$columnx = $column;
$column = Coordinate::columnIndexFromString($column);
if (($column < $this->startColumnIndex) || ($column > $this->endColumnIndex)) {
throw new PhpSpreadsheetException("Column $column is out of range ({$this->startColumnIndex} - {$this->endColumnIndex})");
} elseif ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($column, $this->rowIndex))) {
if ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($column, $this->rowIndex))) {
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
}
if (($column < $this->startColumnIndex) || ($column > $this->endColumnIndex)) {
throw new PhpSpreadsheetException("Column $columnx is out of range ({$this->startColumnIndex} - {$this->endColumnIndex})");
}
$this->currentColumnIndex = $column;
return $this;
@ -181,15 +183,9 @@ class RowCellIterator extends CellIterator
while ((!$this->worksheet->cellExistsByColumnAndRow($this->startColumnIndex, $this->rowIndex)) && ($this->startColumnIndex <= $this->endColumnIndex)) {
++$this->startColumnIndex;
}
if ($this->startColumnIndex > $this->endColumnIndex) {
throw new PhpSpreadsheetException('No cells exist within the specified range');
}
while ((!$this->worksheet->cellExistsByColumnAndRow($this->endColumnIndex, $this->rowIndex)) && ($this->endColumnIndex >= $this->startColumnIndex)) {
--$this->endColumnIndex;
}
if ($this->endColumnIndex < $this->startColumnIndex) {
throw new PhpSpreadsheetException('No cells exist within the specified range');
}
}
}
}

View File

@ -3,6 +3,7 @@
namespace PhpOffice\PhpSpreadsheetTests\Style;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Style\Fill;
use PHPUnit\Framework\TestCase;
class StyleTest extends TestCase
@ -19,4 +20,141 @@ class StyleTest extends TestCase
$outArray = $cell1style->getStyleArray($styleArray);
self::assertEquals($styleArray, $outArray['quotePrefix']);
}
public function testStyleColumn(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$cellCoordinates = 'A:B';
$styleArray = [
'font' => [
'bold' => true,
],
];
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
$sheet->setCellValue('A1', 'xxxa1');
$sheet->setCellValue('A2', 'xxxa2');
$sheet->setCellValue('A3', 'xxxa3');
$sheet->setCellValue('B1', 'xxxa1');
$sheet->setCellValue('B2', 'xxxa2');
$sheet->setCellValue('B3', 'xxxa3');
$sheet->setCellValue('C1', 'xxxc1');
$sheet->setCellValue('C2', 'xxxc2');
$sheet->setCellValue('C3', 'xxxc3');
$styleArray = [
'font' => [
'italic' => true,
],
];
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
self::assertTrue($sheet->getStyle('A1')->getFont()->getBold());
self::assertTrue($sheet->getStyle('B2')->getFont()->getBold());
self::assertFalse($sheet->getStyle('C3')->getFont()->getBold());
self::assertTrue($sheet->getStyle('A1')->getFont()->getItalic());
self::assertTrue($sheet->getStyle('B2')->getFont()->getItalic());
self::assertFalse($sheet->getStyle('C3')->getFont()->getItalic());
}
public function testStyleRow(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$cellCoordinates = '2:3';
$styleArray = [
'font' => [
'bold' => true,
],
];
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
$sheet->setCellValue('A1', 'xxxa1');
$sheet->setCellValue('A2', 'xxxa2');
$sheet->setCellValue('A3', 'xxxa3');
$sheet->setCellValue('B1', 'xxxa1');
$sheet->setCellValue('B2', 'xxxa2');
$sheet->setCellValue('B3', 'xxxa3');
$sheet->setCellValue('C1', 'xxxc1');
$sheet->setCellValue('C2', 'xxxc2');
$sheet->setCellValue('C3', 'xxxc3');
$styleArray = [
'font' => [
'italic' => true,
],
];
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
self::assertFalse($sheet->getStyle('A1')->getFont()->getBold());
self::assertTrue($sheet->getStyle('B2')->getFont()->getBold());
self::assertTrue($sheet->getStyle('C3')->getFont()->getBold());
self::assertFalse($sheet->getStyle('A1')->getFont()->getItalic());
self::assertTrue($sheet->getStyle('B2')->getFont()->getItalic());
self::assertTrue($sheet->getStyle('C3')->getFont()->getItalic());
}
public function testIssue1712A(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$rgb = '4467b8';
$sheet->fromArray(['OK', 'KO']);
$spreadsheet->getActiveSheet()
->getStyle('A1')
->getFill()
->setFillType(Fill::FILL_SOLID)
->getStartColor()
->setRGB($rgb);
$spreadsheet->getActiveSheet()
->getStyle('B')
->getFill()
->setFillType(Fill::FILL_SOLID)
->getStartColor()
->setRGB($rgb);
self::assertEquals($rgb, $sheet->getCell('A1')->getStyle()->getFill()->getStartColor()->getRGB());
self::assertEquals($rgb, $sheet->getCell('B1')->getStyle()->getFill()->getStartColor()->getRGB());
}
public function testIssue1712B(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$rgb = '4467b8';
$spreadsheet->getActiveSheet()
->getStyle('A1')
->getFill()
->setFillType(Fill::FILL_SOLID)
->getStartColor()
->setRGB($rgb);
$spreadsheet->getActiveSheet()
->getStyle('B')
->getFill()
->setFillType(Fill::FILL_SOLID)
->getStartColor()
->setRGB($rgb);
$sheet->fromArray(['OK', 'KO']);
self::assertEquals($rgb, $sheet->getCell('A1')->getStyle()->getFill()->getStartColor()->getRGB());
self::assertEquals($rgb, $sheet->getCell('B1')->getStyle()->getFill()->getStartColor()->getRGB());
}
public function testStyleLoopUpwards(): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$cellCoordinates = 'C5:A3';
$styleArray = [
'font' => [
'bold' => true,
],
];
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
$sheet->setCellValue('A1', 'xxxa1');
$sheet->setCellValue('A2', 'xxxa2');
$sheet->setCellValue('A3', 'xxxa3');
$sheet->setCellValue('B1', 'xxxa1');
$sheet->setCellValue('B2', 'xxxa2');
$sheet->setCellValue('B3', 'xxxa3');
$sheet->setCellValue('C1', 'xxxc1');
$sheet->setCellValue('C2', 'xxxc2');
$sheet->setCellValue('C3', 'xxxc3');
self::assertFalse($sheet->getStyle('A1')->getFont()->getBold());
self::assertFalse($sheet->getStyle('B2')->getFont()->getBold());
self::assertTrue($sheet->getStyle('C3')->getFont()->getBold());
}
}

View File

@ -0,0 +1,75 @@
<?php
namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Worksheet\ColumnCellIterator;
use PHPUnit\Framework\TestCase;
class ColumnCellIterator2Test extends TestCase
{
/**
* @dataProvider providerExistingCell
*/
public function testEndRange(?bool $existing, string $expectedResultFirst, string $expectedResultLast): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->getCell('B2')->setValue('cellb2');
$sheet->getCell('B6')->setValue('cellb6');
$iterator = new ColumnCellIterator($sheet, 'B', 1, 8);
if (isset($existing)) {
$iterator->setIterateOnlyExistingCells($existing);
}
$lastCoordinate = '';
$firstCoordinate = '';
foreach ($iterator as $cell) {
$lastCoordinate = $cell->getCoordinate();
if (!$firstCoordinate) {
$firstCoordinate = $lastCoordinate;
}
}
self::assertEquals($expectedResultFirst, $firstCoordinate);
self::assertEquals($expectedResultLast, $lastCoordinate);
}
public function providerExistingCell(): array
{
return [
[null, 'B1', 'B8'],
[false, 'B1', 'B8'],
[true, 'B2', 'B6'],
];
}
/**
* @dataProvider providerEmptyColumn
*/
public function testEmptyColumn(?bool $existing, int $expectedResult): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->getCell('B2')->setValue('cellb2');
$sheet->getCell('B6')->setValue('cellb6');
$iterator = new ColumnCellIterator($sheet, 'C');
if (isset($existing)) {
$iterator->setIterateOnlyExistingCells($existing);
}
$numCells = 0;
foreach ($iterator as $cell) {
++$numCells;
}
self::assertEquals($expectedResult, $numCells);
}
public function providerEmptyColumn(): array
{
return [
[null, 6],
[false, 6],
[true, 0],
];
}
}

View File

@ -71,11 +71,21 @@ class ColumnCellIteratorTest extends TestCase
public function testSeekOutOfRange(): void
{
$this->expectException(\PhpOffice\PhpSpreadsheet\Exception::class);
$this->expectExceptionMessage('Row 1 is out of range');
$iterator = new ColumnCellIterator($this->mockWorksheet, 'A', 2, 4);
$iterator->seek(1);
}
public function testSeekNotExisting(): void
{
$this->expectException(\PhpOffice\PhpSpreadsheet\Exception::class);
$this->expectExceptionMessage('Cell does not exist');
$iterator = new ColumnCellIterator($this->mockWorksheet, 'A', 2, 4);
$iterator->setIterateOnlyExistingCells(true);
$iterator->seek(2);
}
public function testPrevOutOfRange(): void
{
$iterator = new ColumnCellIterator($this->mockWorksheet, 'A', 2, 4);

View File

@ -0,0 +1,75 @@
<?php
namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Worksheet\RowCellIterator;
use PHPUnit\Framework\TestCase;
class RowCellIterator2Test extends TestCase
{
/**
* @dataProvider providerExistingCell
*/
public function testEndRangeTrue(?bool $existing, string $expectedResultFirst, string $expectedResultLast): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->getCell('C2')->setValue('cellb2');
$sheet->getCell('F2')->setValue('cellf2');
$iterator = new RowCellIterator($sheet, 2, 'B', 'H');
if (isset($existing)) {
$iterator->setIterateOnlyExistingCells($existing);
}
$lastCoordinate = '';
$firstCoordinate = '';
foreach ($iterator as $cell) {
$lastCoordinate = $cell->getCoordinate();
if (!$firstCoordinate) {
$firstCoordinate = $lastCoordinate;
}
}
self::assertEquals($expectedResultFirst, $firstCoordinate);
self::assertEquals($expectedResultLast, $lastCoordinate);
}
public function providerExistingCell(): array
{
return [
[null, 'B2', 'H2'],
[false, 'B2', 'H2'],
[true, 'C2', 'F2'],
];
}
/**
* @dataProvider providerEmptyRow
*/
public function testEmptyRow(?bool $existing, int $expectedResult): void
{
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->getCell('B2')->setValue('cellb2');
$sheet->getCell('F2')->setValue('cellf2');
$iterator = new RowCellIterator($sheet, '3');
if (isset($existing)) {
$iterator->setIterateOnlyExistingCells($existing);
}
$numCells = 0;
foreach ($iterator as $cell) {
++$numCells;
}
self::assertEquals($expectedResult, $numCells);
}
public function providerEmptyRow(): array
{
return [
[null, 6],
[false, 6],
[true, 0],
];
}
}

View File

@ -73,9 +73,22 @@ class RowCellIteratorTest extends TestCase
public function testSeekOutOfRange(): void
{
$this->expectException(\PhpOffice\PhpSpreadsheet\Exception::class);
$this->expectExceptionMessage('Column A is out of range');
$iterator = new RowCellIterator($this->mockWorksheet, 2, 'B', 'D');
$iterator->seek(1);
self::assertFalse($iterator->getIterateOnlyExistingCells());
self::assertEquals(2, $iterator->getCurrentColumnIndex());
$iterator->seek('A');
}
public function testSeekNotExisting(): void
{
$this->expectException(\PhpOffice\PhpSpreadsheet\Exception::class);
$this->expectExceptionMessage('Cell does not exist');
$iterator = new RowCellIterator($this->mockWorksheet, 2, 'B', 'D');
$iterator->setIterateOnlyExistingCells(true);
$iterator->seek('B');
}
public function testPrevOutOfRange(): void