Ability to download archive of CSV and all files

Thanks, this is a nice-looking patch. If you can roll a D7 version that'll speed things along a bit, but either way I'm a bit swamped and I'm not sure I'll get to new Webform features any time soon. To make things easier yet, this patch also needs to follow a few Drupal coding standards (I use the D7 standards for both D6 and D7 for consistency): use a space on either side of concatenation dots; put else and elseif statements on the next line; use elseif instead of else if.

Thanks @dharmatech! These patches look great. Unfortunately I'm having a few issues after applying this patch:

- On D7, the .zip file is 0 bytes. Works great in D6 though.
- After downloading the .zip, the included file is named sample_webformcsv (where "sample webform" is the name of the node). So the dot before the extension is missing. Plus, I actually used tabs as the separator, "tsv" should be the extension, not "csv".
- The .zip option only works with the delimited text file option. If you select "Excel", it just downloads the Excel file.

This is a great patch and I'd love to include it in the project. Feel free to adjust the base class and functions if needed to accompany this new functionality.

It appears that this was previously committed to at least the 6.x-3.x branch (haven't checked which version or if it is 7.x also).

I am setting back to CNW though, as the code references $base_path, which is not set anywhere (shouldn't this use something like http://api.drupal.org/api/drupal/includes--file.inc/function/file_direct... in any case?):

$zh->addFile(realpath($base_path . $file), basename($file));

Wow, you're right Owen! Looks like I might have accidentally committed this as part of another issue.

According to Git history, It looks like I did this as part of a security fix (issue #54554), and I only did it to the Drupal 6 version. We should roll back this change ASAP, it wasn't ready for prime-time yet.

I've rolled back the 6.x-3.x branch with this patch. I'm sort of upset with myself for letting this one go through in such a shady manner. It was entirely unintentional, sorry to anyone who may have had troubles caused by this (or are upset by its sudden removal). We'll get this working and included in a release soon.

Regarding this question:

I am setting back to CNW though, as the code references $base_path, which is not set anywhere (shouldn't this use something like http://api.drupal.org/api/drupal/includes--file.inc/function/file_direct... in any case?)

I think we can just remove the $base_path variable entirely. The file is being manipulated in the Drupal temp directory, so using of file_directory_path() is unnecessarily. Similarly, $base_path shouldn't be needed because the temp path is already relative or absolute and doesn't need a prefix.

Actually you do need $base_path because the ZipArchive handler needs the full path to the files.

Hmm, sounds like we need to use realpath() first then, because the tmp file directory may be either relative or absolute when defined by the user.

I experienced a problem when downloading submission results as a delimited text (tab delimiter) report using the 6.x-3.14 version of the webform module. I'm documenting my experience here in case it may be helpful for debugging this feature.

The webform which gave me the issues included a field for uploading a file. Our website received more than 3,000 submission results. Upon attempting to download with the "include files" option checked, I experienced errors in the form of an invalid .zip archive with a filesize of Zero KB. I tried expanding the zip archive anyway, and it produced a file with a filesize of 4KB and an extension of ".zip.cpgz".

On the theory that webform was choking on the large number of files in our submission results, I tried downloading them in batches by using the "All submissions starting from: to (optional) end" option and entering a partial range of submission IDs. This produced fatal PHP errors if the range between the starting and ending sids was too large, but I was able to successfully download everything in 13 separate batches by experimenting with sid ranges until I found ranges that worked. I seemed to be able to download successfully if the filesize of the resulting .zip archive was 100MB or smaller.

but I was able to successfully download everything in 13 separate batches by experimenting with sid ranges until I found ranges that worked.

We need to implement #1327186: Use BatchAPI to Export very large data sets to CSV/Excel to help solve this problem, though I'm surprised just 3,000 submissions caused this problem for you. What's your PHP memory_limit as reported by phpinfo()?

Our PHP memory_limit is 256M. I think the issue probably has more to do with the total size of all of the uploaded files, which adds up to roughly 1 gigabyte.

Yeah, the issue in #1276098: Add Drush command for bulk export took this approach of writing to a tmp file over multiple requests. I'd prefer to avoid a separate download link and instead stream the file, since we'll need to protect that file somehow and automatically delete it after some period of time.

I can't think how we would use Batch API and stream the file, since Batch API ties up the browser (with the JS/reload code), adn we can't produce large files without Batch API due to PHP timeouts. Might be possible with crazy iFrame stuff, but I think the download link approach is probably more elegant. I don't think there is a way to store the file contents in Batch API or cache tables without loading it all into RAM at some point, which is problematic, so I think a filesystem file is the way to go. The file could still live in /tmp and be streamed (using the same technique as private files, but could be without depending on it), which prevents us needing to load the whole thing into RAM, and means we know when it has been successfully downloaded, so we can clean it up.

Hello all. I am looking to update to the latest D6 webform release. However, I wanted to include (if possible) the patch for downloading files uploaded as part of webform submissions. Is it possible for me to use one of the patches in this issue? If so, which one? Thanks...

Hi,

what about Drupal 7 version? I've tried it and it doesn't work. The system downloads an empty file...

I have to install a PHP library?

Thanks

Me too. I am looking for this approach in D7 - anyone has successfully used this patch ?

Thank you

Closing for lack of activity. In all these years, there aren't that many people that want this. With the changes in download options, this would be a bit more complicated now.

We happened to need this functionality so we have implemented a patch against 7.x-4.x, based on #6.

Attached is the patch, if folks are interested.

@Liam Morland why did you move the issue back to Needs work ? Is there something not working with #26 ?

The patch failed to apply. Tests need to pass.

@Liam Morland oh I see, the CI system tried to apply the patch against webform-7.x-3.x whereas the patch was written for 7.x-4.x.

I have added a test for 7.x-4.x -- now it applies cleanly and all unit tests pass :) https://dispatcher.drupalci.org/job/drupal_d7/44711/console

We can start with an easy patch, one that just implements the new method, get_filename(). Please use single-quoted string.

Alright -- here is a simplified patch that just implements the new method get_filename() and uses single-quoted strings.

Thanks. I have committed #32. This will need tests.

Re-rolled following the refactor (46ee271). I guess this just needs tests now.

Here is another patch that includes tests.

And another one with more tests...

Just confirming that patch in #38 works for me on Drupal 7.75 and Webform 4.23, so I hope this gets merged. Looking forward to this in the next release!

Merge request needs to be rebased.

Rebased merge request.

Thanks.

In reviewing the code, I noticed that there is a change in the return value of webform_results_download_rows_process(). This a non-backward-compatible API change. To make this backward-compatible, there needs to be an optional parameter added which tells the function to include the attached files. The comments need to be updated to document this new functionality.

compress should probably be include_files. The description that shows will ZipArchive is not available should append to the one already there.

if ($options['compress'] == 1) can be if ($options['compress']).

Thanks for the review. I think I have addressed everything.

Would this work with s3fs?