Skip to content

Commit d8bafbf

Browse files
oleibmanoleibman
committed
Fix Several Problems in a Very Complicated Spreadsheet
Fix PHPOffice#3679. That issue was opened for a problem which was already solved by PR PHPOffice#3659, however there were additional problems with the spreadsheet. The main problem is that Data Validations and Conditional Styles can each be supplied in the Xml in either "external" or "internal" formats. The code for each to handle "external" assumes that each is the only "external" item on the worksheet in the Xml, but some of the worksheets in the sample spreadsheet provide both as "external" on some sheets. The code to fix this is verified against the supplied sample, however no formal test has been added for it. The sample is much too large and complicated to be added to the test suite - it takes several minutes to read, and even longer to write (`setPreCalculateFormulas(false)` is highly recommended). I will leave this ticket open for a few days to see if I can hand-craft a suitable test case, but I am not hopeful. A second problem is that something in the Xlsx Reader `$xmlSheetNS->sheetData->row` loop breaks the selected cell for the worksheet. This is easily fixed and verified by eye (and with the supplied sample), but, again, no explicit test case is added. A third problem is that drawings which are part of the supplied sample use `srcRect` tags in the Xml to effectively produce a cropped version of the image. This tag has hitherto been ignored. It is now supported in Xlsx Reader, Xlsx Writer, and Worksheet/BaseDrawing object. This is again verified with the supplied sample; unlike the other parts, it was easy to add a new formal test case for this part of the fix.
1 parent 4971801 commit d8bafbf

File tree

6 files changed

+107
-23
lines changed

6 files changed

+107
-23
lines changed

‎src/PhpSpreadsheet/Reader/Xlsx.php

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -832,6 +832,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
832832
->load($this->getReadFilter(), $this->getReadDataOnly());
833833
}
834834

