From 31d0a2e9f997ba975615ba64bd6d85a80fbe998a Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Thu, 13 Oct 2022 07:37:07 -0700 Subject: [PATCH] Eliminate Some Scrutinizer 'Major' Problems (#3109) Almost all of these are handled through annotations. This shouldn't be our "go-to" solution, but it becomes necessary because Scrutinizer's analysis is often incorrect. Here is a typical example, from Cells.php. ```php if ($this->currentCellIsDirty && isset($this->currentCoordinate, $this->currentCell)) { $this->currentCell->detach(); ``` Scrutinizer complains that `$this->currentCell` can be null here, but the `isset` condition guarantees that it must be non-null. Perhaps Scrutinizer is worried that `isset` might be overridden, and will accept only an explicit equality test for null for each of the isset arguments. Changing the code to do this seems riskier than just adding the annotation. A far more common, and more frustrating, example is: ```php foreach ($simpleXmlElement as $element) { var_dump($element->method()); } ``` Scrutinizer complains that element might be null. I don't think it can. I have previously added code in places to eliminate the objection, and that may be a practical solution when `$element` is used many times in the loop. But, when it's used only once, annotating the objection away seems like a better solution (less overhead, clearer code). Many of the changes in this PR fall into this category. --- phpstan-baseline.neon | 12 ------------ src/PhpSpreadsheet/Calculation/Exception.php | 2 +- src/PhpSpreadsheet/Collection/Cells.php | 2 +- src/PhpSpreadsheet/Reader/Gnumeric.php | 2 +- src/PhpSpreadsheet/Reader/Gnumeric/Styles.php | 7 +++++-- src/PhpSpreadsheet/Reader/Ods.php | 8 +++++--- src/PhpSpreadsheet/Reader/Ods/Properties.php | 1 + src/PhpSpreadsheet/Style/Border.php | 2 +- src/PhpSpreadsheet/Style/Color.php | 2 +- src/PhpSpreadsheet/Worksheet/CellIterator.php | 6 +++--- src/PhpSpreadsheet/Worksheet/Column.php | 1 + src/PhpSpreadsheet/Worksheet/ColumnIterator.php | 6 +++--- src/PhpSpreadsheet/Worksheet/Row.php | 1 + src/PhpSpreadsheet/Worksheet/RowIterator.php | 6 +++--- src/PhpSpreadsheet/Writer/Xls/Parser.php | 15 ++++++++------- 15 files changed, 35 insertions(+), 38 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 118880ce8..fde173729 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1966,18 +1966,6 @@ parameters: message: "#^Parameter \\#1 \\$underline of static method PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Font\\:\\:mapUnderline\\(\\) expects string, string\\|null given\\.$#" count: 1 path: src/PhpSpreadsheet/Writer/Xls/Font.php - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Parser\\:\\:advance\\(\\) has no return type specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Writer/Xls/Parser.php - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Parser\\:\\:\\$spreadsheet has no type specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Writer/Xls/Parser.php - - - message: "#^Unreachable statement \\- code above always terminates\\.$#" - count: 5 - path: src/PhpSpreadsheet/Writer/Xls/Parser.php - message: "#^Cannot access offset 'encoding' on array\\|false\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Calculation/Exception.php b/src/PhpSpreadsheet/Calculation/Exception.php index 87c7d222b..0388f5acb 100644 --- a/src/PhpSpreadsheet/Calculation/Exception.php +++ b/src/PhpSpreadsheet/Calculation/Exception.php @@ -15,7 +15,7 @@ class Exception extends PhpSpreadsheetException * @param mixed $line * @param mixed $context */ - public static function errorHandlerCallback($code, $string, $file, $line, $context): void + public static function errorHandlerCallback($code, $string, $file, $line, /** @scrutinizer ignore-unused */ $context): void { $e = new self($string, $code); $e->line = $line; diff --git a/src/PhpSpreadsheet/Collection/Cells.php b/src/PhpSpreadsheet/Collection/Cells.php index 6183e7335..6b32ba939 100644 --- a/src/PhpSpreadsheet/Collection/Cells.php +++ b/src/PhpSpreadsheet/Collection/Cells.php @@ -352,7 +352,7 @@ class Cells private function storeCurrentCell(): void { if ($this->currentCellIsDirty && isset($this->currentCoordinate, $this->currentCell)) { - $this->currentCell->detach(); + $this->currentCell->/** @scrutinizer ignore-call */ detach(); $stored = $this->cache->set($this->cachePrefix . $this->currentCoordinate, $this->currentCell); if ($stored === false) { diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 1dcb0a125..b33af242e 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -272,7 +272,7 @@ class Gnumeric extends BaseReader // name in line with the formula, not the reverse $this->spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false); - $visibility = $sheetOrNull->attributes()['Visibility'] ?? 'GNM_SHEET_VISIBILITY_VISIBLE'; + $visibility = $sheet->attributes()['Visibility'] ?? 'GNM_SHEET_VISIBILITY_VISIBLE'; if ((string) $visibility !== 'GNM_SHEET_VISIBILITY_VISIBLE') { $this->spreadsheet->getActiveSheet()->setSheetState(Worksheet::SHEETSTATE_HIDDEN); } diff --git a/src/PhpSpreadsheet/Reader/Gnumeric/Styles.php b/src/PhpSpreadsheet/Reader/Gnumeric/Styles.php index e2e9d561c..dc44dcc23 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric/Styles.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric/Styles.php @@ -101,6 +101,7 @@ class Styles private function readStyles(SimpleXMLElement $styleRegion, int $maxRow, int $maxCol): void { foreach ($styleRegion as $style) { + /** @scrutinizer ignore-call */ $styleAttributes = $style->attributes(); if ($styleAttributes !== null && ($styleAttributes['startRow'] <= $maxRow) && ($styleAttributes['startCol'] <= $maxCol)) { $cellRange = $this->readStyleRange($styleAttributes, $maxCol, $maxRow); @@ -117,7 +118,7 @@ class Styles if ($this->readDataOnly === false && $styleAttributes !== null) { // If readDataOnly is false, we set all formatting information $styleArray['numberFormat']['formatCode'] = $formatCode; - $styleArray = $this->readStyle($styleArray, $styleAttributes, $style); + $styleArray = $this->readStyle($styleArray, $styleAttributes, /** @scrutinizer ignore-type */ $style); } $this->spreadsheet->getActiveSheet()->getStyle($cellRange)->applyFromArray($styleArray); } @@ -268,10 +269,12 @@ class Styles $this->addBorderStyle($srssb, $styleArray, 'right'); $this->addBorderDiagonal($srssb, $styleArray); } + // TO DO + /* if (isset($style->Style->HyperLink)) { - // TO DO $hyperlink = $style->Style->HyperLink->attributes(); } + */ return $styleArray; } diff --git a/src/PhpSpreadsheet/Reader/Ods.php b/src/PhpSpreadsheet/Reader/Ods.php index e3de47317..b3a84bf70 100644 --- a/src/PhpSpreadsheet/Reader/Ods.php +++ b/src/PhpSpreadsheet/Reader/Ods.php @@ -52,7 +52,7 @@ class Ods extends BaseReader if ($zip->open($filename) === true) { // check if it is an OOXML archive $stat = $zip->statName('mimetype'); - if ($stat && ($stat['size'] <= 255)) { + if (!empty($stat) && ($stat['size'] <= 255)) { $mimeType = $zip->getFromName($stat['name']); } elseif ($zip->statName('META-INF/manifest.xml')) { $xml = simplexml_load_string( @@ -64,6 +64,7 @@ class Ods extends BaseReader if (isset($namespacesContent['manifest'])) { $manifest = $xml->children($namespacesContent['manifest']); foreach ($manifest as $manifestDataSet) { + /** @scrutinizer ignore-call */ $manifestAttributes = $manifestDataSet->attributes($namespacesContent['manifest']); if ($manifestAttributes && $manifestAttributes->{'full-path'} == '/') { $mimeType = (string) $manifestAttributes->{'media-type'}; @@ -311,7 +312,7 @@ class Ods extends BaseReader // Check loadSheetsOnly if ( - isset($this->loadSheetsOnly) + $this->loadSheetsOnly !== null && $worksheetName && !in_array($worksheetName, $this->loadSheetsOnly) ) { @@ -507,7 +508,7 @@ class Ods extends BaseReader $dataValue = Date::PHPToExcel( strtotime( - '01-01-1970 ' . implode(':', sscanf($timeValue, 'PT%dH%dM%dS') ?? []) + '01-01-1970 ' . implode(':', /** @scrutinizer ignore-type */ sscanf($timeValue, 'PT%dH%dM%dS') ?? []) ) ); $formatting = NumberFormat::FORMAT_DATE_TIME4; @@ -693,6 +694,7 @@ class Ods extends BaseReader // Multiple spaces? /** @var DOMAttr $cAttr */ + /** @scrutinizer ignore-call */ $cAttr = $child->attributes->getNamedItem('c'); $multiplier = self::getMultiplier($cAttr); $str .= str_repeat(' ', $multiplier); diff --git a/src/PhpSpreadsheet/Reader/Ods/Properties.php b/src/PhpSpreadsheet/Reader/Ods/Properties.php index 4389f4b2f..c3045fc96 100644 --- a/src/PhpSpreadsheet/Reader/Ods/Properties.php +++ b/src/PhpSpreadsheet/Reader/Ods/Properties.php @@ -22,6 +22,7 @@ class Properties foreach ($officeProperty as $officePropertyData) { // @var \SimpleXMLElement $officePropertyData if (isset($namespacesMeta['dc'])) { + /** @scrutinizer ignore-call */ $officePropertiesDC = $officePropertyData->children($namespacesMeta['dc']); $this->setCoreProperties($docProps, $officePropertiesDC); } diff --git a/src/PhpSpreadsheet/Style/Border.php b/src/PhpSpreadsheet/Style/Border.php index a5ec980a0..c6fb51f15 100644 --- a/src/PhpSpreadsheet/Style/Border.php +++ b/src/PhpSpreadsheet/Style/Border.php @@ -103,7 +103,7 @@ class Border extends Supervisor /** @var Style */ $parent = $this->parent; - return $parent->getStyleArray([$this->parentPropertyName => $array]); + return $parent->/** @scrutinizer ignore-call */ getStyleArray([$this->parentPropertyName => $array]); } /** diff --git a/src/PhpSpreadsheet/Style/Color.php b/src/PhpSpreadsheet/Style/Color.php index 922be803c..3c002b270 100644 --- a/src/PhpSpreadsheet/Style/Color.php +++ b/src/PhpSpreadsheet/Style/Color.php @@ -171,7 +171,7 @@ class Color extends Supervisor /** @var Style */ $parent = $this->parent; - return $parent->getStyleArray([$this->parentPropertyName => $array]); + return $parent->/** @scrutinizer ignore-call */ getStyleArray([$this->parentPropertyName => $array]); } /** diff --git a/src/PhpSpreadsheet/Worksheet/CellIterator.php b/src/PhpSpreadsheet/Worksheet/CellIterator.php index 94877f667..b0d9d05be 100644 --- a/src/PhpSpreadsheet/Worksheet/CellIterator.php +++ b/src/PhpSpreadsheet/Worksheet/CellIterator.php @@ -2,16 +2,16 @@ namespace PhpOffice\PhpSpreadsheet\Worksheet; -use Iterator; +use Iterator as NativeIterator; use PhpOffice\PhpSpreadsheet\Cell\Cell; use PhpOffice\PhpSpreadsheet\Collection\Cells; /** * @template TKey * - * @implements Iterator + * @implements NativeIterator */ -abstract class CellIterator implements Iterator +abstract class CellIterator implements NativeIterator { public const TREAT_NULL_VALUE_AS_EMPTY_CELL = 1; diff --git a/src/PhpSpreadsheet/Worksheet/Column.php b/src/PhpSpreadsheet/Worksheet/Column.php index e6e332ae7..390a15937 100644 --- a/src/PhpSpreadsheet/Worksheet/Column.php +++ b/src/PhpSpreadsheet/Worksheet/Column.php @@ -85,6 +85,7 @@ class Column $cellIterator = $this->getCellIterator(); $cellIterator->setIterateOnlyExistingCells(true); foreach ($cellIterator as $cell) { + /** @scrutinizer ignore-call */ $value = $cell->getValue(); if ($value === null && $nullValueCellIsEmpty === true) { continue; diff --git a/src/PhpSpreadsheet/Worksheet/ColumnIterator.php b/src/PhpSpreadsheet/Worksheet/ColumnIterator.php index b64ae5a48..fffef1292 100644 --- a/src/PhpSpreadsheet/Worksheet/ColumnIterator.php +++ b/src/PhpSpreadsheet/Worksheet/ColumnIterator.php @@ -2,15 +2,15 @@ namespace PhpOffice\PhpSpreadsheet\Worksheet; -use Iterator; +use Iterator as NativeIterator; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Exception; use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException; /** - * @implements Iterator + * @implements NativeIterator */ -class ColumnIterator implements Iterator +class ColumnIterator implements NativeIterator { /** * Worksheet to iterate. diff --git a/src/PhpSpreadsheet/Worksheet/Row.php b/src/PhpSpreadsheet/Worksheet/Row.php index 5c1627526..223a07caf 100644 --- a/src/PhpSpreadsheet/Worksheet/Row.php +++ b/src/PhpSpreadsheet/Worksheet/Row.php @@ -84,6 +84,7 @@ class Row $cellIterator = $this->getCellIterator(); $cellIterator->setIterateOnlyExistingCells(true); foreach ($cellIterator as $cell) { + /** @scrutinizer ignore-call */ $value = $cell->getValue(); if ($value === null && $nullValueCellIsEmpty === true) { continue; diff --git a/src/PhpSpreadsheet/Worksheet/RowIterator.php b/src/PhpSpreadsheet/Worksheet/RowIterator.php index 5c51bfeea..84f376c1f 100644 --- a/src/PhpSpreadsheet/Worksheet/RowIterator.php +++ b/src/PhpSpreadsheet/Worksheet/RowIterator.php @@ -2,13 +2,13 @@ namespace PhpOffice\PhpSpreadsheet\Worksheet; -use Iterator; +use Iterator as NativeIterator; use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException; /** - * @implements Iterator + * @implements NativeIterator */ -class RowIterator implements Iterator +class RowIterator implements NativeIterator { /** * Worksheet to iterate. diff --git a/src/PhpSpreadsheet/Writer/Xls/Parser.php b/src/PhpSpreadsheet/Writer/Xls/Parser.php index ca407d2a0..3fe9e8728 100644 --- a/src/PhpSpreadsheet/Writer/Xls/Parser.php +++ b/src/PhpSpreadsheet/Writer/Xls/Parser.php @@ -469,6 +469,7 @@ class Parser 'BAHTTEXT' => [368, 1, 0, 0], ]; + /** @var Spreadsheet */ private $spreadsheet; /** @@ -752,6 +753,8 @@ class Parser throw new WriterException('Defined Name is too long'); } + throw new WriterException('Cannot yet write formulae with defined names to Xls'); + /* $nameReference = 1; foreach ($this->spreadsheet->getDefinedNames() as $definedName) { if ($name === $definedName->getName()) { @@ -762,9 +765,9 @@ class Parser $ptgRef = pack('Cvxx', $this->ptg['ptgName'], $nameReference); - throw new WriterException('Cannot yet write formulae with defined names to Xls'); return $ptgRef; + */ } /** @@ -965,7 +968,7 @@ class Parser /** * Advance to the next valid token. */ - private function advance() + private function advance(): void { $token = ''; $i = $this->currentCharacter; @@ -995,7 +998,7 @@ class Parser $this->currentCharacter = $i + 1; $this->currentToken = $token; - return 1; + return; } if ($i < ($formula_length - 2)) { @@ -1035,7 +1038,6 @@ class Parser case '%': return $token; - break; case '>': if ($this->lookAhead === '=') { // it's a GE token break; @@ -1043,7 +1045,6 @@ class Parser return $token; - break; case '<': // it's a LE or a NE token if (($this->lookAhead === '=') || ($this->lookAhead === '>')) { @@ -1052,7 +1053,6 @@ class Parser return $token; - break; default: // if it's a reference A1 or $A$1 or $A1 or A$1 if (preg_match('/^\$?[A-Ia-i]?[A-Za-z]\$?\d+$/', $token) && !preg_match('/\d/', $this->lookAhead) && ($this->lookAhead !== ':') && ($this->lookAhead !== '.') && ($this->lookAhead !== '!')) { @@ -1278,7 +1278,8 @@ class Parser */ private function fact() { - if ($this->currentToken === '(') { + $currentToken = $this->currentToken; + if ($currentToken === '(') { $this->advance(); // eat the "(" $result = $this->parenthesizedExpression(); if ($this->currentToken !== ')') {