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)...

CommentFileSizeAuthor
#17 docs_604902.patch593 bytesdrewish
#14 604902.patch817 bytesrschwab
#1 604902-1.batch_can_fail.patch786 bytesdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs review
FileSize
786 bytes

Status: Needs review » Needs work

The last submitted patch failed testing.

dww’s picture

Yeah, 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?

chx’s picture

If 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.

JacobSingh’s picture

Hmm... 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

yched’s picture

while (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 :-(.

dww’s picture

Assigned: dww » Unassigned
Issue tags: +DX (Developer Experience), +DrupalWTF

@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.

yched’s picture

Then how do you get the $success parameter set to FALSE in your batch 'finished' callback?

$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'.

dww’s picture

So, 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... ;)

yched’s picture

dww: that's right.

sun.core’s picture

Priority: Critical » Normal
Status: Needs work » Postponed (maintainer needs more info)

What critical bug is left here?

dww’s picture

Title: Impossible to mark that a batch has failed » Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions
Status: Postponed (maintainer needs more info) » Needs work
Issue tags: +Needs documentation

The 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

jrust’s picture

Also note that you can mark success as FALSE in your code by throwing an exception:
throw new Exception('Hit an unrecoverable error.');

rschwab’s picture

Status: Needs work » Needs review
FileSize
817 bytes

Would 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.

dww’s picture

Status: Needs review » Needs work

@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...

jhodgdon’s picture

Component: base system » documentation

Sounds 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.

drewish’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
593 bytes

Moved this into the docs for the Batch API.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

See comment #15. Also moving back to 7.x so the test bot can test the patch.

yched’s picture

@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.

drewish’s picture

Status: Needs work » Needs review

@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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Oh 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.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

Needs to go to d8 and then d7

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -DX (Developer Experience), -Needs documentation, -DrupalWTF, -Needs backport to D7

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