Skipping to clone Worksheet object when cloning DrawingCollection #613
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PowerKiKi
requested changes
Jul 25, 2018
| { | ||
| public function testCloningWorksheetWithImages() | ||
| { | ||
| $filename = './data/Reader/Xls/with_images.xls'; |
There was a problem hiding this comment.
Please refrain from adding binary files to the repo and instead create them from scratch, in memory with PhpSpreadsheet.
| $clonedWorksheet = clone $worksheet; | ||
| $clonedDrawingCount = count($clonedWorksheet->getDrawingCollection()); | ||
| self::assertEquals($originDrawingCount, $clonedDrawingCount); | ||
| } |
There was a problem hiding this comment.
You should assert that the drawing collection is not the exact same object and was indeed cloned. Probably with assertNotSame.
And same for worksheet, it must be cloned. So the patch might be too simple as it is now.
…seDrawing:: Iterating over a copy of Worksheet:: because the proper way to add BaseDrawing to a Worksheet already add it to their drawingCollection attribute and in this case the foreach got a infinte loop
…ng cloned objects exist and they are differents
|
@PowerKiKi thanks for your feedback, I completed your suggestions I think |
|
Thanks ! it's been merged |
Dfred
pushed a commit
to Dfred/PhpSpreadsheet
that referenced
this pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is:
Checklist:
Why this change is needed?
Fixing issue #437