Skip to content

Commit c0809b0

Browse files
oleibmanoleibman
authored
Fix Spreadsheet Copy, Disable Clone, Improve Coverage (#2951)
* Fix Spreadsheet Copy, Disable Clone, Improve Coverage This PR was supposed to be merely to increase coverage in Spreadsheet. However, in doing so, I discovered that neither clone nor copy worked correctly. Neither had been covered in the test suite. Copy not only did not work, it broke the source spreadsheet as well. I tried to debug and got nowhere; I even tried using myclabs/deep-copy which is already in use in the test suite, but it failed as well. However, write and reload ought to work just fine for copy. It can't be used for clone; however, since copy does what clone ought to do, there's no reason why clone needs to be used, so __clone is changed to throw an exception if attempted. One other source change was needed, an obvious bug where an if condition uses 'or' when it should use 'and'. Also, one docblock declaration needed a change. Aside from that, the rest of this PR is test cases, and overall coverage passes 89% for the first time. * Clone is Okay After All But copy wasn't, changing it to just return clone. Perhaps save and reload will be needed instead at some point, but not yet. * An Error I Cannot Reproduce PHP8.1 unit test says error because GdImage can't be serialized. I can't reproduce this error on any of my test systems. I have no idea why GdImage is even involved. Using try/catch to see if it helps. * Weird Failures in Github I thought restoring clone was a good idea. That left me in a state where, after one change, copy/clone no longer worked on Github (unable to reproduce on any of my test systems). After a second change, copy worked but clone didn't, again unable to reproduce. So, reverting to original version - copy does save and reload, clone throws exception.
1 parent e460c82 commit c0809b0

File tree

8 files changed

+297
-33
lines changed

8 files changed

+297
-33
lines changed

‎phpstan-baseline.neon

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2860,11 +2860,6 @@ parameters:
28602860
count: 1
28612861
path: src/PhpSpreadsheet/Spreadsheet.php
28622862

2863-
-
2864-
message: "#^Comparison operation \"\\<\\=\" between int\\<min, \\-1\\> and 1000 is always true\\.$#"
2865-
count: 1
2866-
path: src/PhpSpreadsheet/Spreadsheet.php
2867-
28682863
-
28692864
message: "#^Parameter \\#1 \\$worksheet of method PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet\\:\\:getIndex\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet, PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\|null given\\.$#"
28702865
count: 1
@@ -2875,21 +2870,11 @@ parameters:
28752870
count: 1
28762871
path: src/PhpSpreadsheet/Spreadsheet.php
28772872

2878-
-
2879-
message: "#^Result of \\|\\| is always true\\.$#"
2880-
count: 1
2881-
path: src/PhpSpreadsheet/Spreadsheet.php
2882-
28832873
-
28842874
message: "#^Strict comparison using \\=\\=\\= between PhpOffice\\\\PhpSpreadsheet\\\\Spreadsheet and null will always evaluate to false\\.$#"
28852875
count: 1
28862876
path: src/PhpSpreadsheet/Spreadsheet.php
28872877

2888-
-
2889-
message: "#^Strict comparison using \\=\\=\\= between string and null will always evaluate to false\\.$#"
2890-
count: 1
2891-
path: src/PhpSpreadsheet/Spreadsheet.php
2892-
28932878
-
28942879
message: "#^Unreachable statement \\- code above always terminates\\.$#"
28952880
count: 1

‎src/PhpSpreadsheet/Spreadsheet.php

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,13 @@
33
namespace PhpOffice\PhpSpreadsheet;
44

55
use PhpOffice\PhpSpreadsheet\Calculation\Calculation;
6+
use PhpOffice\PhpSpreadsheet\Reader\Xlsx as XlsxReader;
7+
use PhpOffice\PhpSpreadsheet\Shared\File;
68
use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
79
use PhpOffice\PhpSpreadsheet\Style\Style;
810
use PhpOffice\PhpSpreadsheet\Worksheet\Iterator;
911
use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
12+
use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;
1013

1114
class Spreadsheet
1215
{
@@ -1120,28 +1123,24 @@ public function getWorksheetIterator()
11201123
*/
11211124
public function copy()
11221125
{
1123-
$copied = clone $this;
1126+
$filename = File::temporaryFilename();
1127+
$writer = new XlsxWriter($this);
1128+
$writer->setIncludeCharts(true);
1129+
$writer->save($filename);
11241130

1125-
$worksheetCount = count($this->workSheetCollection);
1126-
for ($i = 0; $i < $worksheetCount; ++$i) {
1127-
$this->workSheetCollection[$i] = $this->workSheetCollection[$i]->copy();
1128-
$this->workSheetCollection[$i]->rebindParent($this);
1129-
}
1131+
$reader = new XlsxReader();
1132+
$reader->setIncludeCharts(true);
1133+
$reloadedSpreadsheet = $reader->load($filename);
1134+
unlink($filename);
11301135

1131-
return $copied;
1136+
return $reloadedSpreadsheet;
11321137
}
11331138

1134-
/**
1135-
* Implement PHP __clone to create a deep clone, not just a shallow copy.
1136-
*/
11371139
public function __clone()
11381140
{
1139-
// @phpstan-ignore-next-line
1140-
foreach ($this as $key => $val) {
1141-
if (is_object($val) || (is_array($val))) {
1142-
$this->{$key} = unserialize(serialize($val));
1143-
}
1144-
}
1141+
throw new Exception(
1142+
'Do not use clone on spreadsheet. Use spreadsheet->copy() instead.'
1143+
);
11451144
}
11461145

11471146
/**
@@ -1562,7 +1561,7 @@ public function getVisibility()
15621561
* Workbook window is hidden and cannot be shown in the
15631562
* user interface.
15641563
*
1565-
* @param string $visibility visibility status of the workbook
1564+
* @param null|string $visibility visibility status of the workbook
15661565
*/
15671566
public function setVisibility($visibility): void
15681567
{
@@ -1596,7 +1595,7 @@ public function getTabRatio()
15961595
*/
15971596
public function setTabRatio($tabRatio): void
15981597
{
1599-
if ($tabRatio >= 0 || $tabRatio <= 1000) {
1598+
if ($tabRatio >= 0 && $tabRatio <= 1000) {
16001599
$this->tabRatio = (int) $tabRatio;
16011600
} else {
16021601
throw new Exception('Tab ratio must be between 0 and 1000.');

‎tests/PhpSpreadsheetTests/DefinedNameTest.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,18 @@ public function testRemoveDefinedName(): void
8585
self::assertCount(1, $this->spreadsheet->getDefinedNames());
8686
}
8787

88+
public function testRemoveGlobalDefinedName(): void
89+
{
90+
$this->spreadsheet->addDefinedName(
91+
DefinedName::createInstance('Any', $this->spreadsheet->getActiveSheet(), '=A1')
92+
);
93+
self::assertCount(1, $this->spreadsheet->getDefinedNames());
94+
95+
$this->spreadsheet->removeDefinedName('Any');
96+
self::assertCount(0, $this->spreadsheet->getDefinedNames());
97+
$this->spreadsheet->removeDefinedName('Other');
98+
}
99+
88100
public function testRemoveGlobalDefinedNameWhenDuplicateNames(): void
89101
{
90102
$this->spreadsheet->addDefinedName(

‎tests/PhpSpreadsheetTests/NamedFormulaTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,10 @@ public function testRemoveScopedNamedFormulaWhenDuplicateNames(): void
133133
$formula->getValue()
134134
);
135135
}
136+
137+
public function testRemoveNonExistentNamedFormula(): void
138+
{
139+
self::assertCount(0, $this->spreadsheet->getNamedFormulae());
140+
$this->spreadsheet->removeNamedFormula('Any');
141+
}
136142
}

‎tests/PhpSpreadsheetTests/NamedRangeTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,10 @@ public function testRemoveScopedNamedRangeWhenDuplicateNames(): void
133133
$range->getValue()
134134
);
135135
}
136+
137+
public function testRemoveNonExistentNamedRange(): void
138+
{
139+
self::assertCount(0, $this->spreadsheet->getNamedRanges());
140+
$this->spreadsheet->removeNamedRange('Any');
141+
}
136142
}

‎tests/PhpSpreadsheetTests/Reader/Xlsx/RibbonTest.php

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,34 @@ public function testRibbon(): void
4444
self::assertNull($reloadedSpreadsheet->getRibbonBinObjects());
4545
$reloadedSpreadsheet->disconnectWorksheets();
4646
}
47+
48+
/**
49+
* Same as above but discard macros.
50+
*/
51+
public function testDiscardMacros(): void
52+
{
53+
$filename = 'tests/data/Reader/XLSX/ribbon.donotopen.zip';
54+
$reader = IOFactory::createReader('Xlsx');
55+
$spreadsheet = $reader->load($filename);
56+
self::assertTrue($spreadsheet->hasRibbon());
57+
$target = $spreadsheet->getRibbonXMLData('target');
58+
self::assertSame('customUI/customUI.xml', $target);
59+
$data = $spreadsheet->getRibbonXMLData('data');
60+
self::assertIsString($data);
61+
self::assertSame(1522, strlen($data));
62+
$vbaCode = (string) $spreadsheet->getMacrosCode();
63+
self::assertSame(13312, strlen($vbaCode));
64+
$spreadsheet->discardMacros();
65+
66+
$reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xlsx');
67+
$spreadsheet->disconnectWorksheets();
68+
self::assertTrue($reloadedSpreadsheet->hasRibbon());
69+
$ribbonData = $reloadedSpreadsheet->getRibbonXmlData();
70+
self::assertIsArray($ribbonData);
71+
self::assertSame($target, $ribbonData['target'] ?? '');
72+
self::assertSame($data, $ribbonData['data'] ?? '');
73+
self::assertNull($reloadedSpreadsheet->getMacrosCode());
74+
self::assertNull($reloadedSpreadsheet->getRibbonBinObjects());
75+
$reloadedSpreadsheet->disconnectWorksheets();
76+
}
4777
}

