No Error Visibility

[bmeynell]

We are interested in using this library but all API calls swallow and hide Exceptions which prevents us from knowing what error occurred.

Example: https://github.com/SiftScience/sift-php/blob/master/lib/SiftClient.php#L105-L114:

public function track($event, $properties, $opts = array()) {
    // ...
        try {
            $request = new SiftRequest(
                $path, SiftRequest::POST, $opts['timeout'], $opts['version'], array(
                    'body' => $properties,
                    'params' => $params
                ));
            return $request->send();
        } catch (Exception $e) {
            return null;
        }
}

In other words, can the try/catch within each API method be removed? Example:

public function track($event, $properties, $opts = array()) {
    $this->mergeArguments($opts, array(
        'return_score' => false,
        'return_action' => false,
        'return_workflow_status' => false,
        'abuse_types' => array(),
        'path' => NULL,
        'timeout' => $this->timeout,
        'version' => $this->version
    ));
    $this->validateArgument($event, 'event', 'string');
    $this->validateArgument($properties, 'properties', 'array');

    $path = $opts['path'];
    if (!$path) {
        $path = self::restApiUrl($opts['version']);
    }

    $properties['$api_key'] = $this->api_key;
    $properties['$type'] = $event;

    $params = array();
    if ($opts['return_score']) $params['return_score'] = 'true';
    if ($opts['return_action']) $params['return_action'] = 'true';
    if ($opts['return_workflow_status']) $params['return_workflow_status'] = 'true';
    if ($opts['abuse_types']) $params['abuse_types'] = implode(',', $opts['abuse_types']);

    $request = new SiftRequest(
        $path, SiftRequest::POST, $opts['timeout'], $opts['version'], array(
            'body' => $properties,
            'params' => $params
        ));

        return $request->send();
}

If so, then library adopters can be in charge of handling exceptions as per their unique requirements:

// User code
try {
    $client->track($event, $properties);
} catch (\Exception $e) {
    // Adopter handles here ...
}

Unfortunately, we must know when and why exceptions occur so cannot use this library for that simple reason.

[thomasschiavone]

Thanks for flagging for us. I'll get on the team's radar and I follow up as I know more.

[tiagobutzke]

I would suggest for allowing to inject (optionally) a Psr\Log\LoggerInterface into the client. Then, log the exceptions.

[ehrmann]

#62 will expose errors in SiftResponse. I agree that we should do more to expose those exceptions, even letting them bubble up, but looking through the code paths, I'm trying to see where they could happen under normal error conditions (network failures, invalid request data, bad json response). curl errors are reported through error codes, the json parsers return null, and SiftResponse marks errors with isOk(), so you shouldn't be seeing errors.