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.
For usability and speed, it seems like would be helpful if the result set is small, to have an immediate download instead of going through the batch process. It should be an option, because there may be cases where you always want it to go through the batch process.
Comment | File | Size | Author |
---|---|---|---|
#18 | views_data_export-batch_only_on_large_export-1804624-18.patch | 3.23 KB | kerasai |
Comments
Comment #1
nadavoid CreditAttribution: nadavoid commentedIn addition to adding an option to the settings form, it seems like the relevant logic should live either in
views_data_export_plugin_display_export->is_batched()
or
I'm encountering a bit of a chicken/egg scenario as I try to implement this. In the execute() function, it branches based on whether batching should occur, and what stage of batching the process is currently in. Before that branching occurs, I need to check to see how many total results there are. That requires another
execute()
to be called. I'm not sure how to do that without getting into recursion that I can't get out of.Comment #2
nadavoid CreditAttribution: nadavoid commentedI think that for this to work, I need to execute a one-off query in
is_batched()
and cache it. That query would need to come from the views configuration. The problem that I'm seeing with this approach is that$this->view->query
is usually not populated whenis_batched()
is called. From what I can tell, the only way I know to get the values used in the query is to load and execute the display, which takes us back to the recursion issue mentioned earlier.Is there another way to execute the query of a view, without actually
execute()
ing the view?Comment #3
nadavoid CreditAttribution: nadavoid commentedGetting closer...
I can populate the query property, but this interferes with the regular batch export mechanism.
I tried placing that (also omitting the 2nd
if
statement) inis_batched()
and inexecute()
, but the export doesn't happen. I see an "Export is starting up" then "Exporting 0% complete" and it stays there. So I think that's progress. I probably need to look inexecute_initial()
andexecute_normal()
next.Comment #4
dbehrman CreditAttribution: dbehrman commentedCreated a patch to only use batch export when the total number of results to export are larger than the number in the batch.
Comment #5
dbehrman CreditAttribution: dbehrman commentedComment #6
nadavoid CreditAttribution: nadavoid commentedWith the patch, it works like a charm. When the batch size is smaller than the total records, the batch process is invoked; otherwise I get an immediate download. Nice!
The only problem I see is that I get a notice when the batch is invoked.
Notice: Trying to get property of non-object in views_plugin_query_default->query() (line 1296 of/Users/david/Stuff/Web/Sites/d7/sites/all/modules/views/plugins/views_plugin_query_default.inc).
That's
->addTag('views_' . $this->view->name);
I'm going to try to get my debugger set up and see what's actually invoking that, but my first hunch would be that it's in
if (!$this->view->query) {
Comment #7
nadavoid CreditAttribution: nadavoid commentedI got my debugger working, and discovered that the notice happens from the new line 201:
$count_query = $this->view->query->query()->countQuery();
. At that point in views_plugin_query_default.inc,$this->view
is null. Not sure why though.Comment #8
dbehrman CreditAttribution: dbehrman commentedThere are a few conditions where there is already a built query, so this will rebuild it to the appropriate query when checking for row count. I think the check for that was originally to prevent recursion, so instead I just added $this->building as a flag to skip the rebuild process when it's already in the process of rebuilding. This might also fix the issue with the notice in comment #6.
Comment #9
nadavoid CreditAttribution: nadavoid commentedThe patch in @dbehrman's #8 works great too. The warnings I was seeing are gone, but they are replaced by a new warning. To avoid this new warning, change line 11 of the patch: replace
!$this->building
with!isset($this->building)
.I think that to complete this, it still needs to be an option presented in the UI "Batch only if result set is larger than selected segment size", or at least update the description of Segment Size. Otherwise, people may wonder why, even though they selected batched exports, why batching may not be happening.
Comment #10
dbehrman CreditAttribution: dbehrman commentedAnother small update to that patch, taking some advice from @nadavoid to prevent some notices. Also there was still an issue in some fringe cases where the query would not build properly. So there is a new condition that should prevent that (in all of my tests anyway).
Comment #12
BassistJimmyJam CreditAttribution: BassistJimmyJam commentedLooks like there are still some notices being thrown which is causing test failures.
Comment #13
dbehrman CreditAttribution: dbehrman commentedFixed that notice.
Comment #14
BassistJimmyJam CreditAttribution: BassistJimmyJam commentedProperties should be defined using the appropriate visibility keyword rather than
var
. I realize that other properties in this same class usevar
, perhaps as a carryover from Drupal 6 which is supposed to be PHP 4 compatible, but this does not conform to Drupal 7 coding standards (see "Visibility" under http://drupal.org/node/608152).Comment #15
dbehrman CreditAttribution: dbehrman commentedChanged var to protected
Comment #16
dbehrman CreditAttribution: dbehrman commentedI found a case where the count query would have no conditions and would count the entire table and continue to batch. This patch resolves that issue.
Commerce module did have an issue with this patch in retrieving the table name, but a patch already existed to fix that: http://drupal.org/files/1879260.robust_query_altering.patch
Comment #17
weri CreditAttribution: weri at Previon Plus AG commentedI've tested and reviewed the patch with version 7.x-3.0-beta9 and it works like expected. It would be great to have this patch committed.
Comment #18
kerasai CreditAttribution: kerasai at Hook 42 commentedAfter all these years, this patch still almost worked! I was noticing a couple issues:
What I tracked it down to was that the logic to determine if the batching was happening didn't work for that last step of the batch. Attached is a patch that checks for the batch handling earlier so the
::is_batched
method works for this case as well.Patch written against 7.x-3.x-dev, applies cleanly and tested on 7.x-3.2.