Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#12 | webform_validate_range-1668826-12.patch | 1.18 KB | quicksketch |
#9 | 1668826-9-validate_range_options.patch | 1.51 KB | fenstrat |
#1 | webform-fix_the_download_range_errors-1668826.patch | 4.57 KB | darrell_ulm |
#3 | webform-fix_range_download-1668826.patch | 3.43 KB | darrell_ulm |
Comments
Comment #1
darrell_ulm CreditAttribution: darrell_ulm commented(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.
Comment #2
darrell_ulm CreditAttribution: darrell_ulm commentedShould have set this to 'needs review' because there is a patch at http://drupal.org/node/1668826#comment-6188220
Comment #3
darrell_ulm CreditAttribution: darrell_ulm commentedAh, 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.
Comment #4
quicksketchThis 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.
Comment #5
quicksketchIndeed 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.
Comment #6
quicksketchComment #7
darrell_ulm CreditAttribution: darrell_ulm commentedAgreed with:
and if recalled, think that is what the patch did, and related also agree with:
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
Comment #8
quicksketchYeah, 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.
Comment #9
fenstrat@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.Comment #10
ianthomas_ukIt 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:Instead the range should be passed into
webform_results_export
then intowebform_get_submissions
which can then detect the empty range and return an empty result set. I've not looked atwebform_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.
Comment #11
quicksketchI 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.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.Comment #12
quicksketchHere 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.
Comment #13
quicksketchCommitted #12. Please let me know if there are any problems encountered with this approach.