stechstudio / laravel-zipstream

Easily create Zip files on-the-fly and provide a streaming download
MIT License
413 stars 58 forks source link

Cache file size #87

Closed toto4ds closed 1 year ago

toto4ds commented 1 year ago

Hi All, To recognize PREDICT_SIZE using S3 or HTTP takes a long time for many files. It would be correct to pass a known file size in this case. File.php has a setFilesize() method, but it's not entirely clear how to use it. Any ideas how to implement this?

jszobody commented 1 year ago

Sure thing. You just need to create the S3File model yourself, so you can set the filesize.

// Instead of this...
$zip->add('s3://bucket/key', 'zipfilename.txt');

// ... do this:
$zip->add(
    S3File::make('s3://bucket/key', 'zipfilename.txt')->setFilesize(12345)
);
toto4ds commented 1 year ago

Hi jszobody, Thanks for the quick response. Now it works great, download starts instantly. It would be great to add this information to the README.md

But that doesn't solve the problem for HttpFile model - $this->getHeaders() https://github.com/stechstudio/laravel-zipstream/blob/1960242407a2d0f84370f916286db2d89122c53b/src/Models/HttpFile.php#L50-L54 I think it's a good idea to add if ($this->filesize) { return parent::canPredictZipDataSize(); } here

toto4ds commented 1 year ago

Hi jszobody, I will try to convey my point of view again in other words.

The model File has a canPredictZipDataSize() method. And only in the HttpFile model it is also present. The HttpFile model sets the header variable to make one HTTP request to the file rather than two via the calculateFilesize() and canPredictZipDataSize() methods to get the file size - HEADER_CONTENT_LENGTH We know the size of each file and in order not to do extra work, we transfer it using ->setFilesize(12345). Method calculateFilesize() is no longer called, but canPredictZipDataSize() also continues to run because the $headers variable is empty. What we wanted to get rid of continues to bother us. It is necessary that the $this->getHeaders() are not called if the file size was set in advance here https://github.com/stechstudio/laravel-zipstream/blob/master/src/Models/HttpFile.php#L52 What do you think about this ?

jszobody commented 1 year ago

Yeah, makes sense. It should skip headers if filesize is present. Do you want to submit a PR for this?

Also, I'm curious what kind of HTTP file sources you're using, with known filesizes up front. That's interesting.

toto4ds commented 1 year ago

Yeah, I'll do the PR later.

Initially, for a very long time, I use mod_zip (https://github.com/evanmiller/mod_zip) for nginx, and he wants to know the size and hash in advance. I just decided to see what PHP has achieved while working with zip during this time.

toto4ds commented 1 year ago

On a quick inspection, I ran into a problem.

This line returns the size https://github.com/stechstudio/laravel-zipstream/blob/master/src/Models/HttpFile.php#L26 But there may be no header Content-Length in the response and this will lead to an error. We can process this and answer in the false - if (!array_key_exists(self::HEADER_CONTENT_LENGTH, $headers)) { return false; } But after that we have to return union types - int|false which have been supported since php 8.0. And we have the minimum version php: ^7.1 Using zero to check is not correct too as it is the correct file size. Then it turns out that the correct way is set to check the file size is if (is_numeric($this->filesize))

What do you think?