basvandorst / StravaPHP

Strava API REST client with OAuth authentication
MIT License
184 stars 67 forks source link

Exceptions with Guzzle #49

Closed iDontWantAUsername closed 4 years ago

iDontWantAUsername commented 6 years ago

Hi,

i know this perhaps isn't the right place but is anyone having issues handling exceptions since the update to use Guzzle?

This is my example code:

use Strava\API\OAuth;
use Strava\API\Client;
use Strava\API\Exception;
#use Strava\API\Exception\ClientException;
use Strava\API\Service\REST;
#use GuzzleHttp\Exception\RequestException;
#use GuzzleHttp\Exception\ClientException;

try{
    $token = $_SESSION['AuthToken'] ; 
    $adapter = new \GuzzleHttp\Client(['base_uri' => 'https://www.strava.com/api/v3/']);
    $service = new REST($token, $adapter);

    $client = new Client($service);
    $athlete = $client->getAthlete();
    } catch (Exception $e){
    echo "in here";
            throw new Exception('Unauthorised');
            }

var_dump($athlete);

This returns:

string(209) "Client error: GET https://www.strava.com/api/v3/athlete resulted in a 401 Unauthorized response: {"message":"Authorization Error","errors":[{"resource":"Athlete","field":"access_token","code":"invalid"}]} "

I have tried loads of combinations of ClientException and Exception in different namespaces but do not get an exception thrown, as far as i can tell i am copying the readme examples. Does anyone have an idea what is have done wrong? This worked fine and caught the 401 errors with the previous version.

Thanks! Richard

bwardy commented 6 years ago

I also have the same issue.

bwardy commented 6 years ago

@iDontWantAUsername I found that I could get the getAthlete() function working. Change the line: $service = new REST($token, $adapter); to: $service = new REST($token->getToken(), $adapter);

I'm still trying to figure out how to get the getAthleteActivities() function working now.

iDontWantAUsername commented 6 years ago

Thanks @bwardy , i should have been a bit clearer, if i put in a valid Token the request does work as expected.

My issue is that if i am not authorised i should get an exception from Guzzle that i can handle and return the correct message.

I am not getting the handled exceptions i used to get prior to Guzzle. I should be able to use the Exceptions rather than having to run a string comparison on the getAthlete response.

bwardy commented 6 years ago

@iDontWantAUsername well... I'm out. I'm VERY new to this. :)

maltehuebner commented 5 years ago

Looks like this issue is releated to #45. I fixed the problem other there, unfortunately the fix was not accepted.

vredeling commented 5 years ago

The Guzzle update was quite comprehensive. Also, from 15 jan 2018 onwards there is a change in the authentication process. https://developers.strava.com/docs/oauth-updates/ So I'll try and contact @basvandorst to run over the new issues. To get these hickups ironed out.

djjavo commented 4 years ago

I might be a bit late to the party here, but also experiencing similar issues to @iDontWantAUsername.

Would changing the catch block in https://github.com/basvandorst/StravaPHP/blob/master/src/Strava/API/Service/REST.php be beneficial? https://github.com/basvandorst/StravaPHP/blob/5b2b2a9246434273e5a7b8e9d0333765e0fcafbf/src/Strava/API/Service/REST.php#L71-L80 As opposed to returning $e->getMessage(), return $e->getResponse() (or both)? Would enable you to do look at useful data such as the response code?

vredeling commented 4 years ago

@djjavo Yes, that would be useful. But requires a bit of a refactor of all the getResponse() calls. Could you supply a pull request on the develop branch of my fork? https://github.com/vredeling/StravaPHP/pulls

vredeling commented 4 years ago

@djjavo I'm closing this issue. If you want the $e->getMessage() issue addressed, please open a separate issue or PR.

notflip commented 4 years ago

Ah this is related to my issue: #65 I'm willing to spend time to make a PR but I'm not sure as to what has to happen exactly. Would it be an idea to rethrow instead of returning $e->getMessage()?

vredeling commented 4 years ago

@notflip Yes, that would be the idea. I'll review the PR if you supply one.

I'm a bit concerned we're breaking current implementations of the this class again by making a change like this. But it makes a lot of sense to re-throw and have implementations catch the error instead of the message. Or we define a global config variable which sets the response output to text by default. You could change that param to get a throwable instead of the error text.

djjavo commented 3 years ago

@vredeling I've suggested a non-breaking change (https://github.com/basvandorst/StravaPHP/pull/67) for users of StravaPHP that require extra information in the response.

vredeling commented 3 years ago

@notflip since this PR #67 has been merged to the latest dev version, it is possible to check the response codes. Just instantiate the REST client like so: $service = new REST($token->getToken(), $adapter, 1); That will give you access to the response headers, body and status codes.