835+
$holdSelectedCells = $docSheet->getSelectedCells();
835836
if ($xmlSheetNS && $xmlSheetNS->sheetData && $xmlSheetNS->sheetData->row) {
836837
$cIndex = 1; // Cell Start from 1
837838
foreach ($xmlSheetNS->sheetData->row as $row) {
@@ -968,6 +969,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
968969
++$cIndex;
969970
}
970971
}
972+
$docSheet->setSelectedCells($holdSelectedCells);
971973
if ($xmlSheetNS && $xmlSheetNS->ignoredErrors) {
972974
foreach ($xmlSheetNS->ignoredErrors->ignoredError as $ignoredErrorx) {
973975
$ignoredError = self::testSimpleXml($ignoredErrorx);
@@ -1007,23 +1009,30 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
10071009
$unparsedLoadedData = (new PageSetup($docSheet, $xmlSheet))->load($unparsedLoadedData);
10081010
}
10091011

1010-
if ($xmlSheet !== false && isset($xmlSheet->extLst, $xmlSheet->extLst->ext, $xmlSheet->extLst->ext['uri']) && ($xmlSheet->extLst->ext['uri'] == '{CCE6A557-97BC-4b89-ADB6-D9C93CAAB3DF}')) {
1011-
// Create dataValidations node if does not exists, maybe is better inside the foreach ?
1012-
if (!$xmlSheet->dataValidations) {
1013-
$xmlSheet->addChild('dataValidations');
1014-
}
1015-
1016-
foreach ($xmlSheet->extLst->ext->children(Namespaces::DATA_VALIDATIONS1)->dataValidations->dataValidation as $item) {
1017-
$item = self::testSimpleXml($item);
1018-
$node = self::testSimpleXml($xmlSheet->dataValidations)->addChild('dataValidation');
1019-
foreach ($item->attributes() ?? [] as $attr) {
1020-
$node->addAttribute($attr->getName(), $attr);
1012+
if ($xmlSheet !== false && isset($xmlSheet->extLst->ext)) {
1013+
foreach ($xmlSheet->extLst->ext as $extlst) {
1014+
$extAttrs = $extlst->/** @scrutinizer ignore-call */ attributes() ?? [];
1015+
$extUri = (string) ($extAttrs['uri'] ?? '');
1016+
if ($extUri !== '{CCE6A557-97BC-4b89-ADB6-D9C93CAAB3DF}') {
1017+
continue;
10211018
}
1022-
$node->addAttribute('sqref', $item->children(Namespaces::DATA_VALIDATIONS2)->sqref);
1023-
if (isset($item->formula1)) {
1024-
$childNode = $node->addChild('formula1');
1025-
if ($childNode !== null) { // null should never happen
1026-
$childNode[0] = (string) $item->formula1->children(Namespaces::DATA_VALIDATIONS2)->f; // @phpstan-ignore-line
1019+
// Create dataValidations node if does not exists, maybe is better inside the foreach ?
1020+
if (!$xmlSheet->dataValidations) {
1021+
$xmlSheet->addChild('dataValidations');
1022+
}
1023+
1024+
foreach ($extlst->children(Namespaces::DATA_VALIDATIONS1)->dataValidations->dataValidation as $item) {
1025+
$item = self::testSimpleXml($item);
1026+
$node = self::testSimpleXml($xmlSheet->dataValidations)->addChild('dataValidation');
1027+
foreach ($item->attributes() ?? [] as $attr) {
1028+
$node->addAttribute($attr->getName(), $attr);
1029+
}
1030+
$node->addAttribute('sqref', $item->children(Namespaces::DATA_VALIDATIONS2)->sqref);
1031+
if (isset($item->formula1)) {
1032+
$childNode = $node->addChild('formula1');
1033+
if ($childNode !== null) { // null should never happen
1034+
$childNode[0] = (string) $item->formula1->children(Namespaces::DATA_VALIDATIONS2)->f; // @phpstan-ignore-line
1035+
}
10271036
}
10281037
}
10291038
}
@@ -1475,10 +1484,13 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14751484
foreach ($xmlDrawingChildren->twoCellAnchor as $twoCellAnchor) {
14761485
$twoCellAnchor = self::testSimpleXml($twoCellAnchor);
14771486
if ($twoCellAnchor->pic->blipFill) {
1487+
$objDrawing = new \PhpOffice\PhpSpreadsheet\Worksheet\Drawing();
14781488
$blip = $twoCellAnchor->pic->blipFill->children(Namespaces::DRAWINGML)->blip;
1489+
if (isset($twoCellAnchor->pic->blipFill->children(Namespaces::DRAWINGML)->srcRect)) {
1490+
$objDrawing->setSrcRect($twoCellAnchor->pic->blipFill->children(Namespaces::DRAWINGML)->srcRect->attributes());
1491+
}
14791492
$xfrm = $twoCellAnchor->pic->spPr->children(Namespaces::DRAWINGML)->xfrm;
14801493
$outerShdw = $twoCellAnchor->pic->spPr->children(Namespaces::DRAWINGML)->effectLst->outerShdw;
1481-
$objDrawing = new \PhpOffice\PhpSpreadsheet\Worksheet\Drawing();
14821494
/** @scrutinizer ignore-call */
14831495
$editAs = $twoCellAnchor->attributes();
14841496
if (isset($editAs, $editAs['editAs'])) {

‎src/PhpSpreadsheet/Reader/Xlsx/ConditionalStyles.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,17 @@ private function setConditionalsFromExt(array $conditionals): void
7474
private function readConditionalsFromExt(SimpleXMLElement $extLst, StyleReader $styleReader): array
7575
{
7676
$conditionals = [];
77+
if (!isset($extLst->ext)) {
78+
return $conditionals;
79+
}
7780

78-
if (isset($extLst->ext['uri']) && (string) $extLst->ext['uri'] === '{78C0D931-6437-407d-A8EE-F0AAD7539E65}') {
79-
$conditionalFormattingRuleXml = $extLst->ext->children($this->ns['x14']);
81+
foreach ($extLst->ext as $extlstcond) {
82+
$extAttrs = $extlstcond->/** @scrutinizer ignore-call */ attributes() ?? [];
83+
$extUri = (string) ($extAttrs['uri'] ?? '');
84+
if ($extUri !== '{78C0D931-6437-407d-A8EE-F0AAD7539E65}') {
85+
continue;
86+
}
87+
$conditionalFormattingRuleXml = $extlstcond->children($this->ns['x14']);
8088
if (!$conditionalFormattingRuleXml->conditionalFormattings) {
8189
return [];
8290
}

‎src/PhpSpreadsheet/Worksheet/BaseDrawing.php

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PhpOffice\PhpSpreadsheet\Cell\Hyperlink;
66
use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException;
77
use PhpOffice\PhpSpreadsheet\IComparable;
8+
use SimpleXMLElement;
89

910
class BaseDrawing implements IComparable
1011
{
@@ -164,6 +165,9 @@ class BaseDrawing implements IComparable
164165
*/
165166
protected $type = IMAGETYPE_UNKNOWN;
166167

168+
/** @var null|SimpleXMLElement|string[] */
169+
protected $srcRect = [];
170+
167171
/**
168172
* Create a new BaseDrawing.
169173
*/
@@ -532,4 +536,22 @@ public function validEditAs(): bool
532536
{
533537
return in_array($this->editAs, self::VALID_EDIT_AS, true);
534538
}
539+
540+
/**
541+
* @return null|SimpleXMLElement|string[]
542+
*/
543+
public function getSrcRect()
544+
{
545+
return $this->srcRect;
546+
}
547+
548+
/**
549+
* @param null|SimpleXMLElement|string[] $srcRect
550+
*/
551+
public function setSrcRect($srcRect): self
552+
{
553+
$this->srcRect = $srcRect;
554+
555+
return $this;
556+
}
535557
}

‎src/PhpSpreadsheet/Writer/Xlsx/Drawing.php

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,10 +270,21 @@ public function writeDrawing(XMLWriter $objWriter, BaseDrawing $drawing, $relati
270270
$objWriter->writeAttribute('r:embed', 'rId' . $relationId);
271271
$objWriter->endElement();
272272

273-
// a:stretch
274-
$objWriter->startElement('a:stretch');
275-
$objWriter->writeElement('a:fillRect', null);
276-
$objWriter->endElement();
273+
$srcRect = $drawing->getSrcRect();
274+
if (!empty($srcRect)) {
275+
$objWriter->startElement('a:srcRect');
276+
foreach ($srcRect as $key => $value) {
277+
$objWriter->writeAttribute($key, (string) $value);
278+
}
279+
$objWriter->endElement(); // a:srcRect
280+
$objWriter->startElement('a:stretch');
281+
$objWriter->endElement(); // a:stretch
282+
} else {
283+
// a:stretch
284+
$objWriter->startElement('a:stretch');
285+
$objWriter->writeElement('a:fillRect', null);
286+
$objWriter->endElement();
287+
}
277288

278289
$objWriter->endElement();
279290

‎tests/PhpSpreadsheetTests/Reader/Xlsx/Issue3679ImgTest.php

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
4+
5+
use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
6+
use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
7+
8+
class Issue3679ImgTest extends AbstractFunctional
9+
{
10+
public function testCroppedPicture(): void
11+
{
12+
$file = 'tests/data/Reader/XLSX/issue.3679.img.xlsx';
13+
$reader = new Xlsx();
14+
$spreadsheet = $reader->load($file);
15+
16+
$reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xlsx');
17+
$spreadsheet->disconnectWorksheets();
18+
$sheet = $reloadedSpreadsheet->getActiveSheet();
19+
$drawings = $sheet->getDrawingCollection();
20+
self::assertCount(1, $drawings);
21+
if ($drawings[0] === null) {
22+
self::fail('Unexpected null drawing');
23+
} else {
24+
$srcRect = $drawings[0]->getSrcRect();
25+
self::assertSame('448', (string) ($srcRect['r'] ?? ''));
26+
self::assertSame('65769', (string) ($srcRect['b'] ?? ''));
27+
}
28+
29+
$reloadedSpreadsheet->disconnectWorksheets();
30+
}
31+
}

‎tests/data/Reader/XLSX/issue.3679.img.xlsx

288 KB
Binary file not shown.

0 commit comments

Comments
 (0)