while (empty($current_set['operations']) && ($current_set['success'] = TRUE) && _batch_next_set()) {
Thanks, includes/batch.inc :(
Discovered while working on #538660: Move update manager upgrade process into new authorize.php file (and make it actually work)...
Comment | File | Size | Author |
---|---|---|---|
#17 | docs_604902.patch | 593 bytes | drewish |
#14 | 604902.patch | 817 bytes | rschwab |
#1 | 604902-1.batch_can_fail.patch | 786 bytes | dww |
Comments
Comment #1
dwwComment #3
dwwYeah, ugh. If you do that, then all batches all the time are marked as failure. :( WTF? Is this just an API documentation #fail and I don't understand how to mark a batch failed, or is this just totally fubar?
Comment #4
chx CreditAttribution: chx commentedIf
batch_next_set
returns FALSE then the loop ends. If$current_set['success']
was set to FALSE (is that possible?) meanwhile then there is your failure.Edit: I am wrong. The operation results are in
$old_set = $current_set;
at this point and later on only total and progress_message seems to be reused from this ... Not sure how to proceed will reverse engineer more.Comment #5
JacobSingh CreditAttribution: JacobSingh commentedHmm... Yeah, some type of batch bailout routine is needed, but I'm not sure this is it.
How about batch_error_callback in the batch definition which will catch any exceptions or errors and let the implementer decide how to deal with it?
the success var is used in reporting, sometimes the solution is not do exit the batch, that should only happen when an error occurs (or is thrown).
-J
Comment #6
yched CreditAttribution: yched commentedwhile (empty($current_set['operations']) && ($current_set['success'] = TRUE) && _batch_next_set()) {
The = is intended to be a =, not a ==. We loop through batch sets until we find one with actual operations to execute, marking the sets as 'successfully processed' along the way.
Admittedly, not the most legible code. It's been some time now, but I think I remember I battled quite a bit on that part, I'm not really looking forward into refactoring it :-(.
Comment #7
dww@yched: Then how do you get the $success parameter set to FALSE in your batch 'finished' callback? It seems that is *always* set to TRUE, no matter what you do in your batch operations.
I guess we're just going to have to invent our own definition of success vs. failure, record that in the $results array ourselves, and ignore the argument passed in. It's just a) something of a pain, and b) a major DX WTF that $success is always TRUE.
Comment #8
yched CreditAttribution: yched commented$batch_set['success'] is initialized to FALSE in batch_process(). When something gets wrong during the batch process (PHP fatal error, uncaught exception...), the code that sets it to TRUE is not run.
The error page adds a link to /batch?op=finish. On that page, the finished callback is executed and receives the success value, which is still FALSE, and can act accordingly (which I see update.module does).
Granted, that's merely a way to fail 'nicely' (mostly inspired from what update.php did, if I recall correctly), rather than a full-fledged, generic error handling for batched processes, which seemed a bit daunting back then.
There are many uses cases and definition of 'error', so I left that to client code to do their error managements using 'results'.
Comment #9
dwwSo, in the finished callback, $success only means $no_fatal_php_errors, right? Any other definition of failure is up to the batch operations and the finished callback to handle via its own approach using $results?
At the very least, this needs vastly improved PHP docs... ;)
Comment #10
yched CreditAttribution: yched commenteddww: that's right.
Comment #11
sun.core CreditAttribution: sun.core commentedWhat critical bug is left here?
Comment #12
dwwThe BatchAPI documentation is the remaining bug, I guess. Although the code is hard to read, it's actually doing what it was designed to do. The $success parameter to your callback is just confusingly named. Since renaming that is probably impossible at this stage in D7's lifecycle, the best we can hope for here is to fix the docs to at least explain:
a) What the $success parameter actually means
b) The recommended way for batches to propagate non-PHP-fatal-error errors
Comment #13
jrust CreditAttribution: jrust commentedAlso note that you can mark success as FALSE in your code by throwing an exception:
throw new Exception('Hit an unrecoverable error.');
Comment #14
rschwab CreditAttribution: rschwab commentedWould a good place for this be as an inline comment above this line in
_batch_finished()
:$batch_set['finished']($batch_set['success'], $batch_set['results'], $operations, format_interval($batch_set['elapsed'] / 1000));
Here is a first attempt at a patch for this.
Comment #15
dww@rschwab: Thanks for taking a stab at this. However, for the docs to be useful and visible, they need to be in the PHPDoc outside of the functions so it's visible on api.d.o. Inline code comments help a bit, but very few people trying to get batch API working are necessarily going to read the source to a private function like _batch_finished() to understand how the batch API works...
Comment #16
jhodgdonSounds like this is a pure documentation issue, so I'm moving it to the documentation component, if that's OK.
dww: where on api.drupal.org do you think this needs to be explained? I'm not all that familiar with the batch API functions/topics.
Comment #17
drewish CreditAttribution: drewish commentedMoved this into the docs for the Batch API.
Comment #18
jhodgdonSee comment #15. Also moving back to 7.x so the test bot can test the patch.
Comment #19
yched CreditAttribution: yched commented@jhodgdon, @dww :
Agreed that this information should be in a PHPdoc somewhere. Problem is, the is documentation for a sample callback, not a hook, and so there is no actual 'fake' hook implementation anywhere in any *.api.php file whose PHPdoc could receive that.
Comment #20
drewish CreditAttribution: drewish commented@jhodgdon, the new spot I moved the docs to would appear on: http://api.drupal.org/api/drupal/includes--form.inc/group/batch/7 which is the closest thing we have to proper docs for the batch api.
Comment #21
jhodgdonOh duh, it is part of a docblock. :)
OK I think this is a fine patch then. Let's get it in to 8.x and 7.x.
Comment #22
jhodgdonNeeds to go to d8 and then d7
Comment #23
catchTagging for backport.
Comment #24
webchickCommitted to 8.x and 7.x. Thanks!