Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't escape from Worksheet's cloning loop and exhausts memory #437

Closed
1 of 2 tasks
t-iri opened this issue Mar 27, 2018 · 3 comments
Closed
1 of 2 tasks

Can't escape from Worksheet's cloning loop and exhausts memory #437

t-iri opened this issue Mar 27, 2018 · 3 comments
Labels

Comments

@t-iri
Copy link

t-iri commented Mar 27, 2018

This is:

  • a bug report
  • a feature request

File: /PhpSpreadsheet/Worksheet/Worksheet.php
Method: Worksheet->__clone()
Related: #92

What is the expected behavior?

I hope to succeed in copying Worksheet containing relatively many cells but it fails in whiteout as below.

<?php

//Load
require __DIR__ . '/vendor/autoload.php';

$reader = \PhpOffice\PhpSpreadsheet\IOFactory::createReader('Xlsx'); 
$workbook = $reader->load('sample.xlsx');

//Prepare copying
$worksheet = $workbook->getActiveSheet(); //contains cells about 2,000 in this sheet
$worksheet->setTitle('Let me copy', false);

//Time to be copied but
$copysheet = $worksheet->copy(); //Infinite loop occurs...

$copysheet->setCellValue('B2', 'Copied?'); //Not reach here

What is the current behavior?

In Worksheet->__clone(), I can see the problem at 'drawingCollection' logic.

                } elseif ($key == 'drawingCollection') {
                    $newCollection = new ArrayObject();
                    foreach ($this->drawingCollection as $id => $item) {
                        if (is_object($item)) {
This Line! =>               $newCollection[$id] = clone $this->drawingCollection[$id];
                        }
                    }
                    $this->drawingCollection = $newCollection;

When $item is MemoryDrawing, it also has __clone() and try to clone own value. But in case that value is the Worksheet, it causes nested loop(= back to Worksheet->__clone(), to and fro). I haven't copied the sheet never once...

What are the steps to reproduce?

f99eb8d 's Message:

We need to make note of two things:

  • The old code was trying to clone an objectArray, and hence was performing a shallow copy of memoryDrawing objects (the __clone magic function was not getting invoked).
    Instead, if one loops over the objectArray, and then clones the individual memory drawing objects, then the __clone function for the MemoryDrawing object is invoked.

  • The __clone function for memory drawing was using the clone keyword which does not deal with circular references (Since memoryDrawing object had references to worksheet object, it was encountering an infinite loop). However, serializing and unserializing objects deals with circular references pretty well.

The latter is that but I don't grasp whether it has been solved.
I tried old code(previous of the commit), only declaring clone keyword.

                } elseif ($key == 'drawingCollection') {
                    $newCollection = clone $this->drawingCollection;
                    $this->drawingCollection = $newCollection;

It surely goes well, completed copying in my case but this is just a shallow copy? as the message said. So I would appreciate if you have another look at this issue.

Which versions of PhpSpreadsheet and PHP are affected?

PhpSpreadsheet: 1.2.0
PHP: 7.0.16

@stale
Copy link

stale bot commented May 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label May 26, 2018
@stale stale bot closed this as completed Jun 2, 2018
@wnasich
Copy link
Contributor

wnasich commented Jul 22, 2018

I got this issue when the worksheet to clone content an image

@PowerKiKi
Copy link
Member

PowerKiKi commented Jul 22, 2018 via email

PowerKiKi pushed a commit that referenced this issue Aug 5, 2018
When cloning `BaseDrawing`, its worksheet will be set as null and thus be
orphaned. But when cloning the worksheet, it will re-assign itself as the
new worksheet for the BaseDrawing.

That way we avoid recursive cloning of a Worksheet that would clone a
BaseDrawing that would clone a Worksheet etc.

Fixes #437
Fixes #613
Dfred pushed a commit to Dfred/PhpSpreadsheet that referenced this issue Nov 20, 2018
When cloning `BaseDrawing`, its worksheet will be set as null and thus be
orphaned. But when cloning the worksheet, it will re-assign itself as the
new worksheet for the BaseDrawing.

That way we avoid recursive cloning of a Worksheet that would clone a
BaseDrawing that would clone a Worksheet etc.

Fixes PHPOffice#437
Fixes PHPOffice#613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants