Skip to content

Commit a8462f3

Browse files
authored
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.
1 parent 0861370 commit a8462f3

File tree

9 files changed

+342
-36
lines changed

9 files changed

+342
-36
lines changed

docs/topics/accessing-cells.md

+4-2
Original file line numberDiff line numberDiff line change
@@ -422,8 +422,10 @@ foreach ($worksheet->getRowIterator() as $row) {
422422
$cellIterator = $row->getCellIterator();
423423
$cellIterator->setIterateOnlyExistingCells(FALSE); // This loops through all cells,
424424
// even if a cell value is not set.
425-
// By default, only cells that have a value
426-
// set will be iterated.
425+
// For 'TRUE', we loop through cells
426+
// only when their value is set.
427+
// If this method is not called,
428+
// the default value is 'false'.
427429
foreach ($cellIterator as $cell) {
428430
echo '<td>' .
429431
$cell->getValue() .

src/PhpSpreadsheet/Style/Style.php

+16-14
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,6 @@ class Style extends Supervisor
4242
*/
4343
protected $numberFormat;
4444

45-
/**
46-
* Conditional styles.
47-
*
48-
* @var Conditional[]
49-
*/
50-
protected $conditionalStyles;
51-
5245
/**
5346
* Protection.
5447
*
@@ -85,7 +78,6 @@ public function __construct($isSupervisor = false, $isConditional = false)
8578
parent::__construct($isSupervisor);
8679

8780
// Initialise values
88-
$this->conditionalStyles = [];
8981
$this->font = new Font($isSupervisor, $isConditional);
9082
$this->fill = new Fill($isSupervisor, $isConditional);
9183
$this->borders = new Borders($isSupervisor, $isConditional);
@@ -212,6 +204,8 @@ public function applyFromArray(array $pStyles, $pAdvanced = true)
212204
$rangeEnd = Coordinate::coordinateFromString($rangeB);
213205

214206
// Translate column into index
207+
$rangeStart0 = $rangeStart[0];
208+
$rangeEnd0 = $rangeEnd[0];
215209
$rangeStart[0] = Coordinate::columnIndexFromString($rangeStart[0]);
216210
$rangeEnd[0] = Coordinate::columnIndexFromString($rangeEnd[0]);
217211

@@ -361,6 +355,13 @@ public function applyFromArray(array $pStyles, $pAdvanced = true)
361355
for ($col = $rangeStart[0]; $col <= $rangeEnd[0]; ++$col) {
362356
$oldXfIndexes[$this->getActiveSheet()->getColumnDimensionByColumn($col)->getXfIndex()] = true;
363357
}
358+
foreach ($this->getActiveSheet()->getColumnIterator($rangeStart0, $rangeEnd0) as $columnIterator) {
359+
$cellIterator = $columnIterator->getCellIterator();
360+
$cellIterator->setIterateOnlyExistingCells(true);
361+
foreach ($cellIterator as $columnCell) {
362+
$columnCell->getStyle()->applyFromArray($pStyles);
363+
}
364+
}
364365

365366
break;
366367
case 'ROW':
@@ -372,6 +373,13 @@ public function applyFromArray(array $pStyles, $pAdvanced = true)
372373
$oldXfIndexes[$this->getActiveSheet()->getRowDimension($row)->getXfIndex()] = true;
373374
}
374375
}
376+
foreach ($this->getActiveSheet()->getRowIterator((int) $rangeStart[1], (int) $rangeEnd[1]) as $rowIterator) {
377+
$cellIterator = $rowIterator->getCellIterator();
378+
$cellIterator->setIterateOnlyExistingCells(true);
379+
foreach ($cellIterator as $rowCell) {
380+
$rowCell->getStyle()->applyFromArray($pStyles);
381+
}
382+
}
375383

376384
break;
377385
case 'CELL':
@@ -599,18 +607,12 @@ public function setQuotePrefix($pValue)
599607
*/
600608
public function getHashCode()
601609
{
602-
$hashConditionals = '';
603-
foreach ($this->conditionalStyles as $conditional) {
604-
$hashConditionals .= $conditional->getHashCode();
605-
}
606-
607610
return md5(
608611
$this->fill->getHashCode() .
609612
$this->font->getHashCode() .
610613
$this->borders->getHashCode() .
611614
$this->alignment->getHashCode() .
612615
$this->numberFormat->getHashCode() .
613-
$hashConditionals .
614616
$this->protection->getHashCode() .
615617
($this->quotePrefix ? 't' : 'f') .
616618
__CLASS__

src/PhpSpreadsheet/Worksheet/ColumnCellIterator.php

+4-9
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,11 @@ public function resetEnd($endRow = null)
9292
*/
9393
public function seek($row = 1)
9494
{
95+
if ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $row))) {
96+
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
97+
}
9598
if (($row < $this->startRow) || ($row > $this->endRow)) {
9699
throw new PhpSpreadsheetException("Row $row is out of range ({$this->startRow} - {$this->endRow})");
97-
} elseif ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $row))) {
98-
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
99100
}
100101
$this->currentRow = $row;
101102

