BatchAPI::execute has inconsistent return values

[jlesueur]

Client version

v3.0.0

Expected behaviour

When execute is called, it should always return an array of responses (a single output type).

$batch = $api->start_batch();
$batch->send();
$responses = $batch->execute();
if (is_array($responses)) {
    echo "succeeded";
} else {
    echo "failed";
}

should output succeeded in all cases

Actual behaviour

outputs failed if the batch was too large, or authentication failed, or any failure that's not specific to one of the emails in the batch.

The reason for requesting a change is that the current behavior requires boilerplate whenever calling execute():

if (!is_array($response)) {
   //do we actually want to behave differently for this case? If so, should it be an exception?
} else {
   //we can iterate over the responses to see what succeded, and what failed
}

Simplest "fix" would be to change this line: https://github.com/sendwithus/sendwithus_php/blob/master/lib/API.php#L782 to return the single error as an array. That way, a consumer can always depend on getting back an array.

[demoore]

Hey @jlesueur, after thinking on this I'm not sure I agree. The boiler plate is useful in that you can catch errors that aren't related to the content of the batch request and deal with them appropriately. What are you doing to run into this case?

[jlesueur]

We sent a batch larger than 100 emails. And I agree, the proposed fix, isn't necessarily best. Not sure what your standard/opinion is on exceptions, but this seems like the proper place to throw an exception. When executing a batch, the standard return is a set of responses for each item in the batch. If you can't return that, it's an exceptional case. With an exception, it just becomes more explicit:

try {
   $response = $batch->execute();
   //iterate over responses, or whatever is appropriate for a successful batch
} catch (BatchFailedException $e) {
    //do whatever is right for a wholesale failure. 
    //Maybe $e will indicate a connection failure, which would 
    //recommend retrying at some later date, or $e will indicate 
    //a bad format for the request, which would be an important
    //bug on the client's side.
}

instead of an if (!is_array($response)).

[demoore]

Yeah, I can see that being a better pattern; however, the client currently does something similar with non-batch API calls.

There's a higher level discussion that needs to be had about how a user interacts with failed API calls for all methods. What I'd rather see is all methods throw an exception for any request that doesn't respond with a 200. What do you think @jlesueur ?

[jlesueur]

For single api calls, an exception (with specific types used for different kinds of errors) for any kind of failure is a nice touch.

Batch calls need some special care, since there are two levels of success/failure:

• the actual batch call itself
• the individual items in the batch

So an exception when the batch call itself fails is very appropriate. An exception if any item in the batch fails might make sense, but would have to include some way of discovering which ones succeeded and which failed. I think I would kind of lean away from an exception if any item in the batch failed, and instead return an object that included the total number attempted, and the number that succeeded, and then the array of responses for each item.