When this option in download:

All submissions starting from: and optionally to: (Use submission IDs for the range. Last downloaded end SID: none.)

is used and start and end values are used that are technically valid but have no results, an error will appear.

For instance if the SID's only go to 20000 but the user enters anything higher than 20000 as the start value this error will occur. It can also occur in other instance such as entering a short range, say 3 to 4 where there are actually no SIDs.

Patch forthcoming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

darrell_ulm’s picture

Status: Needs review » Active
FileSize
4.57 KB

(update, this is not the working patch, the one below is at:
http://drupal.org/node/1668826#comment-6191590)

----

Here is a patch against the current version 6.x-3.18 to solve this issue.

Reply back if you have any questions.

darrell_ulm’s picture

Status: Active » Needs review

Should have set this to 'needs review' because there is a patch at http://drupal.org/node/1668826#comment-6188220

darrell_ulm’s picture

Ah, the last patch did not deal with the other range options, like when no range is selected, it would not download the file.

This patch is useful when the range option is not all. This is important when there is a timeout caused by too many records in the download and the range should have returned nothing rather than timing out and giving an error when trying to download everything.

Also if the range given returns 0 elements, that is what should happen for the range if SIDs, right now when too many records time out the download for a large number of records, if the user enters something outside of the range, even 3 to 4 or 100000 to 100001 etc, before it would download the whole table, which would timeout for large submissions. If no results are returned for a range selection, it seems that is what the result should be, and this patch address that.

quicksketch’s picture

Status: Active » Needs work

This patch doesn't seem like it was rolled against the latest code. It reverts a bunch of unrelated code. Needs a reroll for the latest versions.

quicksketch’s picture

If no results are returned for a range selection, it seems that is what the result should be, and this patch address that.

Indeed if the download range is invalid, what ends up happening is just *all* the submissions are included. Better than just returning an empty download file, Webform should throw a validation error and reload the page.

quicksketch’s picture

Title: Download range option times out or errors when range has no submissions. » Download range option returns all results when range is not valid
darrell_ulm’s picture

Agreed with:

Better than just returning an empty download file, Webform should throw a validation error and reload the page.

and if recalled, think that is what the patch did, and related also agree with:

his patch doesn't seem like it was rolled against the latest code.

So it sounds like it is still worthwhile to re-roll it, to current 6.x-3.x-dev ?, need to look at the abundant tags when I pull it again.

Thanks

quicksketch’s picture

Yeah, we'll need to apply this to all branches (6.x-3.x, 7.x-3.x, and 7.x-4.x) code base is still consistent across them so a single patch usually works across the board.

fenstrat’s picture

Version: 6.x-3.x-dev » 7.x-4.x-dev
Status: Needs work » Needs review
FileSize
1.51 KB

@quicksketch Is this more what you had in mind?

My main concern is the use of webform_get_submissions() in the validation - could be slow if there's a huge range. This will also need slight changes to db_query() to apply to 6.x. I've also left out the space in else{ to match the rest of webform_results_download_range_validate() - I can fix that in a follow up.

ianthomas_uk’s picture

Status: Needs review » Active

It seems very strange to be attempting to pre-run the query in the validation function to pre-empt an empty result set. You've now got two copies of the query that you need to make sure always return the same number of results. Much better to leave the validation functions to validate the user's query, and let the query return an empty result set. If you need to improve the empty result set handling (e.g. to display a message instead of download an empty file) then that's a separate issue.

To me the problem is that webform_results_download_form_submit tries to convert a range to a set of sids:

if ($options['range_type'] != 'all') {
 $options['sids'] = webform_download_sids($form_state['values']['node']->nid, $form_state['values']['range']);
}

Instead the range should be passed into webform_results_export then into webform_get_submissions which can then detect the empty range and return an empty result set. I've not looked at webform_download_sids, but it may be possible to refactor into a single query rather than doing the join in PHP.

I'm setting the status to active as IMO the current patch is a change in the opposite direction to that required (it moves the range code higher up the stack, rather than lower) so a totally new patch is required.

quicksketch’s picture

If you need to improve the empty result set handling (e.g. to display a message instead of download an empty file) then that's a separate issue.

I like the idea of avoiding the same calculation in two places, however I think it's perfectly reasonable that we do a (cheap) preparatory query to determine if we're going to download anything at all first. We've already got a function that does this, webform_download_sids() that we're calling in the submit handler. We could either A) move this to the validate handler or B) short-circuit the submit handler and reload the form. Using the validate handler is the official way to handle such a problem, so I think incorporating it in the validate handler and then passing the list of SIDs to the submit handler would be the most logical approach.

My main concern is the use of webform_get_submissions() in the validation - could be slow if there's a huge range.

Yep, I think that's a very valid concern. webform_download_sids() should get us the list without actually loading the submissions though, it should be fast enough to not cause a problem even if there are thousands of submissions.

quicksketch’s picture

Status: Active » Needs review
FileSize
1.18 KB

Here we are. We're already using webform_download_sids() in the submit handler. This just moves the call to the validate handler, checks that a result is returned, and the passes the list to the submit handler.

quicksketch’s picture

Status: Needs review » Fixed

Committed #12. Please let me know if there are any problems encountered with this approach.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.