From 49c92900d305cbb256db6ee4922a1e336d1c09ce Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Tue, 22 Jun 2021 07:09:45 -0700 Subject: [PATCH 1/4] Reader/Gnumeric vs. Scrutinizer 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. I believe this is the only one with which will involve more than 2 or 3 changes. It fixes 5 items ascribed to me, and 4 to others. --- src/PhpSpreadsheet/Reader/Gnumeric.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 64c27366dd..8afbac7eed 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -266,9 +266,10 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S (new Properties($this->spreadsheet))->readProperties($xml, $gnmXML); $worksheetID = 0; - foreach ($gnmXML->Sheets->Sheet as $sheet) { + foreach ($gnmXML->Sheets->Sheet as $sheetOrNull) { + $sheet = self::testSimpleXml($sheetOrNull); $worksheetName = (string) $sheet->Name; - if ((isset($this->loadSheetsOnly)) && (!in_array($worksheetName, $this->loadSheetsOnly))) { + if (is_array($this->loadSheetsOnly) && !in_array($worksheetName, $this->loadSheetsOnly)) { continue; } @@ -288,8 +289,9 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S ->sheetMargins($sheet); } - foreach ($sheet->Cells->Cell as $cell) { - $cellAttributes = $cell->attributes(); + foreach ($sheet->Cells->Cell as $cellOrNull) { + $cell = self::testSimpleXml($cellOrNull); + $cellAttributes = self::testSimpleXml($cell->attributes()); $row = (int) $cellAttributes->Row + 1; $column = (int) $cellAttributes->Col; @@ -367,7 +369,7 @@ private function processMergedCells(?SimpleXMLElement $sheet): void // Handle Merged Cells in this worksheet if ($sheet !== null && isset($sheet->MergedRegions)) { foreach ($sheet->MergedRegions->Merge as $mergeCells) { - if (strpos($mergeCells, ':') !== false) { + if (strpos((string) $mergeCells, ':') !== false) { $this->spreadsheet->getActiveSheet()->mergeCells($mergeCells); } } @@ -404,8 +406,9 @@ private function setColumnInvisible(int $whichColumn): void } } - private function processColumnLoop(int $whichColumn, int $maxCol, SimpleXMLElement $columnOverride, float $defaultWidth): int + private function processColumnLoop(int $whichColumn, int $maxCol, ?SimpleXMLElement $columnOverride, float $defaultWidth): int { + $columnOverride = self::testSimpleXml($columnOverride); $columnAttributes = self::testSimpleXml($columnOverride->attributes()); $column = $columnAttributes['No']; $columnWidth = ((float) $columnAttributes['Unit']) / 5.4; @@ -462,8 +465,9 @@ private function setRowInvisible(int $whichRow): void } } - private function processRowLoop(int $whichRow, int $maxRow, SimpleXMLElement $rowOverride, float $defaultHeight): int + private function processRowLoop(int $whichRow, int $maxRow, ?SimpleXMLElement $rowOverride, float $defaultHeight): int { + $rowOverride = self::testSimpleXml($rowOverride); $rowAttributes = self::testSimpleXml($rowOverride->attributes()); $row = $rowAttributes['No']; $rowHeight = (float) $rowAttributes['Unit']; From 416c360eb8c7efa75ff80ed0b1c6a3827f096e1a Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Thu, 24 Jun 2021 06:53:08 -0700 Subject: [PATCH 2/4] Use Strict Checking for in_array Per suggestion from Mark Baker. --- src/PhpSpreadsheet/Reader/Gnumeric.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 8afbac7eed..438faea69b 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -269,7 +269,7 @@ public function loadIntoExisting(string $pFilename, Spreadsheet $spreadsheet): S foreach ($gnmXML->Sheets->Sheet as $sheetOrNull) { $sheet = self::testSimpleXml($sheetOrNull); $worksheetName = (string) $sheet->Name; - if (is_array($this->loadSheetsOnly) && !in_array($worksheetName, $this->loadSheetsOnly)) { + if (is_array($this->loadSheetsOnly) && !in_array($worksheetName, $this->loadSheetsOnly, true)) { continue; } From 36aec67ca1bbb4738d42654b5f5ca45ec025766f Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sat, 26 Jun 2021 12:20:57 -0700 Subject: [PATCH 3/4] Correct Some Problems Which Will Show Up for PHP8.1 PHP8.1 wants to issue a message when you use a float where it thinks you ought to be using an int (it wants its implicit casts made explicit). This is causing unit tests to fail. The following corrections are made in this PR: - Calculation.php tests `isset(self::binaryOperators[$token])`, where token can be a float. No numeric values are members of that array, so we can test for numeric before isset. - SharedOle.php packs a float, intending it as an int, in 2 places. I simplified the logic here, and added explicit casts to avoid the problem. This is used by Xls Reader and Writer; as added confirmation, I added some timestamps from before 1970 (i.e. negative values) to Document/EpochTest. Because of this, the test suite has been verified for 32-bit PHP as well as PHP 8.1. - Writer/Xlsx/StringTable tests `isset($aFlippedStringTable[$cellValue])`. This is the same problem as in Calculation, but requires a different solution. The same if statement here also tests that the datatype is string, but it does so after the isset test. Changing the order of these tests avoids the problem. There remain PHP8.1 errors having to do with vendor code used as a polyfill for Enum. @MarkBaker has reported the issue to the vendor. --- .../Calculation/Calculation.php | 2 +- src/PhpSpreadsheet/Shared/OLE.php | 19 ++++++------------- .../Writer/Xlsx/StringTable.php | 4 ++-- .../Document/EpochTest.php | 3 +++ 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index eb11143763..0d771ff77c 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -4477,7 +4477,7 @@ private function processTokenStack($tokens, $cellID = null, ?Cell $pCell = null) } // if the token is a binary operator, pop the top two values off the stack, do the operation, and push the result back on the stack - if (isset(self::$binaryOperators[$token])) { + if (!is_numeric($token) && isset(self::$binaryOperators[$token])) { // We must have two operands, error if we don't if (($operand2Data = $stack->pop()) === null) { return $this->raiseFormulaError('Internal error - Operand value missing from stack'); diff --git a/src/PhpSpreadsheet/Shared/OLE.php b/src/PhpSpreadsheet/Shared/OLE.php index 8ecfc6be5c..eb0b978828 100644 --- a/src/PhpSpreadsheet/Shared/OLE.php +++ b/src/PhpSpreadsheet/Shared/OLE.php @@ -512,22 +512,15 @@ public static function localDateToOLE($date) // multiply just to make MS happy $big_date *= 10000000; - $high_part = floor($big_date / $factor); - // lower 4 bytes - $low_part = floor((($big_date / $factor) - $high_part) * $factor); - // Make HEX string $res = ''; - for ($i = 0; $i < 4; ++$i) { - $hex = $low_part % 0x100; - $res .= pack('c', $hex); - $low_part /= 0x100; - } - for ($i = 0; $i < 4; ++$i) { - $hex = $high_part % 0x100; - $res .= pack('c', $hex); - $high_part /= 0x100; + $factor = 2 ** 56; + while ($factor >= 1) { + $hex = (int) floor($big_date / $factor); + $res = pack('c', $hex) . $res; + $big_date = fmod($big_date, $factor); + $factor /= 256; } return $res; diff --git a/src/PhpSpreadsheet/Writer/Xlsx/StringTable.php b/src/PhpSpreadsheet/Writer/Xlsx/StringTable.php index b0f7d6d417..b73d53cca6 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx/StringTable.php +++ b/src/PhpSpreadsheet/Writer/Xlsx/StringTable.php @@ -42,8 +42,8 @@ public function createStringTable(Worksheet $pSheet, $pExistingTable = null) !is_object($cellValue) && ($cellValue !== null) && $cellValue !== '' && - !isset($aFlippedStringTable[$cellValue]) && - ($cell->getDataType() == DataType::TYPE_STRING || $cell->getDataType() == DataType::TYPE_STRING2 || $cell->getDataType() == DataType::TYPE_NULL) + ($cell->getDataType() == DataType::TYPE_STRING || $cell->getDataType() == DataType::TYPE_STRING2 || $cell->getDataType() == DataType::TYPE_NULL) && + !isset($aFlippedStringTable[$cellValue]) ) { $aStringTable[] = $cellValue; $aFlippedStringTable[$cellValue] = true; diff --git a/tests/PhpSpreadsheetTests/Document/EpochTest.php b/tests/PhpSpreadsheetTests/Document/EpochTest.php index 63c51a59d0..5ea5c5a87c 100644 --- a/tests/PhpSpreadsheetTests/Document/EpochTest.php +++ b/tests/PhpSpreadsheetTests/Document/EpochTest.php @@ -11,10 +11,13 @@ class EpochTest extends AbstractFunctional public function providerFormats(): array { return [ + ['Ods', '1921-03-17 11:30:00Z'], ['Ods', '2021-03-17 11:30:00Z'], ['Ods', '2041-03-17 11:30:00Z'], + ['Xls', '1921-03-17 11:30:00Z'], ['Xls', '2021-03-17 11:30:00Z'], ['Xls', '2041-03-17 11:30:00Z'], + ['Xlsx', '1921-03-17 11:30:00Z'], ['Xlsx', '2021-03-17 11:30:00Z'], ['Xlsx', '2041-03-17 11:30:00Z'], ]; From 7787bef4b007f07a6e735a281ecc1f0becd71917 Mon Sep 17 00:00:00 2001 From: oleibman Date: Sat, 26 Jun 2021 12:44:08 -0700 Subject: [PATCH 4/4] Update OLE.php --- src/PhpSpreadsheet/Shared/OLE.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/PhpSpreadsheet/Shared/OLE.php b/src/PhpSpreadsheet/Shared/OLE.php index eb0b978828..6278553f6e 100644 --- a/src/PhpSpreadsheet/Shared/OLE.php +++ b/src/PhpSpreadsheet/Shared/OLE.php @@ -502,9 +502,6 @@ public static function localDateToOLE($date) } $dateTime = Date::dateTimeFromTimestamp("$date"); - // factor used for separating numbers into 4 bytes parts - $factor = 2 ** 32; - // days from 1-1-1601 until the beggining of UNIX era $days = 134774; // calculate seconds