Skip to content

Commit dd69858

Browse files
committed
Fill Patterns/Colors When Xml Attributes are Missing
Fix PHPOffice#4248. PhpSpreadsheet has used what appear to be default attributes and tags when they are missing from Fill patterns and colors. However, Excel handles their absence a little differently from what the "default" would require. PhpSpreadsheet is changed to omit the attributes and tags in question when missing. This change is mostly targeted towards Xlsx read and write, but minor changes for Xls and Html write are also included. This seems like it could be a breaking change, but I don't think it is. One test (DefaultFillTest introduced by PR PHPOffice#2050) must change, but the change is internal - loading and then saving the spreadsheet used in that change will appear the same after this change as it did before. Other differences are very likely to be bug fixes rather than breaks.
1 parent f37b119 commit dd69858

File tree

9 files changed

+172
-19
lines changed

9 files changed

+172
-19
lines changed

src/PhpSpreadsheet/Reader/Xlsx/Styles.php

+27-3
Original file line numberDiff line numberDiff line change
@@ -149,14 +149,22 @@ public function readFillStyle(Fill $fillStyle, SimpleXMLElement $fillStyleXml):
149149
$fillStyle->getStartColor()->setARGB($this->readColor(self::getArrayItem($gradientFill->xpath('sml:stop[@position=0]'))->color)); //* @phpstan-ignore-line
150150
$fillStyle->getEndColor()->setARGB($this->readColor(self::getArrayItem($gradientFill->xpath('sml:stop[@position=1]'))->color)); //* @phpstan-ignore-line
151151
} elseif ($fillStyleXml->patternFill) {
152-
$defaultFillStyle = Fill::FILL_NONE;
152+
$defaultFillStyle = ($fillStyle->getFillType() !== null) ? Fill::FILL_NONE : '';
153+
$fgFound = false;
154+
$bgFound = false;
153155
if ($fillStyleXml->patternFill->fgColor) {
154156
$fillStyle->getStartColor()->setARGB($this->readColor($fillStyleXml->patternFill->fgColor, true));
155-
$defaultFillStyle = Fill::FILL_SOLID;
157+
if ($fillStyle->getFillType() !== null) {
158+
$defaultFillStyle = Fill::FILL_SOLID;
159+
}
160+
$fgFound = true;
156161
}
157162
if ($fillStyleXml->patternFill->bgColor) {
158163
$fillStyle->getEndColor()->setARGB($this->readColor($fillStyleXml->patternFill->bgColor, true));
159-
$defaultFillStyle = Fill::FILL_SOLID;
164+
if ($fillStyle->getFillType() !== null) {
165+
$defaultFillStyle = Fill::FILL_SOLID;
166+
}
167+
$bgFound = true;
160168
}
161169

162170
$type = '';
@@ -169,6 +177,22 @@ public function readFillStyle(Fill $fillStyle, SimpleXMLElement $fillStyleXml):
169177
$patternType = ($type === '') ? $defaultFillStyle : $type;
170178

171179
$fillStyle->setFillType($patternType);
180+
if (
181+
!$fgFound // no foreground color specified
182+
&& !in_array($patternType, [Fill::FILL_NONE, Fill::FILL_SOLID], true) // these patterns aren't relevant
183+
&& $fillStyle->getStartColor()->getARGB() // not conditional
184+
) {
185+
$fillStyle->getStartColor()
186+
->setARGB('', true);
187+
}
188+
if (
189+
!$bgFound // no background color specified
190+
&& !in_array($patternType, [Fill::FILL_NONE, Fill::FILL_SOLID], true) // these patterns aren't relevant
191+
&& $fillStyle->getEndColor()->getARGB() // not conditional
192+
) {
193+
$fillStyle->getEndColor()
194+
->setARGB('', true);
195+
}
172196
}
173197
}
174198

src/PhpSpreadsheet/Style/Color.php