@@ -113,7 +114,7 @@ public function rewind(): void
113114
/**
114115
* Return the current cell in this worksheet column.
115116
*
116-
* @return null|\PhpOffice\PhpSpreadsheet\Cell\Cell
117+
* @return \PhpOffice\PhpSpreadsheet\Cell\Cell
117118
*/
118119
public function current()
119120
{
@@ -180,18 +181,12 @@ protected function adjustForExistingOnlyRange(): void
180181
) {
181182
++$this->startRow;
182183
}
183-
if ($this->startRow > $this->endRow) {
184-
throw new PhpSpreadsheetException('No cells exist within the specified range');
185-
}
186184
while (
187185
(!$this->worksheet->cellExistsByColumnAndRow($this->columnIndex, $this->endRow)) &&
188186
($this->endRow >= $this->startRow)
189187
) {
190188
--$this->endRow;
191189
}
192-
if ($this->endRow < $this->startRow) {
193-
throw new PhpSpreadsheetException('No cells exist within the specified range');
194-
}
195190
}
196191
}
197192
}

src/PhpSpreadsheet/Worksheet/RowCellIterator.php

+5-9
Original file line numberDiff line numberDiff line change
@@ -93,12 +93,14 @@ public function resetEnd($endColumn = null)
9393
*/
9494
public function seek($column = 'A')
9595
{
96+
$columnx = $column;
9697
$column = Coordinate::columnIndexFromString($column);
97-
if (($column < $this->startColumnIndex) || ($column > $this->endColumnIndex)) {
98-
throw new PhpSpreadsheetException("Column $column is out of range ({$this->startColumnIndex} - {$this->endColumnIndex})");
99-
} elseif ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($column, $this->rowIndex))) {
98+
if ($this->onlyExistingCells && !($this->worksheet->cellExistsByColumnAndRow($column, $this->rowIndex))) {
10099
throw new PhpSpreadsheetException('In "IterateOnlyExistingCells" mode and Cell does not exist');
101100
}
101+
if (($column < $this->startColumnIndex) || ($column > $this->endColumnIndex)) {
102+
throw new PhpSpreadsheetException("Column $columnx is out of range ({$this->startColumnIndex} - {$this->endColumnIndex})");
103+
}
102104
$this->currentColumnIndex = $column;
103105

104106
return $this;
@@ -181,15 +183,9 @@ protected function adjustForExistingOnlyRange(): void
181183
while ((!$this->worksheet->cellExistsByColumnAndRow($this->startColumnIndex, $this->rowIndex)) && ($this->startColumnIndex <= $this->endColumnIndex)) {
182184
++$this->startColumnIndex;
183185
}
184-
if ($this->startColumnIndex > $this->endColumnIndex) {
185-
throw new PhpSpreadsheetException('No cells exist within the specified range');
186-
}
187186
while ((!$this->worksheet->cellExistsByColumnAndRow($this->endColumnIndex, $this->rowIndex)) && ($this->endColumnIndex >= $this->startColumnIndex)) {
188187
--$this->endColumnIndex;
189188
}
190-
if ($this->endColumnIndex < $this->startColumnIndex) {
191-
throw new PhpSpreadsheetException('No cells exist within the specified range');
192-
}
193189
}
194190
}
195191
}

tests/PhpSpreadsheetTests/Style/StyleTest.php

+138
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace PhpOffice\PhpSpreadsheetTests\Style;
44

55
use PhpOffice\PhpSpreadsheet\Spreadsheet;
6+
use PhpOffice\PhpSpreadsheet\Style\Fill;
67
use PHPUnit\Framework\TestCase;
78

