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

Support bulk downloading assets as a ZIP archive #6606

Merged
merged 9 commits into from Aug 31, 2022

Conversation

jacksleight
Copy link
Contributor

@jacksleight jacksleight commented Aug 31, 2022

This PR adds bulk asset download support.

I had something similar knocking about from an addon I never finished, so thought I'd PR this as it came up in ideas.

Uses the Zip Stream library, so the archive doesn't have to be saved to disk first.

Closes statamic/ideas#855

@jacksleight jacksleight marked this pull request as draft August 31, 2022 15:56
Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

Neat! But this definitely won't work with S3 or other non-local filesystems.

Also, we just merged #6599 which simplifies the action and removes the need for a javascript bit. You could probably just do the zipping from within the download method of the action. You could return a response that streams the zip file.

@jacksleight
Copy link
Contributor Author

jacksleight commented Aug 31, 2022

But this definitely won't work with S3 or other non-local filesystems.

Ah OK, could this still be added for non-S3 users? Should I add a check for that?

Also, we just merged #6599

Ha, for some reason I thought that had been merged before I started, oh well. I've refactored this and it makes things so much simpler! Only other change I had to make was to the content-disposition regex in Actions.js. Zip Stream returns the filename in a format that includes the encoding type (filename*=UTF-8''assets.zip), so it now allows for that.

@jacksleight jacksleight marked this pull request as ready for review August 31, 2022 17:12
@jasonvarga
Copy link
Member

Ah OK, could this still be added for non-S3 users? Should I add a check for that?

Sorry yeah I meant the approach should be adjusted so it'll work for any driver.

It looks like you can pass in file contents as strings instead of paths to the zip package. So this might work:

$zip = Zip::create('assets.zip');

$items->each(function ($asset) use ($zip) {
     $zip->add($asset->contents(), $asset->basename());
 });

return $zip->response();

Try that?

It's probably better if the assets can be streamed too (using $asset->stream() instead of $asset->contents()) although there's no stream method, and we'd need to add some stuff to our file classes to support that. We could do that later.

@jesseleite
Copy link
Member

Fancy stuff! Just tested on S3 asset container and it works great! 💅

The only negative I can think of is that it's slower over S3 because it has to get the file contents over the api first for each individual asset. I don't see this as a problem for a handful of assets, but I wonder if this will timeout the request if you select too many assets and/or extremely large assets?

@jasonvarga
Copy link
Member

Probably fine for now and we can add streaming support later.

@jasonvarga jasonvarga merged commit 2a764ec into statamic:3.3 Aug 31, 2022
@jesseleite
Copy link
Member

jesseleite commented Aug 31, 2022

That said, I just tested 50 decently sized images and it still worked! Downloaded them from S3 in under 20 seconds.

@jacksleight jacksleight deleted the bulk-download branch September 1, 2022 07:37
@el-schneider
Copy link
Contributor

@jacksleight you're the man!!

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.

Batch download assets
4 participants