Skip to content

Commit

Permalink
Break Some More Circular References (#3716)
Browse files Browse the repository at this point in the history
* Break Some More Circular References

After identifying a circular reference between Worksheet and Table, which could lead to memory leaks, I identified several other similar relationships. I have added a destructor, or added added code to an existing destructor, for those possibilities. This didn't actually lead to any lowering of the memory usage of the unit test suite, so maybe there's no practical benefit, but it does seem like it should be theoretically useful. I am not aware of any practical way to unit test a destructor, although the added code will be exercised many times in most of our unit tests.

The only change here not involving a destructor is to `Worksheet::addChart` (it came to my attention because Worksheet and Chart have a circular reference). It is currently defined with two arguments, the second allowing you to place the chart in a specific location in the containing ArrayObject. However, as Phpstan and Scrutinizer both warn, the code to support this option will fail if it is ever executed (can't use array_splice on an ArrayObject); needless to say, this possibility is not exercised when executing the test suite. It could be made to work, but I really can't see any kind of use case to support this parameter. So I'm removing the second parameter. In theory, this is a breaking change, but, since the code can never work, in practice it won't be.

* Update Worksheet.php

* Update Worksheet.php
  • Loading branch information
oleibman authored Sep 13, 2023
1 parent 72b7702 commit f17e166
Show file tree
Hide file tree
Showing 11 changed files with 42 additions and 15 deletions.
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Cell/CellAddress.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ public function __construct(string $cellAddress, ?Worksheet $worksheet = null)
$this->worksheet = $worksheet;
}

public function __destruct()
{
$this->worksheet = null;
}

private static function validateColumnAndRow(mixed $columnId, mixed $rowId): void
{
if (!is_numeric($columnId) || $columnId <= 0 || !is_numeric($rowId) || $rowId <= 0) {
Expand Down
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Cell/ColumnRange.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ public function __construct(string $from, ?string $to = null, ?Worksheet $worksh
$this->worksheet = $worksheet;
}

public function __destruct()
{
$this->worksheet = null;
}

public static function fromColumnIndexes(int $from, int $to, ?Worksheet $worksheet = null): self
{
return new self(Coordinate::stringFromColumnIndex($from), Coordinate::stringFromColumnIndex($to), $worksheet);
Expand Down
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Cell/RowRange.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ public function __construct(int $from, ?int $to = null, ?Worksheet $worksheet =
$this->worksheet = $worksheet;
}

public function __destruct()
{
$this->worksheet = null;
}

public static function fromArray(array $array, ?Worksheet $worksheet = null): self
{
[$from, $to] = $array;
Expand Down
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Chart/Chart.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ public function __construct(mixed $name, ?Title $title = null, ?Legend $legend =
$this->borderLines = new GridLines();
}

public function __destruct()
{
$this->worksheet = null;
}

/**
* Get Name.
*
Expand Down
1 change: 1 addition & 0 deletions src/PhpSpreadsheet/Collection/Cells.php
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ public function unsetWorksheetCells(): void
public function __destruct()
{
$this->cache->deleteMultiple($this->getAllCacheKeys());
$this->parent = null;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/PhpSpreadsheet/DefinedName.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ public function __construct(
$this->isFormula = self::testIfFormula($this->value);
}

public function __destruct()
{
$this->worksheet = null;
$this->scope = null;
}

/**
* Create a new defined name, either a range or a formula.
*/
Expand Down
1 change: 1 addition & 0 deletions src/PhpSpreadsheet/Spreadsheet.php
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ public function __destruct()
$this->calculationEngine = null;
$this->cellXfCollection = [];
$this->cellStyleXfCollection = [];
$this->definedNames = [];
}

/**
Expand Down
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Worksheet/AutoFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ public function __construct($range = '', ?Worksheet $worksheet = null)
$this->workSheet = $worksheet;
}

public function __destruct()
{
$this->workSheet = null;
}

/**
* Get AutoFilter Parent Worksheet.
*
Expand Down
5 changes: 5 additions & 0 deletions src/PhpSpreadsheet/Worksheet/BaseDrawing.php
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ public function __construct()
$this->imageIndex = self::$imageCounter;
}

public function __destruct()
{
$this->worksheet = null;
}

public function getImageIndex(): int
{
return $this->imageIndex;
Expand Down
1 change: 1 addition & 0 deletions src/PhpSpreadsheet/Worksheet/MemoryDrawing.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public function __destruct()
@imagedestroy($this->imageResource);
$this->imageResource = null;
}
$this->worksheet = null;
}

public function __clone()
Expand Down
18 changes: 3 additions & 15 deletions src/PhpSpreadsheet/Worksheet/Worksheet.php
Original file line number Diff line number Diff line change
Expand Up @@ -423,8 +423,7 @@ public function __destruct()
Calculation::getInstance($this->parent)->clearCalculationCacheForWorksheet($this->title);

$this->disconnectCells();
$this->rowDimensions = [];
$this->tableCollection = new ArrayObject();
unset($this->rowDimensions, $this->columnDimensions, $this->tableCollection, $this->drawingCollection, $this->chartCollection, $this->autoFilter);
}

/**
Expand Down Expand Up @@ -589,21 +588,10 @@ public function getChartCollection()
return $this->chartCollection;
}

/**
* Add chart.
*
* @param null|int $chartIndex Index where chart should go (0,1,..., or null for last)
*/
public function addChart(Chart $chart, $chartIndex = null): Chart
public function addChart(Chart $chart): Chart
{
$chart->setWorksheet($this);
if ($chartIndex === null) {
$this->chartCollection[] = $chart;
} else {
// Insert the chart at the requested index
// @phpstan-ignore-next-line
array_splice(/** @scrutinizer ignore-type */ $this->chartCollection, $chartIndex, 0, [$chart]);
}
$this->chartCollection[] = $chart;

return $chart;
}
Expand Down

0 comments on commit f17e166

Please sign in to comment.