From 2ae948a31915c19e2748d26845eee8f4bb012302 Mon Sep 17 00:00:00 2001 From: oleibman Date: Tue, 29 Jun 2021 11:48:31 -0700 Subject: [PATCH] Reader/Slk vs. Scrutinizer/Phpstan (#2192) Just reviewing Scrutinizer's list of "bugs". There are 19 ascribed to me. For some, I will definitely take no action (e.g. use of bitwise operators in AND, OR, and XOR functions). However, where I can clean things up so that Scrutinizer is satisfied and the resulting code is not too contorted, I will make an attempt. This PR corrects 3 problems (2 mine) according to Scrutinizer, and 7 per Phpstan. It also moves the Reader Slk tests under their own directory, as is the case for all the other Reader types. --- phpstan-baseline.neon | 35 ------------------- src/PhpSpreadsheet/Reader/Slk.php | 33 ++++++++--------- .../Reader/{ => Slk}/SlkTest.php | 9 +++-- 3 files changed, 23 insertions(+), 54 deletions(-) rename tests/PhpSpreadsheetTests/Reader/{ => Slk}/SlkTest.php (96%) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 3538d091c..54c05805d 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -2565,41 +2565,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Reader/Security/XmlScanner.php - - - message: "#^Parameter \\#1 \\$haystack of function substr_count expects string, string\\|false given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Parameter \\#2 \\$str of function explode expects string, string\\|false given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Slk\\:\\:\\$colorArray has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Slk\\:\\:\\$fontStyleMappings has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Slk\\:\\:\\$styleSettingsFont has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Reader\\\\Slk\\:\\:\\$styleSettingsBorder has no typehint specified\\.$#" - count: 1 - path: src/PhpSpreadsheet/Reader/Slk.php - - - - message: "#^Parameter \\#1 \\$columnIndex of static method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Coordinate\\:\\:stringFromColumnIndex\\(\\) expects int, string given\\.$#" - count: 3 - path: src/PhpSpreadsheet/Reader/Slk.php - - message: "#^Call to an undefined method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DgContainer\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DgContainer\\\\SpgrContainer\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DgContainer\\\\SpgrContainer\\\\SpContainer\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DggContainer\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DggContainer\\\\BstoreContainer\\|PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Escher\\\\DggContainer\\\\BstoreContainer\\\\BSE\\:\\:getDgContainer\\(\\)\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Reader/Slk.php b/src/PhpSpreadsheet/Reader/Slk.php index b58fdcba9..4e76a1616 100644 --- a/src/PhpSpreadsheet/Reader/Slk.php +++ b/src/PhpSpreadsheet/Reader/Slk.php @@ -79,7 +79,7 @@ class Slk extends BaseReader } // Read sample data (first 2 KB will do) - $data = fread($this->fileHandle, 2048); + $data = (string) fread($this->fileHandle, 2048); // Count delimiters in file $delimiterCount = substr_count($data, ';'); @@ -210,7 +210,7 @@ class Slk extends BaseReader return $this->loadIntoExisting($pFilename, $spreadsheet); } - private $colorArray = [ + private const COLOR_ARRAY = [ 'FF00FFFF', // 0 - cyan 'FF000000', // 1 - black 'FFFFFFFF', // 2 - white @@ -221,7 +221,7 @@ class Slk extends BaseReader 'FFFF00FF', // 7 - magenta ]; - private $fontStyleMappings = [ + private const FONT_STYLE_MAPPINGS = [ 'B' => 'bold', 'I' => 'italic', 'U' => 'underline', @@ -235,7 +235,8 @@ class Slk extends BaseReader $key = false; foreach ($temp as &$value) { // Only count/replace in alternate array entries - if ($key = !$key) { + $key = !$key; + if ($key) { preg_match_all('/(R(\[?-?\d*\]?))(C(\[?-?\d*\]?))/', $value, $cellReferences, PREG_SET_ORDER + PREG_OFFSET_CAPTURE); // Reverse the matches array, otherwise all our offsets will become incorrect if we modify our way // through the formula from left to right. Reversing means that we work right to left.through @@ -357,9 +358,9 @@ class Slk extends BaseReader $this->addWidth($spreadsheet, $columnWidth, $startCol, $endCol); } - private $styleSettingsFont = ['D' => 'bold', 'I' => 'italic']; + private const STYLE_SETTINGS_FONT = ['D' => 'bold', 'I' => 'italic']; - private $styleSettingsBorder = [ + private const STYLE_SETTINGS_BORDER = [ 'B' => 'bottom', 'L' => 'left', 'R' => 'right', @@ -372,10 +373,10 @@ class Slk extends BaseReader $iMax = strlen($styleSettings); for ($i = 0; $i < $iMax; ++$i) { $char = $styleSettings[$i]; - if (array_key_exists($char, $this->styleSettingsFont)) { - $styleData['font'][$this->styleSettingsFont[$char]] = true; - } elseif (array_key_exists($char, $this->styleSettingsBorder)) { - $styleData['borders'][$this->styleSettingsBorder[$char]]['borderStyle'] = Border::BORDER_THIN; + if (array_key_exists($char, self::STYLE_SETTINGS_FONT)) { + $styleData['font'][self::STYLE_SETTINGS_FONT[$char]] = true; + } elseif (array_key_exists($char, self::STYLE_SETTINGS_BORDER)) { + $styleData['borders'][self::STYLE_SETTINGS_BORDER[$char]]['borderStyle'] = Border::BORDER_THIN; } elseif ($char == 'S') { $styleData['fill']['fillType'] = \PhpOffice\PhpSpreadsheet\Style\Fill::FILL_PATTERN_GRAY125; } elseif ($char == 'M') { @@ -409,7 +410,7 @@ class Slk extends BaseReader private function addStyle(Spreadsheet &$spreadsheet, array $styleData, string $row, string $column): void { if ((!empty($styleData)) && $column > '' && $row > '') { - $columnLetter = Coordinate::stringFromColumnIndex($column); + $columnLetter = Coordinate::stringFromColumnIndex((int) $column); $spreadsheet->getActiveSheet()->getStyle($columnLetter . $row)->applyFromArray($styleData); } } @@ -421,8 +422,8 @@ class Slk extends BaseReader $startCol = Coordinate::stringFromColumnIndex((int) $startCol); $spreadsheet->getActiveSheet()->getColumnDimension($startCol)->setWidth((float) $columnWidth); } else { - $startCol = Coordinate::stringFromColumnIndex($startCol); - $endCol = Coordinate::stringFromColumnIndex($endCol); + $startCol = Coordinate::stringFromColumnIndex((int) $startCol); + $endCol = Coordinate::stringFromColumnIndex((int) $endCol); $spreadsheet->getActiveSheet()->getColumnDimension($startCol)->setWidth((float) $columnWidth); do { $spreadsheet->getActiveSheet()->getColumnDimension(++$startCol)->setWidth((float) $columnWidth); @@ -469,7 +470,7 @@ class Slk extends BaseReader { if (preg_match('/L([1-9]\\d*)/', $rowDatum, $matches)) { $fontColor = $matches[1] % 8; - $formatArray['font']['color']['argb'] = $this->colorArray[$fontColor]; + $formatArray['font']['color']['argb'] = self::COLOR_ARRAY[$fontColor]; } } @@ -478,8 +479,8 @@ class Slk extends BaseReader $styleSettings = substr($rowDatum, 1); $iMax = strlen($styleSettings); for ($i = 0; $i < $iMax; ++$i) { - if (array_key_exists($styleSettings[$i], $this->fontStyleMappings)) { - $formatArray['font'][$this->fontStyleMappings[$styleSettings[$i]]] = true; + if (array_key_exists($styleSettings[$i], self::FONT_STYLE_MAPPINGS)) { + $formatArray['font'][self::FONT_STYLE_MAPPINGS[$styleSettings[$i]]] = true; } } } diff --git a/tests/PhpSpreadsheetTests/Reader/SlkTest.php b/tests/PhpSpreadsheetTests/Reader/Slk/SlkTest.php similarity index 96% rename from tests/PhpSpreadsheetTests/Reader/SlkTest.php rename to tests/PhpSpreadsheetTests/Reader/Slk/SlkTest.php index 6881648d5..3a7dff6c4 100644 --- a/tests/PhpSpreadsheetTests/Reader/SlkTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Slk/SlkTest.php @@ -1,6 +1,6 @@ filename) { + if ($this->filename !== '') { unlink($this->filename); $this->filename = ''; } @@ -134,6 +134,7 @@ class SlkTest extends \PHPUnit\Framework\TestCase self::assertEquals(Border::BORDER_THIN, $sheet->getCell('C18')->getStyle()->getBorders()->getBottom()->getBorderStyle()); self::assertEquals(Border::BORDER_THIN, $sheet->getCell('C18')->getStyle()->getBorders()->getLeft()->getBorderStyle()); // Have not yet figured out how C6/C7 are centred + $spreadsheet->disconnectWorksheets(); } public function testSheetIndex(): void @@ -147,6 +148,7 @@ class SlkTest extends \PHPUnit\Framework\TestCase self::assertEquals('SylkTest', $sheet->getTitle()); self::assertEquals('FFFF0000', $sheet->getCell('A1')->getStyle()->getFont()->getColor()->getARGB()); + $spreadsheet->disconnectWorksheets(); } public function testLongName(): void @@ -160,5 +162,6 @@ class SlkTest extends \PHPUnit\Framework\TestCase $sheet = $spreadsheet->getActiveSheet(); self::assertEquals('123456789a123456789b123456789c1', $sheet->getTitle()); self::assertEquals('FFFF0000', $sheet->getCell('A1')->getStyle()->getFont()->getColor()->getARGB()); + $spreadsheet->disconnectWorksheets(); } }