‎tests/PhpSpreadsheetTests/SpreadsheetCoverageTest.php

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
<?php
2+
3+
namespace PhpOffice\PhpSpreadsheetTests;
4+
5+
use PhpOffice\PhpSpreadsheet\Exception as SSException;
6+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
7+
use PhpOffice\PhpSpreadsheet\Style\Style;
8+
use PHPUnit\Framework\TestCase;
9+
10+
class SpreadsheetCoverageTest extends TestCase
11+
{
12+
public function testDocumentProperties(): void
13+
{
14+
$spreadsheet = new Spreadsheet();
15+
$properties = $spreadsheet->getProperties();
16+
$properties->setCreator('Anyone');
17+
$properties->setTitle('Description');
18+
$spreadsheet2 = new Spreadsheet();
19+
self::assertNotEquals($properties, $spreadsheet2->getProperties());
20+
$properties2 = clone $properties;
21+
$spreadsheet2->setProperties($properties2);
22+
self::assertEquals($properties, $spreadsheet2->getProperties());
23+
$spreadsheet->disconnectWorksheets();
24+
$spreadsheet2->disconnectWorksheets();
25+
}
26+
27+
public function testDocumentSecurity(): void
28+
{
29+
$spreadsheet = new Spreadsheet();
30+
$security = $spreadsheet->getSecurity();
31+
$security->setLockRevision(true);
32+
$revisionsPassword = 'revpasswd';
33+
$security->setRevisionsPassword($revisionsPassword);
34+
$spreadsheet2 = new Spreadsheet();
35+
self::assertNotEquals($security, $spreadsheet2->getSecurity());
36+
$security2 = clone $security;
37+
$spreadsheet2->setSecurity($security2);
38+
self::assertEquals($security, $spreadsheet2->getSecurity());
39+
$spreadsheet->disconnectWorksheets();
40+
$spreadsheet2->disconnectWorksheets();
41+
}
42+
43+
public function testCellXfCollection(): void
44+
{
45+
$spreadsheet = new Spreadsheet();
46+
$sheet = $spreadsheet->getActiveSheet();
47+
$sheet->getStyle('A1')->getFont()->setName('font1');
48+
$sheet->getStyle('A2')->getFont()->setName('font2');
49+
$sheet->getStyle('A3')->getFont()->setName('font3');
50+
$sheet->getStyle('B1')->getFont()->setName('font1');
51+
$sheet->getStyle('B2')->getFont()->setName('font2');
52+
$collection = $spreadsheet->getCellXfCollection();
53+
self::assertCount(4, $collection);
54+
$font1Style = $collection[1];
55+
self::assertTrue($spreadsheet->cellXfExists($font1Style));
56+
self::assertSame('font1', $spreadsheet->getCellXfCollection()[1]->getFont()->getName());
57+
self::assertSame('font1', $sheet->getStyle('A1')->getFont()->getName());
58+
self::assertSame('font2', $sheet->getStyle('A2')->getFont()->getName());
59+
self::assertSame('font3', $sheet->getStyle('A3')->getFont()->getName());
60+
self::assertSame('font1', $sheet->getStyle('B1')->getFont()->getName());
61+
self::assertSame('font2', $sheet->getStyle('B2')->getFont()->getName());
62+
63+
$spreadsheet->removeCellXfByIndex(1);
64+
self::assertFalse($spreadsheet->cellXfExists($font1Style));
65+
self::assertSame('font2', $spreadsheet->getCellXfCollection()[1]->getFont()->getName());
66+
self::assertSame('Calibri', $sheet->getStyle('A1')->getFont()->getName());
67+
self::assertSame('font2', $sheet->getStyle('A2')->getFont()->getName());
68+
self::assertSame('font3', $sheet->getStyle('A3')->getFont()->getName());
69+
self::assertSame('Calibri', $sheet->getStyle('B1')->getFont()->getName());
70+
self::assertSame('font2', $sheet->getStyle('B2')->getFont()->getName());
71+
$spreadsheet->disconnectWorksheets();
72+
}
73+
74+
public function testInvalidRemoveCellXfByIndex(): void
75+
{
76+
$this->expectException(SSException::class);
77+
$this->expectExceptionMessage('CellXf index is out of bounds.');
78+
$spreadsheet = new Spreadsheet();
79+
$sheet = $spreadsheet->getActiveSheet();
80+
$sheet->getStyle('A1')->getFont()->setName('font1');
81+
$sheet->getStyle('A2')->getFont()->setName('font2');
82+
$sheet->getStyle('A3')->getFont()->setName('font3');
83+
$sheet->getStyle('B1')->getFont()->setName('font1');
84+
$sheet->getStyle('B2')->getFont()->setName('font2');
85+
$spreadsheet->removeCellXfByIndex(5);
86+
$spreadsheet->disconnectWorksheets();
87+
}
88+
89+
public function testInvalidRemoveDefaultStyle(): void
90+
{
91+
$this->expectException(SSException::class);
92+
$this->expectExceptionMessage('No default style found for this workbook');
93+
// Removing default style probably should be disallowed.
94+
$spreadsheet = new Spreadsheet();
95+
$sheet = $spreadsheet->getActiveSheet();
96+
$spreadsheet->removeCellXfByIndex(0);
97+
$style = $spreadsheet->getDefaultStyle();
98+
$spreadsheet->disconnectWorksheets();
99+
}
100+
101+
public function testCellStyleXF(): void
102+
{
103+
$spreadsheet = new Spreadsheet();
104+
$collection = $spreadsheet->getCellStyleXfCollection();
105+
self::assertCount(1, $collection);
106+
$styleXf = $collection[0];
107+
self::assertSame($styleXf, $spreadsheet->getCellStyleXfByIndex(0));
108+
$hash = $styleXf->getHashCode();
109+
self::assertSame($styleXf, $spreadsheet->getCellStyleXfByHashCode($hash));
110+
self::assertFalse($spreadsheet->getCellStyleXfByHashCode($hash . 'x'));
111+
$spreadsheet->disconnectWorksheets();
112+
}
113+
114+
public function testInvalidRemoveCellStyleXfByIndex(): void
115+
{
116+
$this->expectException(SSException::class);
117+
$this->expectExceptionMessage('CellStyleXf index is out of bounds.');
118+
$spreadsheet = new Spreadsheet();
119+
$sheet = $spreadsheet->getActiveSheet();
120+
$spreadsheet->removeCellStyleXfByIndex(5);
121+
$spreadsheet->disconnectWorksheets();
122+
}
123+
124+
public function testInvalidFirstSheetIndex(): void
125+
{
126+
$this->expectException(SSException::class);
127+
$this->expectExceptionMessage('First sheet index must be a positive integer.');
128+
$spreadsheet = new Spreadsheet();
129+
$spreadsheet->setFirstSheetIndex(-1);
130+
$spreadsheet->disconnectWorksheets();
131+
}
132+
133+
public function testInvalidVisibility(): void
134+
{
135+
$this->expectException(SSException::class);
136+
$this->expectExceptionMessage('Invalid visibility value.');
137+
$spreadsheet = new Spreadsheet();
138+
$spreadsheet->setVisibility(Spreadsheet::VISIBILITY_HIDDEN);
139+
self::assertSame(Spreadsheet::VISIBILITY_HIDDEN, $spreadsheet->getVisibility());
140+
$spreadsheet->setVisibility(null);
141+
self::assertSame(Spreadsheet::VISIBILITY_VISIBLE, $spreadsheet->getVisibility());
142+
$spreadsheet->setVisibility('badvalue');
143+
$spreadsheet->disconnectWorksheets();
144+
}
145+
146+
public function testInvalidTabRatio(): void
147+
{
148+
$this->expectException(SSException::class);
149+
$this->expectExceptionMessage('Tab ratio must be between 0 and 1000.');
150+
$spreadsheet = new Spreadsheet();
151+
$spreadsheet->setTabRatio(2000);
152+
$spreadsheet->disconnectWorksheets();
153+
}
154+
155+
public function testCopy(): void
156+
{
157+
$spreadsheet = new Spreadsheet();
158+
$sheet = $spreadsheet->getActiveSheet();
159+
$sheet->getStyle('A1')->getFont()->setName('font1');
160+
$sheet->getStyle('A2')->getFont()->setName('font2');
161+
$sheet->getStyle('A3')->getFont()->setName('font3');
162+
$sheet->getStyle('B1')->getFont()->setName('font1');
163+
$sheet->getStyle('B2')->getFont()->setName('font2');
164+
$sheet->getCell('A1')->setValue('this is a1');
165+
$sheet->getCell('A2')->setValue('this is a2');
166+
$sheet->getCell('A3')->setValue('this is a3');
167+
$sheet->getCell('B1')->setValue('this is b1');
168+
$sheet->getCell('B2')->setValue('this is b2');
169+
$copied = $spreadsheet->copy();
170+
$copysheet = $copied->getActiveSheet();
171+
$copysheet->getStyle('A2')->getFont()->setName('font12');
172+
$copysheet->getCell('A2')->setValue('this was a2');
173+
174+
self::assertSame('font1', $sheet->getStyle('A1')->getFont()->getName());
175+
self::assertSame('font2', $sheet->getStyle('A2')->getFont()->getName());
176+
self::assertSame('font3', $sheet->getStyle('A3')->getFont()->getName());
177+
self::assertSame('font1', $sheet->getStyle('B1')->getFont()->getName());
178+
self::assertSame('font2', $sheet->getStyle('B2')->getFont()->getName());
179+
self::assertSame('this is a1', $sheet->getCell('A1')->getValue());
180+
self::assertSame('this is a2', $sheet->getCell('A2')->getValue());
181+
self::assertSame('this is a3', $sheet->getCell('A3')->getValue());
182+
self::assertSame('this is b1', $sheet->getCell('B1')->getValue());
183+
self::assertSame('this is b2', $sheet->getCell('B2')->getValue());
184+
185+
self::assertSame('font1', $copysheet->getStyle('A1')->getFont()->getName());
186+
self::assertSame('font12', $copysheet->getStyle('A2')->getFont()->getName());
187+
self::assertSame('font3', $copysheet->getStyle('A3')->getFont()->getName());
188+
self::assertSame('font1', $copysheet->getStyle('B1')->getFont()->getName());
189+
self::assertSame('font2', $copysheet->getStyle('B2')->getFont()->getName());
190+
self::assertSame('this is a1', $copysheet->getCell('A1')->getValue());
191+
self::assertSame('this was a2', $copysheet->getCell('A2')->getValue());
192+
self::assertSame('this is a3', $copysheet->getCell('A3')->getValue());
193+
self::assertSame('this is b1', $copysheet->getCell('B1')->getValue());
194+
self::assertSame('this is b2', $copysheet->getCell('B2')->getValue());
195+
196+
$spreadsheet->disconnectWorksheets();
197+
$copied->disconnectWorksheets();
198+
}
199+
200+
public function testClone(): void
201+
{
202+
$this->expectException(SSException::class);
203+
$this->expectExceptionMessage('Do not use clone on spreadsheet. Use spreadsheet->copy() instead.');
204+
$spreadsheet = new Spreadsheet();
205+
$clone = clone $spreadsheet;
206+
$spreadsheet->disconnectWorksheets();
207+
$clone->disconnectWorksheets();
208+
}
209+
}

0 commit comments

Comments
 (0)