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

Filesize estimate #11

Merged
merged 5 commits into from
Nov 19, 2019
Merged

Conversation

Firtzberg
Copy link
Contributor

Improves estimation of file sizes and resulting zip stream.

Accurate estimation requires the Archive options to be provided. therefore the FileContract was modified.

@jszobody jszobody mentioned this pull request Oct 21, 2019
@Firtzberg
Copy link
Contributor Author

in the ZipArchive process function you had a bug. It was calling the ->finish() method which was resetting the archive options. Causing the estimation of the size to change. The ZipSizePredictionFailed event was possibly raised/omitted under certain conditions.

@Firtzberg
Copy link
Contributor Author

@jszobody please review so we can move forward

@Firtzberg
Copy link
Contributor Author

@jszobody Please review.
I am happy to do further changes if necessary.
This is a braking change so you should release a new major version when all the PRs get merged.

@jszobody
Copy link
Member

I'm out of town this whole week at a business conference with limited availability. I don't expect to be able to really focuse on this until next week.

Have you built zips with this new code and tested them on both Windows and Mac? I seem to remember having issues with the zeroHeader option, I think Mac Finder couldn't open the zip? I don't remember for sure.

@Firtzberg
Copy link
Contributor Author

Thank you for the update.
I am sure when you find the time you'll understand that I split the PRs for a reason.
I understand your concerns, but this PR does not force the user to use the zeroHeader flag. But in case they decide to do so the filesize estimation will be correct. Earlier on it was wrong.
Earlier the user wasn't able to use the zeroHeader flag at all, since the file got cut off.

@jszobody jszobody merged commit 6daf434 into stechstudio:master Nov 19, 2019
@jszobody
Copy link
Member

@Firtzberg I meant to ask you: do the improvements in this PR to filesize prediction properly handle Zip64 now?

I previously just avoided zip size prediction if the size of an individual file or the total zip size was greater than 32 bit. I didn't understand enough about zip headers in Zip64 to know how that is calculated.

return $this->options->getMethod() == Method::STORE() && $this->getFilesize() < 0xFFFFFFFF;

&& $this->queue->sum->getFilesize() < 0xFFFFFFFF;

If you believe filesize prediction now works with Zip64, I'll remove these checks.

@Firtzberg
Copy link
Contributor Author

@Firtzberg I meant to ask you: do the improvements in this PR to filesize prediction properly handle Zip64 now?

I don't know. I wondered about this myself. I had a look at the ZIP format specification. I think the underlying ZipStream-PHP package doesn't handle this properly itself. If it does it does it in a very hard to read manner.
Your task is to predict the size generated by the ZipStream-PHP package, not according the specification (if there is any difference).

Anyway I thought how to test this? I think the only way to do it is to generate a large zip file.

@jszobody
Copy link
Member

Aight see #13

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

Successfully merging this pull request may close these issues.

None yet

2 participants