Skip to content

Commit 31d0a2e

Browse files
oleibmanoleibman
authored
Eliminate Some Scrutinizer 'Major' Problems (PHPOffice#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.
1 parent befbc56 commit 31d0a2e

File tree

15 files changed

+35
-38
lines changed

15 files changed

+35
-38
lines changed

Diff for: ‎phpstan-baseline.neon

-12
Original file line numberDiff line numberDiff line change
@@ -1966,18 +1966,6 @@ parameters:
19661966
message: "#^Parameter \\#1 \\$underline of static method PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Font\\:\\:mapUnderline\\(\\) expects string, string\\|null given\\.$#"
19671967
count: 1
19681968
path: src/PhpSpreadsheet/Writer/Xls/Font.php
1969-
-
1970-
message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Parser\\:\\:advance\\(\\) has no return type specified\\.$#"
1971-
count: 1
1972-
path: src/PhpSpreadsheet/Writer/Xls/Parser.php
1973-
-
1974-
message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Writer\\\\Xls\\\\Parser\\:\\:\\$spreadsheet has no type specified\\.$#"
1975-
count: 1
1976-
path: src/PhpSpreadsheet/Writer/Xls/Parser.php
1977-
-
1978-
message: "#^Unreachable statement \\- code above always terminates\\.$#"
1979-
count: 5
1980-
path: src/PhpSpreadsheet/Writer/Xls/Parser.php
19811969
-
19821970
message: "#^Cannot access offset 'encoding' on array\\|false\\.$#"
19831971
count: 1

Diff for: ‎src/PhpSpreadsheet/Calculation/Exception.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ class Exception extends PhpSpreadsheetException
1515
* @param mixed $line
1616
* @param mixed $context
1717
*/
18-
public static function errorHandlerCallback($code, $string, $file, $line, $context): void
18+
public static function errorHandlerCallback($code, $string, $file, $line, /** @scrutinizer ignore-unused */ $context): void
1919
{
2020
$e = new self($string, $code);
2121
$e->line = $line;

Diff for: ‎src/PhpSpreadsheet/Collection/Cells.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ public function removeColumn($column): void
352352
private function storeCurrentCell(): void
353353
{
354354
if ($this->currentCellIsDirty && isset($this->currentCoordinate, $this->currentCell)) {
355-
$this->currentCell->detach();
355+
$this->currentCell->/** @scrutinizer ignore-call */ detach();
356356

357357
$stored = $this->cache->set($this->cachePrefix . $this->currentCoordinate, $this->currentCell);
358358
if ($stored === false) {

Diff for: ‎src/PhpSpreadsheet/Reader/Gnumeric.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ public function loadIntoExisting(string $filename, Spreadsheet $spreadsheet): Sp
272272
// name in line with the formula, not the reverse
273273
$this->spreadsheet->getActiveSheet()->setTitle($worksheetName, false, false);
274274

275-
$visibility = $sheetOrNull->attributes()['Visibility'] ?? 'GNM_SHEET_VISIBILITY_VISIBLE';
275+
$visibility = $sheet->attributes()['Visibility'] ?? 'GNM_SHEET_VISIBILITY_VISIBLE';
276276
if ((string) $visibility !== 'GNM_SHEET_VISIBILITY_VISIBLE') {
277277
$this->spreadsheet->getActiveSheet()->setSheetState(Worksheet::SHEETSTATE_HIDDEN);
278278
}

Diff for: ‎src/PhpSpreadsheet/Reader/Gnumeric/Styles.php

+5-2
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ public function read(SimpleXMLElement $sheet, int $maxRow, int $maxCol): void
101101
private function readStyles(SimpleXMLElement $styleRegion, int $maxRow, int $maxCol): void
102102
{
103103
foreach ($styleRegion as $style) {
104+
/** @scrutinizer ignore-call */
104105
$styleAttributes = $style->attributes();
105106
if ($styleAttributes !== null && ($styleAttributes['startRow'] <= $maxRow) && ($styleAttributes['startCol'] <= $maxCol)) {
106107
$cellRange = $this->readStyleRange($styleAttributes, $maxCol, $maxRow);
@@ -117,7 +118,7 @@ private function readStyles(SimpleXMLElement $styleRegion, int $maxRow, int $max
117118
if ($this->readDataOnly === false && $styleAttributes !== null) {
118119
// If readDataOnly is false, we set all formatting information
119120
$styleArray['numberFormat']['formatCode'] = $formatCode;
120-
$styleArray = $this->readStyle($styleArray, $styleAttributes, $style);
121+
$styleArray = $this->readStyle($styleArray, $styleAttributes, /** @scrutinizer ignore-type */ $style);
121122
}
122123
$this->spreadsheet->getActiveSheet()->getStyle($cellRange)->applyFromArray($styleArray);
123124
}
@@ -268,10 +269,12 @@ private function readStyle(array $styleArray, SimpleXMLElement $styleAttributes,
268269
$this->addBorderStyle($srssb, $styleArray, 'right');
269270
$this->addBorderDiagonal($srssb, $styleArray);
270271
}
272+
// TO DO
273+
/*
271274
if (isset($style->Style->HyperLink)) {
272-
// TO DO
273275
$hyperlink = $style->Style->HyperLink->attributes();
274276
}
277+
*/
275278

276279
return $styleArray;
277280
}

Diff for: ‎src/PhpSpreadsheet/Reader/Ods.php

+5-3
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public function canRead(string $filename): bool
5252
if ($zip->open($filename) === true) {
5353
// check if it is an OOXML archive
5454
$stat = $zip->statName('mimetype');
55-
if ($stat && ($stat['size'] <= 255)) {
55+
if (!empty($stat) && ($stat['size'] <= 255)) {
5656
$mimeType = $zip->getFromName($stat['name']);
5757
} elseif ($zip->statName('META-INF/manifest.xml')) {
5858
$xml = simplexml_load_string(
@@ -64,6 +64,7 @@ public function canRead(string $filename): bool
6464
if (isset($namespacesContent['manifest'])) {
6565
$manifest = $xml->children($namespacesContent['manifest']);
6666
foreach ($manifest as $manifestDataSet) {
67+
/** @scrutinizer ignore-call */
6768
$manifestAttributes = $manifestDataSet->attributes($namespacesContent['manifest']);
6869
if ($manifestAttributes && $manifestAttributes->{'full-path'} == '/') {
6970
$mimeType = (string) $manifestAttributes->{'media-type'};
@@ -311,7 +312,7 @@ public function loadIntoExisting($filename, Spreadsheet $spreadsheet)
311312

312313
// Check loadSheetsOnly
313314
if (
314-
isset($this->loadSheetsOnly)
315+
$this->loadSheetsOnly !== null
315316
&& $worksheetName
316317
&& !in_array($worksheetName, $this->loadSheetsOnly)
317318
) {
@@ -507,7 +508,7 @@ public function loadIntoExisting($filename, Spreadsheet $spreadsheet)
507508

508509
$dataValue = Date::PHPToExcel(
509510
strtotime(
510-
'01-01-1970 ' . implode(':', sscanf($timeValue, 'PT%dH%dM%dS') ?? [])
511+
'01-01-1970 ' . implode(':', /** @scrutinizer ignore-type */ sscanf($timeValue, 'PT%dH%dM%dS') ?? [])
511512
)
512513
);
513514
$formatting = NumberFormat::FORMAT_DATE_TIME4;
@@ -693,6 +694,7 @@ protected function scanElementForText(DOMNode $element)
693694

694695
// Multiple spaces?
695696
/** @var DOMAttr $cAttr */
697+
/** @scrutinizer ignore-call */
696698
$cAttr = $child->attributes->getNamedItem('c');
697699
$multiplier = self::getMultiplier($cAttr);
698700
$str .= str_repeat(' ', $multiplier);

Diff for: ‎src/PhpSpreadsheet/Reader/Ods/Properties.php

+1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ public function load(SimpleXMLElement $xml, $namespacesMeta): void
2222
foreach ($officeProperty as $officePropertyData) {
2323
// @var \SimpleXMLElement $officePropertyData
2424
if (isset($namespacesMeta['dc'])) {
25+
/** @scrutinizer ignore-call */
2526
$officePropertiesDC = $officePropertyData->children($namespacesMeta['dc']);
2627
$this->setCoreProperties($docProps, $officePropertiesDC);
2728
}

Diff for: ‎src/PhpSpreadsheet/Style/Border.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public function getStyleArray($array)
103103
/** @var Style */
104104
$parent = $this->parent;
105105

106-
return $parent->getStyleArray([$this->parentPropertyName => $array]);
106+
return $parent->/** @scrutinizer ignore-call */ getStyleArray([$this->parentPropertyName => $array]);
107107
}
108108

109109
/**

Diff for: ‎src/PhpSpreadsheet/Style/Color.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public function getStyleArray($array)
171171
/** @var Style */
172172
$parent = $this->parent;
173173

174-
return $parent->getStyleArray([$this->parentPropertyName => $array]);
174+
return $parent->/** @scrutinizer ignore-call */ getStyleArray([$this->parentPropertyName => $array]);
175175
}
176176

177177
/**

Diff for: ‎src/PhpSpreadsheet/Worksheet/CellIterator.php

+3-3
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,16 @@
22

33
namespace PhpOffice\PhpSpreadsheet\Worksheet;
44

5-
use Iterator;
5+
use Iterator as NativeIterator;
66
use PhpOffice\PhpSpreadsheet\Cell\Cell;
77
use PhpOffice\PhpSpreadsheet\Collection\Cells;
88

99
/**
1010
* @template TKey
1111
*
12-
* @implements Iterator<TKey, Cell>
12+
* @implements NativeIterator<TKey, Cell>
1313
*/
14-
abstract class CellIterator implements Iterator
14+
abstract class CellIterator implements NativeIterator
1515
{
1616
public const TREAT_NULL_VALUE_AS_EMPTY_CELL = 1;
1717

Diff for: ‎src/PhpSpreadsheet/Worksheet/Column.php

+1
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ public function isEmpty(int $definitionOfEmptyFlags = 0): bool
8585
$cellIterator = $this->getCellIterator();
8686
$cellIterator->setIterateOnlyExistingCells(true);
8787
foreach ($cellIterator as $cell) {
88+
/** @scrutinizer ignore-call */
8889
$value = $cell->getValue();
8990
if ($value === null && $nullValueCellIsEmpty === true) {
9091
continue;

Diff for: ‎src/PhpSpreadsheet/Worksheet/ColumnIterator.php

+3-3
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,15 @@
22

33
namespace PhpOffice\PhpSpreadsheet\Worksheet;
44

5-
use Iterator;
5+
use Iterator as NativeIterator;
66
use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
77
use PhpOffice\PhpSpreadsheet\Exception;
88
use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException;
99

1010
/**
11-
* @implements Iterator<string, Column>
11+
* @implements NativeIterator<string, Column>
1212
*/
13-
class ColumnIterator implements Iterator
13+
class ColumnIterator implements NativeIterator
1414
{
1515
/**
1616
* Worksheet to iterate.

Diff for: ‎src/PhpSpreadsheet/Worksheet/Row.php

+1
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public function isEmpty(int $definitionOfEmptyFlags = 0): bool
8484
$cellIterator = $this->getCellIterator();
8585
$cellIterator->setIterateOnlyExistingCells(true);
8686
foreach ($cellIterator as $cell) {
87+
/** @scrutinizer ignore-call */
8788
$value = $cell->getValue();
8889
if ($value === null && $nullValueCellIsEmpty === true) {
8990
continue;

Diff for: ‎src/PhpSpreadsheet/Worksheet/RowIterator.php

+3-3
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@
22

33
namespace PhpOffice\PhpSpreadsheet\Worksheet;
44

5-
use Iterator;
5+
use Iterator as NativeIterator;
66
use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException;
77

88
/**
9-
* @implements Iterator<int, Row>
9+
* @implements NativeIterator<int, Row>
1010
*/
11-
class RowIterator implements Iterator
11+
class RowIterator implements NativeIterator
1212
{
1313
/**
1414
* Worksheet to iterate.

Diff for: ‎src/PhpSpreadsheet/Writer/Xls/Parser.php

+8-7
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,7 @@ class Parser
469469
'BAHTTEXT' => [368, 1, 0, 0],
470470
];
471471

472+
/** @var Spreadsheet */
472473
private $spreadsheet;
473474

474475
/**
@@ -752,6 +753,8 @@ private function convertDefinedName(string $name): string
752753
throw new WriterException('Defined Name is too long');
753754
}
754755

756+
throw new WriterException('Cannot yet write formulae with defined names to Xls');
757+
/*
755758
$nameReference = 1;
756759
foreach ($this->spreadsheet->getDefinedNames() as $definedName) {
757760
if ($name === $definedName->getName()) {
@@ -762,9 +765,9 @@ private function convertDefinedName(string $name): string
762765
763766
$ptgRef = pack('Cvxx', $this->ptg['ptgName'], $nameReference);
764767
765-
throw new WriterException('Cannot yet write formulae with defined names to Xls');
766768
767769
return $ptgRef;
770+
*/
768771
}
769772

770773
/**
@@ -965,7 +968,7 @@ private function cellToRowcol($cell)
965968
/**
966969
* Advance to the next valid token.
967970
*/
968-
private function advance()
971+
private function advance(): void
969972
{
970973
$token = '';
971974
$i = $this->currentCharacter;
@@ -995,7 +998,7 @@ private function advance()
995998
$this->currentCharacter = $i + 1;
996999
$this->currentToken = $token;
9971000

998-
return 1;
1001+
return;
9991002
}
10001003

10011004
if ($i < ($formula_length - 2)) {
@@ -1035,15 +1038,13 @@ private function match($token)
10351038
case '%':
10361039
return $token;
10371040

1038-
break;
10391041
case '>':
10401042
if ($this->lookAhead === '=') { // it's a GE token
10411043
break;
10421044
}
10431045

10441046
return $token;
10451047

1046-
break;
10471048
case '<':
10481049
// it's a LE or a NE token
10491050
if (($this->lookAhead === '=') || ($this->lookAhead === '>')) {
@@ -1052,7 +1053,6 @@ private function match($token)
10521053

10531054
return $token;
10541055

1055-
break;
10561056
default:
10571057
// if it's a reference A1 or $A$1 or $A1 or A$1
10581058
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 @@ private function term()
12781278
*/
12791279
private function fact()
12801280
{
1281-
if ($this->currentToken === '(') {
1281+
$currentToken = $this->currentToken;
1282+
if ($currentToken === '(') {
12821283
$this->advance(); // eat the "("
12831284
$result = $this->parenthesizedExpression();
12841285
if ($this->currentToken !== ')') {

0 commit comments

Comments
 (0)