AbstractProvider - unexpected, undocumented exceptions

[lukaszmakuch]

Hello! 👋

First of all, thank you for this project. It has saved me tons of time! 👍

I'm opening this issue to discuss the exceptions thrown by the AbstractProvider class.

When working with a provider, like the GenericProvider, I'd expect to catch some IdentityProviderExceptions. Its nicely documented:

* Exception thrown if the provider response contains errors.

However, much to my surprise, some methods such as getAccessToken or getResourceOwner may throw an UnexpectedValueException when there's something wrong with the response received from the Identity Provider. Sadly, this is not documented.

To be honest, I expected to catch nothing but IdentityProviderExceptions and my code broke because there was no catch block for UnexpectedValueExceptions.

What are your thoughts on this matter? 🤔 Should the fact that an UnexpectedValueException may be thrown be documented? Or maybe we can risk some breaking changes and modify the code to throw IdentityProviderExceptions instead of UnexpectedValueExceptions?

Cheers!

[cloudcogsio]

You can probably adjust your code to catch the different exceptions?

try {
    $accessToken = $provider->getAccessToken($grant, array $options = [])
} catch (IdentityProviderException $e) {
    // handle IdentityProviderException exceptions here
} catch (UnexpectedValueException $e) {
    // handle UnexpectedValueException  exceptions here
}

OR

try {
    $accessToken = $provider->getAccessToken($grant, array $options = [])
} catch (IdentityProviderException | UnexpectedValueException $e) {
    // handle IdentityProviderException and UnexpectedValueException exceptions here
} 

See: https://www.php.net/manual/en/language.exceptions.php#language.exceptions.catch

[lukaszmakuch]

Thank you for your reply. You’re right and it’s pretty straightforward to catch all the exceptions once you know they may be thrown. What I’d like to see improved is the clarity of the documentation (or the documented code). I believe it’d be great if developers could write all the necessary catch blocks based solely on the documentation, without the need of debugging their code, analyzing logs etc.

…

On 3. Aug 2021, at 09:13, cloudcogsio ***@***.***> wrote:  You can probably adjust your code to catch the different exceptions? try { $accessToken = $provider->getAccessToken($grant, array $options = []) } catch (IdentityProviderException $e) { // handle IdentityProviderException exceptions here } catch (UnexpectedValueException $e) { // handle UnexpectedValueException exceptions here } OR try { $accessToken = $provider->getAccessToken($grant, array $options = []) } catch (IdentityProviderException | UnexpectedValueException $e) { // handle IdentityProviderException and UnexpectedValueException exceptions here } See: https://www.php.net/manual/en/language.exceptions.php#language.exceptions.catch — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[ramsey]

Thanks for the suggestion, @lukaszmakuch. We'll keep this open until we've added appropriate @throws tags to all the docblocks. I hope that will help.