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

General : Fixed copying bug when presentation had multiple slides #786

Merged
merged 2 commits into from
Dec 13, 2023
Merged

General : Fixed copying bug when presentation had multiple slides #786

merged 2 commits into from
Dec 13, 2023

Conversation

dees040
Copy link
Contributor

@dees040 dees040 commented Dec 12, 2023

If in the original code you would add a dump of the slide collection like the example below and try to copy a presentation with three slides you'll get an exception:

Undefined array key 2

The code:

public function copy(): self
{
    $copied = clone $this;

    $slideCount = count($this->slideCollection);
    for ($i = 0; $i < $slideCount; ++$i) {
        var_dump(array_keys($this->slideCollection));

        $this->slideCollection[$i] = $this->slideCollection[$i]->copy();
        $this->slideCollection[$i]->rebindParent($this);
    }

    return $copied;
}

The dump output:

array:3 [ // vendor/phpoffice/phppresentation/src/PhpPresentation/PhpPresentation.php:325
  0 => 0
  1 => 1
  2 => 2
]
array:2 [ // vendor/phpoffice/phppresentation/src/PhpPresentation/PhpPresentation.php:325
  0 => 0
  1 => 1
]
array:1 [ // vendor/phpoffice/phppresentation/src/PhpPresentation/PhpPresentation.php:325
  0 => 0
]

Because of the rebindParent() which is being called on the copied slide, the copied presentation's original $slideCollection is being edited. This causes the code to break. This PR fixes that problem by saving the original $slideCollection before starting the slide copying process. Afterwards we set the new slide collection to the new copied presentation and also revert the original contact of the old presentation which is being copied.

I've also updated the unit test so that a presentation with multiple slides is being copied. If you test the unit test without the changes in PhpOffice\PhpPresentation\PhpPresentation you can also see the exception being thrown:

There was 1 error:

  1. PhpOffice\PhpPresentation\Tests\PhpPresentationTest::testCopy
    Error: Call to a member function copy() on null

/PHPPresentation/src/PhpPresentation/PhpPresentation.php:316
/PHPPresentation/tests/PhpPresentation/Tests/PhpPresentationTest.php:90

ERRORS!
Tests: 1, Assertions: 0, Errors: 1, Warnings: 2, Deprecations: 1.

Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dees040 I approve your PR. Could you update the changelog file 1.1.0.md before I merge it ?

@Progi1984 Progi1984 added this to the 1.1.0 milestone Dec 12, 2023
@dees040
Copy link
Contributor Author

dees040 commented Dec 13, 2023

@Progi1984 thanks! And of course. I wasn't sure what to put before the semicolon so I just did General. Let me know if I should change that to something else.

Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dees040 Perfect

@Progi1984 Progi1984 changed the title fix: presentation with multiple slides should be copyable General : Fixed copying bug when presentation had multiple slides Dec 13, 2023
@Progi1984 Progi1984 merged commit dae3f3d into PHPOffice:develop Dec 13, 2023
28 checks passed
@Progi1984
Copy link
Member

🎉 Thank you @dees040 for your contribution 🎉

@dees040 dees040 deleted the bug/copy branch December 13, 2023 09:56
@dees040
Copy link
Contributor Author

dees040 commented Jan 11, 2024

My pleasure @Progi1984! Any chance version 1.1 can be tagged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants