From 762d73daf5b4e3072aa5a56f5a42bb0b0bba78b9 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 26 Jul 2024 08:30:52 -0700 Subject: [PATCH 1/3] Addsheet May Leave Active Sheet Uninitialized Fix #4112. Direct cause is that `applyStylesFromArray` tries to save and restore `activeSheetIndex`. However, if activeSheetIndex is -1, indicating no active sheet, the restore should not be attempted. Code is changed to test before attempting to restore. The actual problem, however, is that user specified a sheet number for `addSheet`. That method will set activeSheetIndex most of the time, but this was a gap - when the supplied sheet number (0 in this case) is greater than activeSheetIndex (-1 in this case), it was leaving activeSheetIndex as -1. It is changed to set activeSheetIndex to 0 when activeSheetIndex is negative. --- CHANGELOG.md | 2 +- src/PhpSpreadsheet/Spreadsheet.php | 3 ++ src/PhpSpreadsheet/Worksheet/Worksheet.php | 4 +- .../Worksheet/Issue4112Test.php | 43 +++++++++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php diff --git a/CHANGELOG.md b/CHANGELOG.md index cd609aed93..e645df82e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Fixed -- Nothing +- Add Sheet may leave Active Sheet uninitialized. [Issue #4112](https://github.com/PHPOffice/PhpSpreadsheet/issues/4112) [PR #4113](https://github.com/PHPOffice/PhpSpreadsheet/pull/4113) ## 2024-07-24 - 2.2.0 diff --git a/src/PhpSpreadsheet/Spreadsheet.php b/src/PhpSpreadsheet/Spreadsheet.php index e571cc4f6b..bcea8e6a74 100644 --- a/src/PhpSpreadsheet/Spreadsheet.php +++ b/src/PhpSpreadsheet/Spreadsheet.php @@ -558,6 +558,9 @@ public function addSheet(Worksheet $worksheet, ?int $sheetIndex = null): Workshe if ($this->activeSheetIndex >= $sheetIndex) { ++$this->activeSheetIndex; } + if ($this->activeSheetIndex < 0) { + $this->activeSheetIndex = 0; + } } if ($worksheet->getParent() === null) { diff --git a/src/PhpSpreadsheet/Worksheet/Worksheet.php b/src/PhpSpreadsheet/Worksheet/Worksheet.php index 0bb64ba594..7afa82b5d9 100644 --- a/src/PhpSpreadsheet/Worksheet/Worksheet.php +++ b/src/PhpSpreadsheet/Worksheet/Worksheet.php @@ -3684,7 +3684,9 @@ public function applyStylesFromArray(string $coordinate, array $styleArray): boo $originalSelected = $this->selectedCells; $this->getStyle($coordinate)->applyFromArray($styleArray); $this->selectedCells = $originalSelected; - $spreadsheet->setActiveSheetIndex($activeSheetIndex); + if ($activeSheetIndex >= 0) { + $spreadsheet->setActiveSheetIndex($activeSheetIndex); + } return true; } diff --git a/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php b/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php new file mode 100644 index 0000000000..ec232b08e6 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php @@ -0,0 +1,43 @@ +removeSheetByIndex(0); + $worksheet = new Worksheet($mySpreadsheet, 'addedsheet'); + self::assertSame(-1, $mySpreadsheet->getActiveSheetIndex()); + $mySpreadsheet->addSheet($worksheet, 0); + self::assertSame('addedsheet', $mySpreadsheet->getActiveSheet()->getTitle()); + $row = 1; + $col = 1; + $worksheet->getCell([$col, $row])->setValue('id_uti'); + self::assertSame('id_uti', $worksheet->getCell([$col, $row])->getValue()); + $mySpreadsheet->disconnectWorksheets(); + } + + public static function providerSheetNumber(): array + { + return [ + 'problem case' => [0], + 'normal case' => [null], + 'negative 1 (as if there were no sheets)' => [-1], + 'diffeent negative number' => [-4], + 'positive number' => [4], + ]; + } +} From 1df4b17d55e36d071993ff156a2ff4ed00b97b46 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 26 Jul 2024 08:53:00 -0700 Subject: [PATCH 2/3] Scrutinizer Found a Real Problem My test was imperfect,and Scrutinizer detected it. --- CHANGELOG.md | 2 +- tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e645df82e4..6044ff12f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Fixed -- Add Sheet may leave Active Sheet uninitialized. [Issue #4112](https://github.com/PHPOffice/PhpSpreadsheet/issues/4112) [PR #4113](https://github.com/PHPOffice/PhpSpreadsheet/pull/4113) +- Add Sheet may leave Active Sheet uninitialized. [Issue #4112](https://github.com/PHPOffice/PhpSpreadsheet/issues/4112) [PR #4114](https://github.com/PHPOffice/PhpSpreadsheet/pull/4114) ## 2024-07-24 - 2.2.0 diff --git a/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php b/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php index ec232b08e6..9b230a9c9b 100644 --- a/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php +++ b/tests/PhpSpreadsheetTests/Worksheet/Issue4112Test.php @@ -21,7 +21,7 @@ public function testIssue4112(?int $sheetNumber): void $mySpreadsheet->removeSheetByIndex(0); $worksheet = new Worksheet($mySpreadsheet, 'addedsheet'); self::assertSame(-1, $mySpreadsheet->getActiveSheetIndex()); - $mySpreadsheet->addSheet($worksheet, 0); + $mySpreadsheet->addSheet($worksheet, $sheetNumber); self::assertSame('addedsheet', $mySpreadsheet->getActiveSheet()->getTitle()); $row = 1; $col = 1; From 459f442b9ec62d6b8679788d27d47d69846012d0 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 26 Jul 2024 11:52:35 -0700 Subject: [PATCH 3/3] Additional Test New test testGifIssue4112 uses the same technique as reported in the original issue, and it would fail on all PhpSpreadsheet releases, not just 2.2.0. --- .../Writer/Xls/XlsGifBmpTest.php | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/PhpSpreadsheetTests/Writer/Xls/XlsGifBmpTest.php b/tests/PhpSpreadsheetTests/Writer/Xls/XlsGifBmpTest.php index 74901dd40c..21e64f2a92 100644 --- a/tests/PhpSpreadsheetTests/Writer/Xls/XlsGifBmpTest.php +++ b/tests/PhpSpreadsheetTests/Writer/Xls/XlsGifBmpTest.php @@ -9,6 +9,7 @@ use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Worksheet\Drawing; use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing; +use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional; class XlsGifBmpTest extends AbstractFunctional @@ -83,6 +84,34 @@ public function testGif(): void $reloadedSpreadsheet->disconnectWorksheets(); } + public function testGifIssue4112(): void + { + $spreadsheet = new Spreadsheet(); + $spreadsheet->removeSheetByIndex(0); + $sheet = new Worksheet($spreadsheet, 'Insured List'); + $spreadsheet->addSheet($sheet, 0); + + // Add a drawing to the worksheet + $drawing = new Drawing(); + $drawing->setName('Letters G, I, and G'); + $drawing->setDescription('Handwritten G, I, and F'); + $drawing->setPath(__DIR__ . '/../../../../samples/images/gif.gif'); + $drawing->setHeight(36); + $drawing->setWorksheet($sheet); + $drawing->setCoordinates('A1'); + + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xls'); + $spreadsheet->disconnectWorksheets(); + $worksheet = $reloadedSpreadsheet->getActiveSheet(); + $drawings = $worksheet->getDrawingCollection(); + self::assertCount(1, $drawings); + foreach ($worksheet->getDrawingCollection() as $drawing) { + $mimeType = ($drawing instanceof MemoryDrawing) ? $drawing->getMimeType() : 'notmemorydrawing'; + self::assertEquals('image/png', $mimeType); + } + $reloadedSpreadsheet->disconnectWorksheets(); + } + public function testInvalidTimestamp(): void { $this->expectException(ReaderException::class);