+6-4
Original file line numberDiff line numberDiff line change
@@ -230,12 +230,14 @@ public function getARGB(): ?string
230230
*
231231
* @return $this
232232
*/
233-
public function setARGB(?string $colorValue = self::COLOR_BLACK): static
233+
public function setARGB(?string $colorValue = self::COLOR_BLACK, bool $nullStringOkay = false): static
234234
{
235235
$this->hasChanged = true;
236-
$colorValue = $this->validateColor($colorValue);
237-
if ($colorValue === '') {
238-
return $this;
236+
if (!$nullStringOkay || $colorValue !== '') {
237+
$colorValue = $this->validateColor($colorValue);
238+
if ($colorValue === '') {
239+
return $this;
240+
}
239241
}
240242

241243
if ($this->isSupervisor) {

src/PhpSpreadsheet/Writer/Html.php

+10-3
Original file line numberDiff line numberDiff line change
@@ -1139,9 +1139,16 @@ private function createCSSStyleFill(Fill $fill): array
11391139

11401140
// Create CSS
11411141
if ($fill->getFillType() !== Fill::FILL_NONE) {
1142-
$value = $fill->getFillType() == Fill::FILL_NONE
1143-
? 'white' : '#' . $fill->getStartColor()->getRGB();
1144-
$css['background-color'] = $value;
1142+
if (
1143+
(in_array($fill->getFillType(), ['', Fill::FILL_SOLID], true) || !$fill->getEndColor()->getRGB())
1144+
&& $fill->getStartColor()->getRGB()
1145+
) {
1146+
$value = '#' . $fill->getStartColor()->getRGB();
1147+
$css['background-color'] = $value;
1148+
} elseif ($fill->getEndColor()->getRGB()) {
1149+
$value = '#' . $fill->getEndColor()->getRGB();
1150+
$css['background-color'] = $value;
1151+
}
11451152
}
11461153

11471154
return $css;

src/PhpSpreadsheet/Writer/Xls/Workbook.php

+14-2
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,20 @@ public function addXfWriter(Style $style, bool $isStyleXf = false): int
210210
$xfWriter->setFontIndex($fontIndex);
211211

212212
// Background colors, best to treat these after the font so black will come after white in custom palette
213-
$xfWriter->setFgColor($this->addColor($style->getFill()->getStartColor()->getRGB()));
214-
$xfWriter->setBgColor($this->addColor($style->getFill()->getEndColor()->getRGB()));
213+
if ($style->getFill()->getStartColor()->getRGB()) {
214+
$xfWriter->setFgColor(
215+
$this->addColor(
216+
$style->getFill()->getStartColor()->getRGB()
217+
)
218+
);
219+
}
220+
if ($style->getFill()->getEndColor()->getRGB()) {
221+
$xfWriter->setBgColor(
222+
$this->addColor(
223+
$style->getFill()->getEndColor()->getRGB()
224+
)
225+
);
226+
}
215227
$xfWriter->setBottomColor($this->addColor($style->getBorders()->getBottom()->getColor()->getRGB()));
216228
$xfWriter->setTopColor($this->addColor($style->getBorders()->getTop()->getColor()->getRGB()));
217229
$xfWriter->setRightColor($this->addColor($style->getBorders()->getRight()->getColor()->getRGB()));

src/PhpSpreadsheet/Writer/Xls/Worksheet.php

+3-3
Original file line numberDiff line numberDiff line change
@@ -2875,9 +2875,9 @@ private function writeCFRule(
28752875
$bFormatBorder = 0;
28762876
}
28772877
// Pattern
2878-
$bFillStyle = ($conditional->getStyle()->getFill()->getFillType() === null ? 0 : 1);
2879-
$bFillColor = ($conditional->getStyle()->getFill()->getStartColor()->getARGB() === null ? 0 : 1);
2880-
$bFillColorBg = ($conditional->getStyle()->getFill()->getEndColor()->getARGB() === null ? 0 : 1);
2878+
$bFillStyle = ($conditional->getStyle()->getFill()->getFillType() == null ? 0 : 1);
2879+
$bFillColor = ($conditional->getStyle()->getFill()->getStartColor()->getARGB() == null ? 0 : 1);
2880+
$bFillColorBg = ($conditional->getStyle()->getFill()->getEndColor()->getARGB() == null ? 0 : 1);
28812881
if ($bFillStyle == 1 || $bFillColor == 1 || $bFillColorBg == 1) {
28822882
$bFormatFill = 1;
28832883
} else {

src/PhpSpreadsheet/Writer/Xlsx/Style.php

+5-3
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ private function writeGradientFill(XMLWriter $objWriter, Fill $fill): void
199199
$objWriter->writeAttribute('position', '0');
200200

201201
// color
202-
if ($fill->getStartColor()->getARGB() !== null) {
202+
if (!empty($fill->getStartColor()->getARGB())) {
203203
$objWriter->startElement('color');
204204
$objWriter->writeAttribute('rgb', $fill->getStartColor()->getARGB());
205205
$objWriter->endElement();
@@ -212,7 +212,7 @@ private function writeGradientFill(XMLWriter $objWriter, Fill $fill): void
212212
$objWriter->writeAttribute('position', '1');
213213

214214
// color
215-
if ($fill->getEndColor()->getARGB() !== null) {
215+
if (!empty($fill->getEndColor()->getARGB())) {
216216
$objWriter->startElement('color');
217217
$objWriter->writeAttribute('rgb', $fill->getEndColor()->getARGB());
218218
$objWriter->endElement();
@@ -244,7 +244,9 @@ private function writePatternFill(XMLWriter $objWriter, Fill $fill): void
244244

245245
// patternFill
246246
$objWriter->startElement('patternFill');
247-
$objWriter->writeAttribute('patternType', (string) $fill->getFillType());
247+
if ($fill->getFillType()) {
248+
$objWriter->writeAttribute('patternType', (string) $fill->getFillType());
249+
}
248250

249251
if (self::writePatternColors($fill)) {
250252
// fgColor

tests/PhpSpreadsheetTests/Reader/Xlsx/DefaultFillTest.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,6 @@ public function testDefaultConditionalFill(): void
4040
$spreadsheet = $reader->load($filename);
4141

4242
$style = $spreadsheet->getActiveSheet()->getConditionalStyles('A1')[0]->getStyle();
43-
self::assertSame('solid', $style->getFill()->getFillType());
43+
self::assertSame('', $style->getFill()->getFillType());
4444
}
4545
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
6+
7+
use PhpOffice\PhpSpreadsheet\Reader\Xlsx as XlsxReader;
8+
use PhpOffice\PhpSpreadsheet\Shared\File;
9+
use PhpOffice\PhpSpreadsheet\Writer\Html as HtmlWriter;
10+
use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;
11+
use PHPUnit\Framework\TestCase;
12+
13+
class Issue4248Test extends TestCase
14+
{
15+
private string $outfile = '';
16+
17+
protected function tearDown(): void
18+
{
19+
if ($this->outfile !== '') {
20+
unlink($this->outfile);
21+
$this->outfile = '';
22+
}
23+
}
24+
25+
public function testStyles(): void
26+
{
27+
$file = 'tests/data/Reader/XLSX/issue.4248.xlsx';
28+
$reader = new XlsxReader();
29+
$spreadsheet = $reader->load($file);
30+
$sheet = $spreadsheet->getActiveSheet();
31+
$writer = new XlsxWriter($spreadsheet);
32+
$this->outfile = File::temporaryFilename();
33+
$writer->save($this->outfile);
34+
$spreadsheet->disconnectWorksheets();
35+
36+
$file = 'zip://';
37+
$file .= $this->outfile;
38+
$file .= '#xl/styles.xml';
39+
$data = file_get_contents($file) ?: '';
40+
$expected = '<fill>'
41+
. '<patternFill patternType="darkDown"/>'
42+
. '</fill>';
43+
self::assertStringContainsString($expected, $data, 'neither fgColor nor bgColor');
44+
$expected = '<fill>'
45+
. '<patternFill patternType="darkDown">'
46+
. '<bgColor rgb="FFBDD7EE"/>'
47+
. '</patternFill></fill>';
48+
self::assertStringContainsString($expected, $data, 'bgColor but no fgColor');
49+
$expected = '<dxfs count="15">'
50+
. '<dxf>' // dxfId 1 - fill color for Oui
51+
. '<fill>'
52+
. '<patternFill><bgColor rgb="FF00B050"/></patternFill>'
53+
. '</fill>'
54+
. '<border/>'
55+
. '</dxf>'
56+
. '<dxf>' // dxfId 2 - fill color for Non
57+
. '<font><color rgb="FF9C0006"/></font>'
58+
. '<fill>'
59+
. '<patternFill><bgColor rgb="FFFFC7CE"/></patternFill>'
60+
. '</fill>'
61+
. '<border/>'
62+
. '</dxf>';
63+
self::assertStringContainsString($expected, $data, 'conditional fill styles');
64+
65+
$file = 'zip://';
66+
$file .= $this->outfile;
67+
$file .= '#xl/worksheets/sheet1.xml';
68+
$data = file_get_contents($file) ?: '';
69+
$expected = '<conditionalFormatting sqref="C16:C38 E17:H18 I17:J37 D18 J23:J38 E38 I38">'
70+
. '<cfRule type="containsText" dxfId="0" priority="1" operator="containsText" text="Oui">'
71+
. '<formula>NOT(ISERROR(SEARCH(&quot;Oui&quot;,C16)))</formula>'
72+
. '</cfRule>'
73+
. '</conditionalFormatting>';
74+
self::assertStringContainsString($expected, $data, 'first condition for D18');
75+
$expected = '<conditionalFormatting sqref="C16:C38 I17:J37 E17:H18 D18 J23:J38 E38 I38">'
76+
. '<cfRule type="containsText" dxfId="1" priority="2" operator="containsText" text="Non">'
77+
. '<formula>NOT(ISERROR(SEARCH(&quot;Non&quot;,C16)))</formula>'
78+
. '</cfRule>'
79+
. '</conditionalFormatting>';
80+
self::assertStringContainsString($expected, $data, 'second condition for D18');
81+
}
82+
83+
public function testHtml(): void
84+
{
85+
$file = 'tests/data/Reader/XLSX/issue.4248.xlsx';
86+
$reader = new XlsxReader();
87+
$spreadsheet = $reader->load($file);
88+
$sheet = $spreadsheet->getActiveSheet();
89+
$writer = new HtmlWriter($spreadsheet);
90+
91+
$file = 'zip://';
92+
$file .= $this->outfile;
93+
$file .= '#xl/styles.xml';
94+
$data = str_replace(["\r", "\n"], '', $writer->generateHtmlAll());
95+
$expected = ' <tr class="row17">' // Cell D18
96+
. ' <td class="column0 style0">&nbsp;</td>'
97+
. ' <td class="column1 style28 null"></td>'
98+
. ' <td class="column2 style35 s">Eligible </td>'
99+
. ' <td class="column3 style70 f">Non</td>';
100+
self::assertStringContainsString($expected, $data, 'Cell D18 style');
101+
$expected = ' td.style70, th.style70 { vertical-align:middle; text-align:center; border-bottom:1px solid #000000 !important; border-top:2px solid #000000 !important; border-left:2px solid #000000 !important; border-right:1px solid #000000 !important; font-weight:bold; color:#000000; font-family:\'Calibri\'; font-size:16pt; background-color:#BDD7EE }';
102+
self::assertStringContainsString($expected, $data, 'background color');
103+
104+
$spreadsheet->disconnectWorksheets();
105+
}
106+
}
696 KB
Binary file not shown.

0 commit comments

Comments
 (0)