89
class StyleTest extends TestCase
@@ -19,4 +20,141 @@ public function testStyleOddMethods(): void
1920
$outArray = $cell1style->getStyleArray($styleArray);
2021
self::assertEquals($styleArray, $outArray['quotePrefix']);
2122
}
23+
24+
public function testStyleColumn(): void
25+
{
26+
$spreadsheet = new Spreadsheet();
27+
$sheet = $spreadsheet->getActiveSheet();
28+
$cellCoordinates = 'A:B';
29+
$styleArray = [
30+
'font' => [
31+
'bold' => true,
32+
],
33+
];
34+
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
35+
$sheet->setCellValue('A1', 'xxxa1');
36+
$sheet->setCellValue('A2', 'xxxa2');
37+
$sheet->setCellValue('A3', 'xxxa3');
38+
$sheet->setCellValue('B1', 'xxxa1');
39+
$sheet->setCellValue('B2', 'xxxa2');
40+
$sheet->setCellValue('B3', 'xxxa3');
41+
$sheet->setCellValue('C1', 'xxxc1');
42+
$sheet->setCellValue('C2', 'xxxc2');
43+
$sheet->setCellValue('C3', 'xxxc3');
44+
$styleArray = [
45+
'font' => [
46+
'italic' => true,
47+
],
48+
];
49+
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
50+
self::assertTrue($sheet->getStyle('A1')->getFont()->getBold());
51+
self::assertTrue($sheet->getStyle('B2')->getFont()->getBold());
52+
self::assertFalse($sheet->getStyle('C3')->getFont()->getBold());
53+
self::assertTrue($sheet->getStyle('A1')->getFont()->getItalic());
54+
self::assertTrue($sheet->getStyle('B2')->getFont()->getItalic());
55+
self::assertFalse($sheet->getStyle('C3')->getFont()->getItalic());
56+
}
57+
58+
public function testStyleRow(): void
59+
{
60+
$spreadsheet = new Spreadsheet();
61+
$sheet = $spreadsheet->getActiveSheet();
62+
$cellCoordinates = '2:3';
63+
$styleArray = [
64+
'font' => [
65+
'bold' => true,
66+
],
67+
];
68+
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
69+
$sheet->setCellValue('A1', 'xxxa1');
70+
$sheet->setCellValue('A2', 'xxxa2');
71+
$sheet->setCellValue('A3', 'xxxa3');
72+
$sheet->setCellValue('B1', 'xxxa1');
73+
$sheet->setCellValue('B2', 'xxxa2');
74+
$sheet->setCellValue('B3', 'xxxa3');
75+
$sheet->setCellValue('C1', 'xxxc1');
76+
$sheet->setCellValue('C2', 'xxxc2');
77+
$sheet->setCellValue('C3', 'xxxc3');
78+
$styleArray = [
79+
'font' => [
80+
'italic' => true,
81+
],
82+
];
83+
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
84+
self::assertFalse($sheet->getStyle('A1')->getFont()->getBold());
85+
self::assertTrue($sheet->getStyle('B2')->getFont()->getBold());
86+
self::assertTrue($sheet->getStyle('C3')->getFont()->getBold());
87+
self::assertFalse($sheet->getStyle('A1')->getFont()->getItalic());
88+
self::assertTrue($sheet->getStyle('B2')->getFont()->getItalic());
89+
self::assertTrue($sheet->getStyle('C3')->getFont()->getItalic());
90+
}
91+
92+
public function testIssue1712A(): void
93+
{
94+
$spreadsheet = new Spreadsheet();
95+
$sheet = $spreadsheet->getActiveSheet();
96+
$rgb = '4467b8';
97+
$sheet->fromArray(['OK', 'KO']);
98+
$spreadsheet->getActiveSheet()
99+
->getStyle('A1')
100+
->getFill()
101+
->setFillType(Fill::FILL_SOLID)
102+
->getStartColor()
103+
->setRGB($rgb);
104+
$spreadsheet->getActiveSheet()
105+
->getStyle('B')
106+
->getFill()
107+
->setFillType(Fill::FILL_SOLID)
108+
->getStartColor()
109+
->setRGB($rgb);
110+
self::assertEquals($rgb, $sheet->getCell('A1')->getStyle()->getFill()->getStartColor()->getRGB());
111+
self::assertEquals($rgb, $sheet->getCell('B1')->getStyle()->getFill()->getStartColor()->getRGB());
112+
}
113+
114+
public function testIssue1712B(): void
115+
{
116+
$spreadsheet = new Spreadsheet();
117+
$sheet = $spreadsheet->getActiveSheet();
118+
$rgb = '4467b8';
119+
$spreadsheet->getActiveSheet()
120+
->getStyle('A1')
121+
->getFill()
122+
->setFillType(Fill::FILL_SOLID)
123+
->getStartColor()
124+
->setRGB($rgb);
125+
$spreadsheet->getActiveSheet()
126+
->getStyle('B')
127+
->getFill()
128+
->setFillType(Fill::FILL_SOLID)
129+
->getStartColor()
130+
->setRGB($rgb);
131+
$sheet->fromArray(['OK', 'KO']);
132+
self::assertEquals($rgb, $sheet->getCell('A1')->getStyle()->getFill()->getStartColor()->getRGB());
133+
self::assertEquals($rgb, $sheet->getCell('B1')->getStyle()->getFill()->getStartColor()->getRGB());
134+
}
135+
136+
public function testStyleLoopUpwards(): void
137+
{
138+
$spreadsheet = new Spreadsheet();
139+
$sheet = $spreadsheet->getActiveSheet();
140+
$cellCoordinates = 'C5:A3';
141+
$styleArray = [
142+
'font' => [
143+
'bold' => true,
144+
],
145+
];
146+
$sheet->getStyle($cellCoordinates)->applyFromArray($styleArray);
147+
$sheet->setCellValue('A1', 'xxxa1');
148+
$sheet->setCellValue('A2', 'xxxa2');
149+
$sheet->setCellValue('A3', 'xxxa3');
150+
$sheet->setCellValue('B1', 'xxxa1');
151+
$sheet->setCellValue('B2', 'xxxa2');
152+
$sheet->setCellValue('B3', 'xxxa3');
153+
$sheet->setCellValue('C1', 'xxxc1');
154+
$sheet->setCellValue('C2', 'xxxc2');
155+
$sheet->setCellValue('C3', 'xxxc3');
156+
self::assertFalse($sheet->getStyle('A1')->getFont()->getBold());
157+
self::assertFalse($sheet->getStyle('B2')->getFont()->getBold());
158+
self::assertTrue($sheet->getStyle('C3')->getFont()->getBold());
159+
}
22160
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
3+
namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
4+
5+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
6+
use PhpOffice\PhpSpreadsheet\Worksheet\ColumnCellIterator;
7+
use PHPUnit\Framework\TestCase;
8+
9+
class ColumnCellIterator2Test extends TestCase
10+
{
11+
/**
12+
* @dataProvider providerExistingCell
13+
*/
14+
public function testEndRange(?bool $existing, string $expectedResultFirst, string $expectedResultLast): void
15+
{
16+
$spreadsheet = new Spreadsheet();
17+
$sheet = $spreadsheet->getActiveSheet();
18+
$sheet->getCell('B2')->setValue('cellb2');
19+
$sheet->getCell('B6')->setValue('cellb6');
20+
21+
$iterator = new ColumnCellIterator($sheet, 'B', 1, 8);
22+
if (isset($existing)) {
23+
$iterator->setIterateOnlyExistingCells($existing);
24+
}
25+
$lastCoordinate = '';
26+
$firstCoordinate = '';
27+
foreach ($iterator as $cell) {
28+
$lastCoordinate = $cell->getCoordinate();
29+
if (!$firstCoordinate) {
30+
$firstCoordinate = $lastCoordinate;
31+
}
32+
}
33+
self::assertEquals($expectedResultFirst, $firstCoordinate);
34+
self::assertEquals($expectedResultLast, $lastCoordinate);
35+
}
36+
37+
public function providerExistingCell(): array
38+
{
39+
return [
40+
[null, 'B1', 'B8'],
41+
[false, 'B1', 'B8'],
42+
[true, 'B2', 'B6'],
43+
];
44+
}
45+
46+
/**
47+
* @dataProvider providerEmptyColumn
48+
*/
49+
public function testEmptyColumn(?bool $existing, int $expectedResult): void
50+
{
51+
$spreadsheet = new Spreadsheet();
52+
$sheet = $spreadsheet->getActiveSheet();
53+
$sheet->getCell('B2')->setValue('cellb2');
54+
$sheet->getCell('B6')->setValue('cellb6');
55+
56+
$iterator = new ColumnCellIterator($sheet, 'C');
57+
if (isset($existing)) {
58+
$iterator->setIterateOnlyExistingCells($existing);
59+
}
60+
$numCells = 0;
61+
foreach ($iterator as $cell) {
62+
++$numCells;
63+
}
64+
self::assertEquals($expectedResult, $numCells);
65+
}
66+
67+
public function providerEmptyColumn(): array
68+
{
69+
return [
70+
[null, 6],
71+
[false, 6],
72+
[true, 0],
73+
];
74+
}
75+
}

0 commit comments

Comments
 (0)