From 84096ae92431a5017839dcd575c4902287aa3bc4 Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sat, 24 Apr 2021 18:00:21 -0700 Subject: [PATCH 1/5] Gnumeric Better Namespace Handling There have been a number of issues concerning the handling of legitimate but unexpected namespace prefixes in Xlsx spreadsheets created by software other than Excel and PhpSpreadsheet/PhpExcel.I have studied them, but, till now, have not had a good idea on how to act on them. A recent comment https://github.com/PHPOffice/PhpSpreadsheet/issues/860#issuecomment-824926224 in issue #860 by @IMSoP has triggered an idea about how to proceed. Although the issues exclusively concern Xlsx format, I am starting out by dealing with Gnumeric. It is simpler and smaller than Xlsx, and, more important, already has a test for an unexpected prefix, since, at some point, it changed its generic prefix from gmr to gnm. I added support and a test for that some time ago, but almost certainly not in the best possible manner. The code as changed for this PR seems simpler and less kludgey, both for that exceptional case as well as for normal handling. My hope is that this change can be a template for similar Reader changes for Xml, Ods, and, especially, Xlsx. All grandfathered Phpstan issues with Gnumeric are fixed and eliminated from baseline as part of this change. --- phpstan-baseline.neon | 90 ------------ src/PhpSpreadsheet/Reader/Gnumeric.php | 128 +++++++++++++----- .../Reader/Gnumeric/PageSetup.php | 11 +- .../Reader/Gnumeric/Properties.php | 99 +++++++------- .../Reader/Gnumeric/GnumericLoadTest.php | 7 +- 5 files changed, 149 insertions(+), 186 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6390e0fc21..f96562615c 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -2825,91 +2825,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Reader/Csv/Delimiter.php - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:\\$referenceHelper has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Parameter \\#1 \\$fp of function fread expects resource, resource\\|false given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Parameter \\#1 \\$fp of function fclose expects resource, resource\\|false given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:\\$mappings has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Offset 'No' does not exist on SimpleXMLElement\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Offset 'Unit' does not exist on SimpleXMLElement\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Cannot call method setWidth\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\ColumnDimension\\|null\\.$#" - count: 3 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Cannot call method setVisible\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\ColumnDimension\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Offset 'DefaultSizePts' does not exist on SimpleXMLElement\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Cannot call method setRowHeight\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\RowDimension\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Cannot call method setVisible\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\RowDimension\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseBorderAttributes\\(\\) has no return typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseBorderAttributes\\(\\) has parameter \\$borderAttributes with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseRichText\\(\\) has no return typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseRichText\\(\\) has parameter \\$is with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseGnumericColour\\(\\) has no return typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Gnumeric\\:\\:parseGnumericColour\\(\\) has parameter \\$gnmColour with no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Gnumeric.php - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Html\\:\\:\\$rowspan has no typehint specified\\.$#" count: 1 @@ -8325,11 +8240,6 @@ parameters: count: 1 path: tests/PhpSpreadsheetTests/Reader/CsvTest.php - - - message: "#^Cannot call method getVisible\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\RowDimension\\|null\\.$#" - count: 1 - path: tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php - - message: "#^Cannot call method getRowHeight\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\RowDimension\\|null\\.$#" count: 2 diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index d3cdf1b002..7086cc6358 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -25,7 +25,19 @@ class Gnumeric extends BaseReader { - private const UOM_CONVERSION_POINTS_TO_CENTIMETERS = 0.03527777778; + const NAMESPACE_GNM = 'http://www.gnumeric.org/v10.dtd'; // gmr in old sheets + + const NAMESPACE_XSI = 'http://www.w3.org/2001/XMLSchema-instance'; + + const NAMESPACE_OFFICE = 'urn:oasis:names:tc:opendocument:xmlns:office:1.0'; + + const NAMESPACE_XLINK = 'http://www.w3.org/1999/xlink'; + + const NAMESPACE_DC = 'http://purl.org/dc/elements/1.1/'; + + const NAMESPACE_META = 'urn:oasis:names:tc:opendocument:xmlns:meta:1.0'; + + const NAMESPACE_OOO = 'http://openoffice.org/2004/office'; /** * Shared Expressions. @@ -41,16 +53,9 @@ class Gnumeric extends BaseReader */ private $spreadsheet; + /** @var ReferenceHelper */ private $referenceHelper; - /** - * Namespace shared across all functions. - * It is 'gnm', except for really old sheets which use 'gmr'. - * - * @var string - */ - private $gnm = 'gnm'; - /** * Create a new Gnumeric. */ @@ -77,8 +82,10 @@ public function canRead($pFilename) if (function_exists('gzread')) { // Read signature data (first 3 bytes) $fh = fopen($pFilename, 'rb'); - $data = fread($fh, 2); - fclose($fh); + if ($fh !== false) { + $data = fread($fh, 2); + fclose($fh); + } } return $data == chr(0x1F) . chr(0x8B); @@ -86,7 +93,7 @@ public function canRead($pFilename) private static function matchXml(string $name, string $field): bool { - return 1 === preg_match("/^(gnm|gmr):$field$/", $name); + return 1 === preg_match("/:$field$/", $name); } /** @@ -188,6 +195,7 @@ private function gzfileGetContents($filename) return $data; } + /** @var array */ private static $mappings = [ 'borderStyle' => [ '0' => Border::BORDER_NONE, @@ -266,7 +274,7 @@ public static function gnumericMappings(): array private function processComments(SimpleXMLElement $sheet): void { if ((!$this->readDataOnly) && (isset($sheet->Objects))) { - foreach ($sheet->Objects->children($this->gnm, true) as $key => $comment) { + foreach ($sheet->Objects->children(self::NAMESPACE_GNM) as $key => $comment) { $commentAttributes = $comment->attributes(); // Only comment objects are handled at the moment if ($commentAttributes->Text) { @@ -276,6 +284,14 @@ private function processComments(SimpleXMLElement $sheet): void } } + /** + * @param mixed $value + */ + private function testSimpleXml($value): SimpleXMLElement + { + return ($value instanceof SimpleXMLElement) ? $value : new SimpleXMLElement(''); + } + /** * Loads Spreadsheet from file. * @@ -304,12 +320,10 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S $gFileData = $this->gzfileGetContents($pFilename); $xml2 = simplexml_load_string($this->securityScanner->scan($gFileData), 'SimpleXMLElement', Settings::getLibXmlLoaderOptions()); - $xml = ($xml2 !== false) ? $xml2 : new SimpleXMLElement(''); - $namespacesMeta = $xml->getNamespaces(true); - $this->gnm = array_key_exists('gmr', $namespacesMeta) ? 'gmr' : 'gnm'; + $xml = self::testSimpleXml($xml2); - $gnmXML = $xml->children($namespacesMeta[$this->gnm]); - (new Properties($this->spreadsheet))->readProperties($xml, $gnmXML, $namespacesMeta); + $gnmXML = $xml->children(self::NAMESPACE_GNM); + (new Properties($this->spreadsheet))->readProperties($xml, $gnmXML); $worksheetID = 0; foreach ($gnmXML->Sheets->Sheet as $sheet) { @@ -329,7 +343,7 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S $this->spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false); if (!$this->readDataOnly) { - (new PageSetup($this->spreadsheet, $this->gnm)) + (new PageSetup($this->spreadsheet)) ->printInformation($sheet) ->sheetMargins($sheet); } @@ -510,21 +524,37 @@ private function processMergedCells(SimpleXMLElement $sheet): void } } + private function setColumnWidth(int $c, float $defaultWidth): void + { + $colDim = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1)); + if ($colDim !== null) { + $colDim->setWidth($defaultWidth); + } + } + + private function setColumnInvisible(int $c): void + { + $colDim = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1)); + if ($colDim !== null) { + $colDim->setVisible(false); + } + } + private function processColumnLoop(int $c, int $maxCol, SimpleXMLElement $columnOverride, float $defaultWidth): int { - $columnAttributes = $columnOverride->attributes(); + $columnAttributes = self::testSimpleXml($columnOverride->attributes()); $column = $columnAttributes['No']; $columnWidth = ((float) $columnAttributes['Unit']) / 5.4; $hidden = (isset($columnAttributes['Hidden'])) && ((string) $columnAttributes['Hidden'] == '1'); $columnCount = (int) ($columnAttributes['Count'] ?? 1); while ($c < $column) { - $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1))->setWidth($defaultWidth); + $this->setColumnWidth($c, $defaultWidth); ++$c; } while (($c < ($column + $columnCount)) && ($c <= $maxCol)) { - $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1))->setWidth($columnWidth); + $this->setColumnWidth($c, $columnWidth); if ($hidden) { - $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1))->setVisible(false); + self::setColumnInvisible($c); } ++$c; } @@ -536,35 +566,54 @@ private function processColumnWidths(SimpleXMLElement $sheet, int $maxCol): void { if ((!$this->readDataOnly) && (isset($sheet->Cols))) { // Column Widths + $defaultWidth = 0; $columnAttributes = $sheet->Cols->attributes(); - $defaultWidth = $columnAttributes['DefaultSizePts'] / 5.4; + if ($columnAttributes !== null) { + $defaultWidth = $columnAttributes['DefaultSizePts'] / 5.4; + } $c = 0; foreach ($sheet->Cols->ColInfo as $columnOverride) { $c = $this->processColumnLoop($c, $maxCol, $columnOverride, $defaultWidth); } while ($c <= $maxCol) { - $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1))->setWidth($defaultWidth); + $this->setColumnWidth($c, $defaultWidth); ++$c; } } } + private function setRowHeight(int $r, float $defaultHeight): void + { + $rowDim = $this->spreadsheet->getActiveSheet()->getRowDimension($r); + if ($rowDim !== null) { + $rowDim->setRowHeight($defaultHeight); + } + } + + private function setRowInvisible(int $r): void + { + $rowDim = $this->spreadsheet->getActiveSheet()->getRowDimension($r); + if ($rowDim !== null) { + $rowDim->setVisible(false); + } + } + private function processRowLoop(int $r, int $maxRow, SimpleXMLElement $rowOverride, float $defaultHeight): int { - $rowAttributes = $rowOverride->attributes(); + $rowAttributes = self::testSimpleXml($rowOverride->attributes()); $row = $rowAttributes['No']; $rowHeight = (float) $rowAttributes['Unit']; $hidden = (isset($rowAttributes['Hidden'])) && ((string) $rowAttributes['Hidden'] == '1'); $rowCount = (int) ($rowAttributes['Count'] ?? 1); while ($r < $row) { ++$r; - $this->spreadsheet->getActiveSheet()->getRowDimension($r)->setRowHeight($defaultHeight); + $this->setRowHeight($r, $defaultHeight); } while (($r < ($row + $rowCount)) && ($r < $maxRow)) { ++$r; - $this->spreadsheet->getActiveSheet()->getRowDimension($r)->setRowHeight($rowHeight); + $this->setRowHeight($r, $rowHeight); if ($hidden) { - $this->spreadsheet->getActiveSheet()->getRowDimension($r)->setVisible(false); + $this->setRowInvisible($r); } } @@ -575,8 +624,11 @@ private function processRowHeights(SimpleXMLElement $sheet, int $maxRow): void { if ((!$this->readDataOnly) && (isset($sheet->Rows))) { // Row Heights + $defaultHeight = 0; $rowAttributes = $sheet->Rows->attributes(); - $defaultHeight = (float) $rowAttributes['DefaultSizePts']; + if ($rowAttributes !== null) { + $defaultHeight = (float) $rowAttributes['DefaultSizePts']; + } $r = 0; foreach ($sheet->Rows->RowInfo as $rowOverride) { @@ -639,19 +691,21 @@ private static function addStyle2(array &$styleArray, string $key1, string $key, } } - private static function parseBorderAttributes($borderAttributes) + private static function parseBorderAttributes(?SimpleXMLElement $borderAttributes): array { $styleArray = []; - if (isset($borderAttributes['Color'])) { - $styleArray['color']['rgb'] = self::parseGnumericColour($borderAttributes['Color']); - } + if ($borderAttributes !== null) { + if (isset($borderAttributes['Color'])) { + $styleArray['color']['rgb'] = self::parseGnumericColour($borderAttributes['Color']); + } - self::addStyle($styleArray, 'borderStyle', $borderAttributes['Style']); + self::addStyle($styleArray, 'borderStyle', $borderAttributes['Style']); + } return $styleArray; } - private function parseRichText($is) + private function parseRichText(string $is): RichText { $value = new RichText(); $value->createText($is); @@ -659,7 +713,7 @@ private function parseRichText($is) return $value; } - private static function parseGnumericColour($gnmColour) + private static function parseGnumericColour(string $gnmColour): string { [$gnmR, $gnmG, $gnmB] = explode(':', $gnmColour); $gnmR = substr(str_pad($gnmR, 4, '0', STR_PAD_RIGHT), 0, 2); diff --git a/src/PhpSpreadsheet/Reader/Gnumeric/PageSetup.php b/src/PhpSpreadsheet/Reader/Gnumeric/PageSetup.php index 0fe7300593..accc27160f 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric/PageSetup.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric/PageSetup.php @@ -2,6 +2,7 @@ namespace PhpOffice\PhpSpreadsheet\Reader\Gnumeric; +use PhpOffice\PhpSpreadsheet\Reader\Gnumeric; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Worksheet\PageMargins; use PhpOffice\PhpSpreadsheet\Worksheet\PageSetup as WorksheetPageSetup; @@ -14,15 +15,9 @@ class PageSetup */ private $spreadsheet; - /** - * @var string - */ - private $gnm; - - public function __construct(Spreadsheet $spreadsheet, string $gnm) + public function __construct(Spreadsheet $spreadsheet) { $this->spreadsheet = $spreadsheet; - $this->gnm = $gnm; } public function printInformation(SimpleXMLElement $sheet): self @@ -68,7 +63,7 @@ public function sheetMargins(SimpleXMLElement $sheet): self private function buildMarginSet(SimpleXMLElement $sheet, array $marginSet): array { - foreach ($sheet->PrintInformation->Margins->children($this->gnm, true) as $key => $margin) { + foreach ($sheet->PrintInformation->Margins->children(Gnumeric::NAMESPACE_GNM) as $key => $margin) { $marginAttributes = $margin->attributes(); $marginSize = ($marginAttributes['Points']) ?? 72; // Default is 72pt // Convert value in points to inches diff --git a/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php b/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php index 16d9c2e082..1696f06940 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php @@ -2,6 +2,7 @@ namespace PhpOffice\PhpSpreadsheet\Reader\Gnumeric; +use PhpOffice\PhpSpreadsheet\Reader\Gnumeric; use PhpOffice\PhpSpreadsheet\Spreadsheet; use SimpleXMLElement; @@ -91,74 +92,72 @@ private function docPropertiesDC(SimpleXMLElement $officePropertyDC): void } } - private function docPropertiesMeta(SimpleXMLElement $officePropertyMeta, array $namespacesMeta): void + private function docPropertiesMeta(SimpleXMLElement $officePropertyMeta): void { $docProps = $this->spreadsheet->getProperties(); foreach ($officePropertyMeta as $propertyName => $propertyValue) { - if ($propertyValue === null) { - continue; + if ($propertyValue !== null) { + $attributes = $propertyValue->attributes(Gnumeric::NAMESPACE_META); + $propertyValue = trim((string) $propertyValue); + switch ($propertyName) { + case 'keyword': + $docProps->setKeywords($propertyValue); + + break; + case 'initial-creator': + $docProps->setCreator($propertyValue); + $docProps->setLastModifiedBy($propertyValue); + + break; + case 'creation-date': + $creationDate = strtotime($propertyValue); + $creationDate = $creationDate === false ? time() : $creationDate; + $docProps->setCreated($creationDate); + $docProps->setModified($creationDate); + + break; + case 'user-defined': + [, $attrName] = explode(':', $attributes['name']); + self::userDefinedProperties($attrName, $propertyValue); + + break; + } } + } + } - $attributes = $propertyValue->attributes($namespacesMeta['meta']); - $propertyValue = trim((string) $propertyValue); - switch ($propertyName) { - case 'keyword': - $docProps->setKeywords($propertyValue); - - break; - case 'initial-creator': - $docProps->setCreator($propertyValue); - $docProps->setLastModifiedBy($propertyValue); - - break; - case 'creation-date': - $creationDate = strtotime($propertyValue); - $creationDate = $creationDate === false ? time() : $creationDate; - $docProps->setCreated($creationDate); - $docProps->setModified($creationDate); - - break; - case 'user-defined': - [, $attrName] = explode(':', $attributes['name']); - switch ($attrName) { - case 'publisher': - $docProps->setCompany($propertyValue); - - break; - case 'category': - $docProps->setCategory($propertyValue); + private function userDefinedProperties(string $attrName, string $propertyValue): void + { + $docProps = $this->spreadsheet->getProperties(); + switch ($attrName) { + case 'publisher': + $docProps->setCompany($propertyValue); - break; - case 'manager': - $docProps->setManager($propertyValue); + break; + case 'category': + $docProps->setCategory($propertyValue); - break; - } + break; + case 'manager': + $docProps->setManager($propertyValue); - break; - } + break; } } - public function readProperties(SimpleXMLElement $xml, SimpleXMLElement $gnmXML, array $namespacesMeta): void + public function readProperties(SimpleXMLElement $xml, SimpleXMLElement $gnmXML): void { - if (isset($namespacesMeta['office'])) { - $officeXML = $xml->children($namespacesMeta['office']); + $officeXML = $xml->children(Gnumeric::NAMESPACE_OFFICE); + if (!empty($officeXML)) { $officeDocXML = $officeXML->{'document-meta'}; $officeDocMetaXML = $officeDocXML->meta; foreach ($officeDocMetaXML as $officePropertyData) { - $officePropertyDC = []; - if (isset($namespacesMeta['dc'])) { - $officePropertyDC = $officePropertyData->children($namespacesMeta['dc']); - } + $officePropertyDC = $officePropertyData->children(Gnumeric::NAMESPACE_DC); $this->docPropertiesDC($officePropertyDC); - $officePropertyMeta = []; - if (isset($namespacesMeta['meta'])) { - $officePropertyMeta = $officePropertyData->children($namespacesMeta['meta']); - } - $this->docPropertiesMeta($officePropertyMeta, $namespacesMeta); + $officePropertyMeta = $officePropertyData->children(Gnumeric::NAMESPACE_META); + $this->docPropertiesMeta($officePropertyMeta); } } elseif (isset($gnmXML->Summary)) { $this->docPropertiesOld($gnmXML); diff --git a/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php b/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php index e24178e558..65511153b8 100644 --- a/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php @@ -115,7 +115,12 @@ public function testLoad(): void self::assertEquals(Font::UNDERLINE_DOUBLE, $sheet->getCell('A24')->getStyle()->getFont()->getUnderline()); self::assertTrue($sheet->getCell('B23')->getStyle()->getFont()->getSubScript()); self::assertTrue($sheet->getCell('B24')->getStyle()->getFont()->getSuperScript()); - self::assertFalse($sheet->getRowDimension(30)->getVisible()); + $rowDimension = $sheet->getRowDimension(30); + if ($rowDimension === null) { + self::fail('Unable to get RowDimension for row 30)'); + } else { + self::assertFalse($rowDimension->getVisible()); + } } public function testLoadFilter(): void From 961945d867f09eb7e29abd37fcf5627fa62f98b2 Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sat, 24 Apr 2021 18:23:43 -0700 Subject: [PATCH 2/5] Scrutinizer Static vs. non-static in 5 places. --- src/PhpSpreadsheet/Reader/Gnumeric.php | 4 ++-- src/PhpSpreadsheet/Reader/Gnumeric/Properties.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 7086cc6358..5cdfcb8bcd 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -287,7 +287,7 @@ private function processComments(SimpleXMLElement $sheet): void /** * @param mixed $value */ - private function testSimpleXml($value): SimpleXMLElement + private static function testSimpleXml($value): SimpleXMLElement { return ($value instanceof SimpleXMLElement) ? $value : new SimpleXMLElement(''); } @@ -554,7 +554,7 @@ private function processColumnLoop(int $c, int $maxCol, SimpleXMLElement $column while (($c < ($column + $columnCount)) && ($c <= $maxCol)) { $this->setColumnWidth($c, $columnWidth); if ($hidden) { - self::setColumnInvisible($c); + $this->setColumnInvisible($c); } ++$c; } diff --git a/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php b/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php index 1696f06940..c466a859e8 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric/Properties.php @@ -118,7 +118,7 @@ private function docPropertiesMeta(SimpleXMLElement $officePropertyMeta): void break; case 'user-defined': [, $attrName] = explode(':', $attributes['name']); - self::userDefinedProperties($attrName, $propertyValue); + $this->userDefinedProperties($attrName, $propertyValue); break; } From 43577164ec9ca79bed6a37ad2225e269a30d595c Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sun, 25 Apr 2021 10:04:53 -0700 Subject: [PATCH 3/5] Namespace Handling using XMLReader Adopt a suggestion from @IMSoP affecting listWorkSheetInfo, which uses XMLReader rather than SimpleXML for its processing. --- src/PhpSpreadsheet/Reader/Gnumeric.php | 38 +++++++++++++------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 5cdfcb8bcd..33a7b82f6f 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -91,9 +91,11 @@ public function canRead($pFilename) return $data == chr(0x1F) . chr(0x8B); } - private static function matchXml(string $name, string $field): bool + private static function matchXml(XMLReader $xml, string $expectedLocalName): bool { - return 1 === preg_match("/:$field$/", $name); + return $xml->namespaceURI == self::NAMESPACE_GNM + && $xml->localName == $expectedLocalName + && $xml->nodeType == XMLReader::ELEMENT; } /** @@ -113,10 +115,10 @@ public function listWorksheetNames($pFilename) $worksheetNames = []; while ($xml->read()) { - if (self::matchXml($xml->name, 'SheetName') && $xml->nodeType == XMLReader::ELEMENT) { + if (self::matchXml($xml, 'SheetName')) { $xml->read(); // Move onto the value node $worksheetNames[] = (string) $xml->value; - } elseif (self::matchXml($xml->name, 'Sheets')) { + } elseif (self::matchXml($xml, 'Sheets')) { // break out of the loop once we've got our sheet names rather than parse the entire file break; } @@ -142,7 +144,7 @@ public function listWorksheetInfo($pFilename) $worksheetInfo = []; while ($xml->read()) { - if (self::matchXml($xml->name, 'Sheet') && $xml->nodeType == XMLReader::ELEMENT) { + if (self::matchXml($xml, 'Sheet')) { $tmpInfo = [ 'worksheetName' => '', 'lastColumnLetter' => 'A', @@ -152,20 +154,18 @@ public function listWorksheetInfo($pFilename) ]; while ($xml->read()) { - if ($xml->nodeType == XMLReader::ELEMENT) { - if (self::matchXml($xml->name, 'Name')) { - $xml->read(); // Move onto the value node - $tmpInfo['worksheetName'] = (string) $xml->value; - } elseif (self::matchXml($xml->name, 'MaxCol')) { - $xml->read(); // Move onto the value node - $tmpInfo['lastColumnIndex'] = (int) $xml->value; - $tmpInfo['totalColumns'] = (int) $xml->value + 1; - } elseif (self::matchXml($xml->name, 'MaxRow')) { - $xml->read(); // Move onto the value node - $tmpInfo['totalRows'] = (int) $xml->value + 1; - - break; - } + if (self::matchXml($xml, 'Name')) { + $xml->read(); // Move onto the value node + $tmpInfo['worksheetName'] = (string) $xml->value; + } elseif (self::matchXml($xml, 'MaxCol')) { + $xml->read(); // Move onto the value node + $tmpInfo['lastColumnIndex'] = (int) $xml->value; + $tmpInfo['totalColumns'] = (int) $xml->value + 1; + } elseif (self::matchXml($xml, 'MaxRow')) { + $xml->read(); // Move onto the value node + $tmpInfo['totalRows'] = (int) $xml->value + 1; + + break; } } $tmpInfo['lastColumnLetter'] = Coordinate::stringFromColumnIndex($tmpInfo['lastColumnIndex'] + 1); From 64eedd835c86f2da42a287446b05e50acc945ebb Mon Sep 17 00:00:00 2001 From: oleibman Date: Sun, 25 Apr 2021 10:26:06 -0700 Subject: [PATCH 4/5] Update GnumericLoadTest.php PR #2024 was pushed last night, causing a Phpstan problem with this member. --- .../Reader/Gnumeric/GnumericLoadTest.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php b/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php index 65511153b8..9544fc3a00 100644 --- a/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Gnumeric/GnumericLoadTest.php @@ -116,11 +116,7 @@ public function testLoad(): void self::assertTrue($sheet->getCell('B23')->getStyle()->getFont()->getSubScript()); self::assertTrue($sheet->getCell('B24')->getStyle()->getFont()->getSuperScript()); $rowDimension = $sheet->getRowDimension(30); - if ($rowDimension === null) { - self::fail('Unable to get RowDimension for row 30)'); - } else { - self::assertFalse($rowDimension->getVisible()); - } + self::assertFalse($rowDimension->getVisible()); } public function testLoadFilter(): void From aff65ec57c3d205475056feef2f198980e981edc Mon Sep 17 00:00:00 2001 From: oleibman Date: Mon, 3 May 2021 20:11:10 -0700 Subject: [PATCH 5/5] Update Gnumeric.php Suggestions from Mark Baker - strict equality test, more descriptive variable names. --- src/PhpSpreadsheet/Reader/Gnumeric.php | 100 ++++++++++++------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 33a7b82f6f..390a7687ee 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -88,14 +88,14 @@ public function canRead($pFilename) } } - return $data == chr(0x1F) . chr(0x8B); + return $data === chr(0x1F) . chr(0x8B); } private static function matchXml(XMLReader $xml, string $expectedLocalName): bool { - return $xml->namespaceURI == self::NAMESPACE_GNM - && $xml->localName == $expectedLocalName - && $xml->nodeType == XMLReader::ELEMENT; + return $xml->namespaceURI === self::NAMESPACE_GNM + && $xml->localName === $expectedLocalName + && $xml->nodeType === XMLReader::ELEMENT; } /** @@ -396,7 +396,7 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S if (array_key_exists($vtype, self::$mappings['dataType'])) { $type = self::$mappings['dataType'][$vtype]; } - if ($vtype == '20') { // Boolean + if ($vtype === '20') { // Boolean $cell = $cell == 'TRUE'; } } @@ -524,42 +524,42 @@ private function processMergedCells(SimpleXMLElement $sheet): void } } - private function setColumnWidth(int $c, float $defaultWidth): void + private function setColumnWidth(int $whichColumn, float $defaultWidth): void { - $colDim = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1)); - if ($colDim !== null) { - $colDim->setWidth($defaultWidth); + $columnDimension = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($whichColumn + 1)); + if ($columnDimension !== null) { + $columnDimension->setWidth($defaultWidth); } } - private function setColumnInvisible(int $c): void + private function setColumnInvisible(int $whichColumn): void { - $colDim = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($c + 1)); - if ($colDim !== null) { - $colDim->setVisible(false); + $columnDimension = $this->spreadsheet->getActiveSheet()->getColumnDimension(Coordinate::stringFromColumnIndex($whichColumn + 1)); + if ($columnDimension !== null) { + $columnDimension->setVisible(false); } } - private function processColumnLoop(int $c, int $maxCol, SimpleXMLElement $columnOverride, float $defaultWidth): int + private function processColumnLoop(int $whichColumn, int $maxCol, SimpleXMLElement $columnOverride, float $defaultWidth): int { $columnAttributes = self::testSimpleXml($columnOverride->attributes()); $column = $columnAttributes['No']; $columnWidth = ((float) $columnAttributes['Unit']) / 5.4; $hidden = (isset($columnAttributes['Hidden'])) && ((string) $columnAttributes['Hidden'] == '1'); $columnCount = (int) ($columnAttributes['Count'] ?? 1); - while ($c < $column) { - $this->setColumnWidth($c, $defaultWidth); - ++$c; + while ($whichColumn < $column) { + $this->setColumnWidth($whichColumn, $defaultWidth); + ++$whichColumn; } - while (($c < ($column + $columnCount)) && ($c <= $maxCol)) { - $this->setColumnWidth($c, $columnWidth); + while (($whichColumn < ($column + $columnCount)) && ($whichColumn <= $maxCol)) { + $this->setColumnWidth($whichColumn, $columnWidth); if ($hidden) { - $this->setColumnInvisible($c); + $this->setColumnInvisible($whichColumn); } - ++$c; + ++$whichColumn; } - return $c; + return $whichColumn; } private function processColumnWidths(SimpleXMLElement $sheet, int $maxCol): void @@ -571,53 +571,53 @@ private function processColumnWidths(SimpleXMLElement $sheet, int $maxCol): void if ($columnAttributes !== null) { $defaultWidth = $columnAttributes['DefaultSizePts'] / 5.4; } - $c = 0; + $whichColumn = 0; foreach ($sheet->Cols->ColInfo as $columnOverride) { - $c = $this->processColumnLoop($c, $maxCol, $columnOverride, $defaultWidth); + $whichColumn = $this->processColumnLoop($whichColumn, $maxCol, $columnOverride, $defaultWidth); } - while ($c <= $maxCol) { - $this->setColumnWidth($c, $defaultWidth); - ++$c; + while ($whichColumn <= $maxCol) { + $this->setColumnWidth($whichColumn, $defaultWidth); + ++$whichColumn; } } } - private function setRowHeight(int $r, float $defaultHeight): void + private function setRowHeight(int $whichRow, float $defaultHeight): void { - $rowDim = $this->spreadsheet->getActiveSheet()->getRowDimension($r); - if ($rowDim !== null) { - $rowDim->setRowHeight($defaultHeight); + $rowDimension = $this->spreadsheet->getActiveSheet()->getRowDimension($whichRow); + if ($rowDimension !== null) { + $rowDimension->setRowHeight($defaultHeight); } } - private function setRowInvisible(int $r): void + private function setRowInvisible(int $whichRow): void { - $rowDim = $this->spreadsheet->getActiveSheet()->getRowDimension($r); - if ($rowDim !== null) { - $rowDim->setVisible(false); + $rowDimension = $this->spreadsheet->getActiveSheet()->getRowDimension($whichRow); + if ($rowDimension !== null) { + $rowDimension->setVisible(false); } } - private function processRowLoop(int $r, int $maxRow, SimpleXMLElement $rowOverride, float $defaultHeight): int + private function processRowLoop(int $whichRow, int $maxRow, SimpleXMLElement $rowOverride, float $defaultHeight): int { $rowAttributes = self::testSimpleXml($rowOverride->attributes()); $row = $rowAttributes['No']; $rowHeight = (float) $rowAttributes['Unit']; $hidden = (isset($rowAttributes['Hidden'])) && ((string) $rowAttributes['Hidden'] == '1'); $rowCount = (int) ($rowAttributes['Count'] ?? 1); - while ($r < $row) { - ++$r; - $this->setRowHeight($r, $defaultHeight); + while ($whichRow < $row) { + ++$whichRow; + $this->setRowHeight($whichRow, $defaultHeight); } - while (($r < ($row + $rowCount)) && ($r < $maxRow)) { - ++$r; - $this->setRowHeight($r, $rowHeight); + while (($whichRow < ($row + $rowCount)) && ($whichRow < $maxRow)) { + ++$whichRow; + $this->setRowHeight($whichRow, $rowHeight); if ($hidden) { - $this->setRowInvisible($r); + $this->setRowInvisible($whichRow); } } - return $r; + return $whichRow; } private function processRowHeights(SimpleXMLElement $sheet, int $maxRow): void @@ -629,17 +629,17 @@ private function processRowHeights(SimpleXMLElement $sheet, int $maxRow): void if ($rowAttributes !== null) { $defaultHeight = (float) $rowAttributes['DefaultSizePts']; } - $r = 0; + $whichRow = 0; foreach ($sheet->Rows->RowInfo as $rowOverride) { - $r = $this->processRowLoop($r, $maxRow, $rowOverride, $defaultHeight); + $whichRow = $this->processRowLoop($whichRow, $maxRow, $rowOverride, $defaultHeight); } // never executed, I can't figure out any circumstances // under which it would be executed, and, even if // such exist, I'm not convinced this is needed. - //while ($r < $maxRow) { - // ++$r; - // $this->spreadsheet->getActiveSheet()->getRowDimension($r)->setRowHeight($defaultHeight); + //while ($whichRow < $maxRow) { + // ++$whichRow; + // $this->spreadsheet->getActiveSheet()->getRowDimension($whichRow)->setRowHeight($defaultHeight); //} } } @@ -731,7 +731,7 @@ private function addColors(array &$styleArray, SimpleXMLElement $styleAttributes $shade = (string) $styleAttributes['Shade']; if (($RGB != '000000') || ($shade != '0')) { $RGB2 = self::parseGnumericColour($styleAttributes['PatternColor']); - if ($shade == '1') { + if ($shade === '1') { $styleArray['fill']['startColor']['rgb'] = $RGB; $styleArray['fill']['endColor']['rgb'] = $RGB2; } else {