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

Refactor xlsx writer #1922

Merged
merged 8 commits into from
Apr 6, 2021
Merged

Conversation

ccrims0n
Copy link
Contributor

@ccrims0n ccrims0n commented Mar 14, 2021

Fix #1911

  • Move the writer part to the end of the save()

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

@ccrims0n ccrims0n closed this Mar 14, 2021
* Move file handler creation and file addition to the end
@ccrims0n ccrims0n reopened this Mar 14, 2021
@ccrims0n ccrims0n changed the title Update the fopen mode for writer to wb Refactor xlsx writer Mar 14, 2021
@ccrims0n ccrims0n marked this pull request as ready for review March 14, 2021 17:01
@ccrims0n
Copy link
Contributor Author

@MarkBaker Can someone help with reviewing this PR

@oleibman
Copy link
Collaborator

oleibman commented Apr 5, 2021

Can you please make a minor change to your test? Rather than editing your test file in place, copy it to the temporary directory (file_get_contents followed by file_put_contents should suffice), run your test against the temporary copy, and delete the temporary copy at the end. The difference between this and your existing test is that, if something goes wrong, we don't lose the good copy of the test file.

@ccrims0n
Copy link
Contributor Author

ccrims0n commented Apr 5, 2021

@oleibman perfect. will make the changes.

Edit: This is done

@ccrims0n ccrims0n force-pushed the fix_excel_overwrite branch from 1dafa16 to 59de56b Compare April 5, 2021 06:56
@oleibman
Copy link
Collaborator

oleibman commented Apr 5, 2021

Please unlink the temporary file at the end of your test, and I think this will be good to go.
@MarkBaker and @PowerKiKi , after this last change is made, I would like to proceed with merging this change. Since it will be my first time doing that (I am reviewing one other), I just want you to alert you about that in case something goes amiss.

@PowerKiKi
Copy link
Member

in case something goes amiss

As long as GitHub is all green, merging should be fine. But I'll keep an eye open...

@oleibman oleibman merged commit 063496d into PHPOffice:master Apr 6, 2021
@ccrims0n ccrims0n deleted the fix_excel_overwrite branch April 6, 2021 05:47
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.

Error when saving an Xlsx file (with images) with the same name!
3 participants