Modernize exceptions

[ob-stripe]

This is a large PR that is kind of hard to review -- sorry!

• Rename \Stripe\Error namespace to \Stripe\Exception
• Rename all exception classes to append an Exception suffix
• Add an ExceptionInterface interface implemented by all our exception classes
• Rename the Base / OAuthBase abstract classes to ApiErrorException / OAuthErrorException
  • We're no longer overriding the constructor, instead the abstract classes provide a static factory method to create instances
• Add new exception classes BadMethodCallException, InvalidArgumentException and UnexpectedValueException that wrap the matching SPL exceptions (the purpose of these wrappers is to add the ExceptionInterface interface, so users can catch any and all exceptions raised within stripe-php by catching \Stripe\Exception\ExceptionInterface)
• Rename Api to UnknownApiErrorException
• Add UnknownOAuthErrorException

All the above changes make the exceptions much more idiomatic and less surprising for PHP developers. Defining an ExceptionInterface implemented by all exceptions in the codebase (including standard SPL exceptions by defining wrapper classes) is standard practice for modern PHP codebases.

[ob-stripe]

r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries

Fixes #605.

Alright, I think this is ready to review. Sorry this is so hard to review, I should have done separate commits :/ I would recommend looking at the new classes in lib/Exception/ first, then looking at the changes in ApiRequestor.php to see how request exceptions are instantiated. Most other changes are mostly updates to use the new names.

Also cc @nickdnk and @Nek-, curious if you have any feedback!

[nickdnk]

I think this is a fine solution.

On your last point left unchecked: In my opinion it should definitely not inherit from RuntimeException, especially now when everything can be handled by simply catching the interface. Almost no other PHP libraries (except for AWS') I've worked with use RuntimeException for errors that are expected to happen and that you should recover gracefully from. Doing this would remove all warnings about missing or redundant exception handling in PhpStorm, which renders the entire @throws PhpDoc part of this library useless. I don't believe that was the point of this change.

While this thread is a question about Java, its answers outline the reasoning behind checked/unchecked (runtime) exceptions and when and why they are useful (and when they are not): https://stackoverflow.com/questions/6115896/understanding-checked-vs-unchecked-exceptions-in-java.

Since we're interacting with an API far away and outside our control as users of the library, it makes very much sense to force us to always take into consideration that Stripe may return an error - even in cases we expect it not to. We should always recover gracefully from this. Using RuntimeException for our parent class discourages this behaviour, as developers may simply forget to handle errors which break their program. If you get tired of handling this for Stripe, the ExceptionInterface can simply be added to ignored Exceptions in PHPStorm (I assume most serious PHP developers use this program). RuntimeException is ignored by default, so inheriting from this would disable the try/catch warnings out of the box and make it impossible to enable for Stripe alone: If we remove RuntimeException from the list of ignored exceptions we'll be forced to implement a lot of suppression by annotation or a lot more try/catch blocks. \mysqli_sql_exception for instance inherits from RuntimeException and can be thrown anywhere MySQLi is used, while Stripe is probably in most cases isolated to specific classes or files where error handling can easily be contained and handled properly by whoever implements it. I realise this depends a lot on how one's project is structured, but at least that's the case for me.

That's my take on it anyway. I know checked vs. unchecked is somewhat of a religion, but hey, I believe checked exceptions are a good thing when used properly.

Some other libraries that use \Exception and not \RuntimeException that I use:

https://github.com/facebook/php-graph-sdk
https://github.com/slimphp/Slim
https://github.com/fightbulc/moment.php
https://github.com/cheprasov/php-redis-client
https://github.com/PHPMailer/PHPMailer

[brandur-stripe]

LGTM OB. A cleanup of epic proportions!

I might be missing something due to general PHP ignorance, but one question I had was around the UX of ExceptionInterface. I can see from your examples that you can just use duck typing to access information on it (e.g. $e->getMessage()), but doesn't having no properties of its own make things inconvenient for editors and for people to reason about?

Put another way, having an ExceptionInterface doesn't tell you anything about what you're allowed to do with a caught exception. You'd need to go and look through the code to see what exceptions inherit it, and what common properties they might have between them that you could use in your rescue handler. Should ExceptionInterface define some common Stripe exception properties for usability purposes?

[nickdnk]

LGTM OB. A cleanup of epic proportions!

I might be missing something due to general PHP ignorance, but one question I had was around the UX of ExceptionInterface. I can see from you examples that you can just use duck typing to access information on it (e.g. $e->getMessage()), but doesn't having no properties of its own make things inconvenient for editors and for people to reason about?

Put another way, having an ExceptionInterface doesn't tell you anything about what you're allowed to with a caught exception. You'd need to go and look through the code to see what exceptions inherit it, and what common properties they might have between them that you could use in your rescue handler. Should ExceptionInterface define some common Stripe exception properties for usability purposes?

Perhaps this thread can enlighten us: guzzle/guzzle#2184 - we may end up creating a situation with problems similar to what developers experience with this Guzzle interface.

I'd recommend that we have all exceptions extend from an abstract class (that extends \Exception per my previous reply) with some functionality common to all Stripe exceptions (such as $e->getStripeErrorCode()) and if necessary this can also have abstract methods that all exceptions must implement if we want "interfacy" behaviour. Catching this exception would accomplish the same as catching the interface.

[ob-stripe]

I'd recommend that we have all exceptions extend from an abstract class that has some functionality common to all Stripe exceptions (such as $->getStripeErrorCode()) and if necessary this can also have abstract methods that all exceptions must implement if we want "interfacy" behaviour. Catching this exception would accomplish the same as catching the interface.

That was basically the way it was implemented before, all exceptions derived from the \Stripe\Error\Base abstract class.

I think the interface is preferable because it lets us wrap SPL exceptions, which are now used for some client-side errors (previously we used InvalidRequest or Api but I think that was bad because it meant those exceptions could be thrown on either purely client-side errors or map to an API error, which are two very different things).

Right now the stuff common to all API errors is implemented by the ExceptionTrait trait. I could change that to be an abstract class named e.g. StripeServerException or something.

[nickdnk]

That was basically the way it was implemented before, all exceptions derived from the \Stripe\Error\Base abstract class.

But was that a problem? I don't think this implementation solves any of the problems we had with the Base error specifically (except for its name). It solves other stuff, sure, but when it comes to the issue I raised with PhpDocs, the @throws interface solution is pretty similar to throwing a base class.

[nickdnk]

I think the interface is preferable because it lets us wrap SPL exceptions, which are now used for some client-side errors (previously we used InvalidRequest or Api but I think that was bad because it meant those exceptions could be thrown on either purely client-side errors or map to an API error, which are two very different things).

Right now the stuff common to all API errors is implemented by the ExceptionTrait trait. I could change that to be an abstract class named e.g. StripeServerException or something.

I think having a different inheritance tree on errors that are caused by programming errors is a good approach, such as throwing an InvalidArgumentException when providing something that cannot possibly be correct to the library - before even sending the request. Obviously this cannot have functionality that only applies to HTTP responses from the Stripe API. Edit: It's also fine that those are RuntimeExceptions.

[brandur-stripe]

Right now the stuff common to all API errors is implemented by the ExceptionTrait trait. I could change that to be an abstract class named e.g. StripeServerException or something.

I just wanted to say as well that IMO it'd also be fine to leave the trait, but just duplicate a lot of its properties onto ExceptionInterface. It might seem a little messy, but in practice this stuff doesn't change very often, and it'd solve the usability/editor discovery problem.

[ob-stripe]

I just wanted to say as well that IMO it'd also be fine to leave the trait, but just duplicate a lot of its properties onto ExceptionInterface. It might seem a little messy, but in practice this stuff doesn't change very often, and it'd solve the usability/editor discovery problem.

We can't do that because ExceptionInterface is also used to wrap SPL errors, which don't have these properties (e.g. https://github.com/stripe/stripe-php/pull/709/files#diff-73c622c93f7f919e037e154f6375232aR5). We'd need a different interface or abstract class to group all exceptions that map to API errors. Currently this is done via the ExceptionTrait trait, but I'll change it to an abstract class (and give it a better name to make it clear that it maps to an API error).

[brandur-stripe]

We can't do that because ExceptionInterface is also used to wrap SPL errors, which don't have these properties (e.g. https://github.com/stripe/stripe-php/pull/709/files#diff-73c622c93f7f919e037e154f6375232aR5). We'd need a different interface or abstract class to group all exceptions that map to API errors. Currently this is done via the ExceptionTrait trait, but I'll change it to an abstract class (and give it a better name to make it clear that it maps to an API error).

Ah, I see! Okay, that sounds good too though.

[ob-stripe]

Okay, I made a few changes:

• replaced the ExceptionTrait with the ApiErrorException abstract class
• changed PHPDoc @throws on request methods to use ApiErrorException instead of ExceptionInterface (to avoid the problem that Guzzle users described in the thread Nick linked)
• changed SignatureVerificationException to not derive from ApiErrorException, because it's not an API error

I'm fairly happy with the result except for one thing: we have ApiException that derives from ApiErrorException. ApiException is only used in the case where the API returns an error that the PHP SDK doesn't know about. I think we should either rename it to UnknownApiErrorException, or maybe make ApiErrorException non-abstract and just use that directly. Thoughts?

[nickdnk]

@ob-stripe I think renaming it to UnknownApiErrorException is a good idea. I like having the base class being abstract, and they're named too similarly otherwise, which can be confusing. Perhaps you could add to the documentation of that exception that when it's thrown it may indicate that you're using an outdated version of the library and you should check for updates to properly handle the error.

[ob-stripe]

Cool, that's my preference too. I'll go ahead and make that change. (Also thanks for all your help and feedback here, much appreciated!)

[nickdnk]

@ob-stripe

You're welcome.

I'm also a little confused about the purpose of ExceptionInterface after this latest change. What does it actually accomplish at this point? Is it just a way to do a quick catch-all? I mean this can be accomplished by catching \Exception as well?

[nickdnk]

Just to clarify; #605 is not solved by these changes, as we'll still get the warnings that all the actual exceptions are "not thrown" (as phpdocs only mention the abstract base class). I don't think that can be solved without explicitly adding all possible API errors to all methods - which I'm well aware you understandingly don't want to do.

[ob-stripe]

I'm also a little confused about the purpose of ExceptionInterface after this latest change. What does it actually accomplish at this point? Is it just a way to do a quick catch-all? I mean this can be accomplished by catching \Exception as well?

I'm no PHP expert but I saw this pattern being employed by a lot of PHP codebases (https://www.google.com/search?q=exceptioninterface.php+site%3Agit.luolix.top). My understanding is that it serves as a nice way to know whether an exception was thrown by a library or by your own code, as long as the library makes sure that all exceptions it throws implement its interface (which I believe I've done here).

Just to clairfy; #605 is not solved by these changes, as we'll still get the warnings that all the actual exceptions are "not thrown" (as phpdocs only mention the abstract base class). I don't think that can be solved without explicitly adding all possible API errors to all methods.

Yes, sorry, I should have said that it "partially" solves the issue. Previously I had annotated the methods with ExceptionInterface, which would also cause warnings for the non-API error exceptions. That's no longer the case. Besides, with the exception of CardException, all API request methods can potentially throw any API error exception.

[nickdnk]

I'm also a little confused about the purpose of ExceptionInterface after this latest change. What does it actually accomplish at this point? Is it just a way to do a quick catch-all? I mean this can be accomplished by catching \Exception as well?

I'm no PHP expert but I saw this pattern being employed by a lot of PHP codebases (https://www.google.com/search?q=exceptioninterface.php+site%3Agit.luolix.top). My understanding is that it serves as a nice way to know whether an exception was thrown by a library or by your own code, as long as the library makes sure that all exceptions it throws implement its interface.

That does make sense. Let's stick with that.

Just to clairfy; #605 is not solved by these changes, as we'll still get the warnings that all the actual exceptions are "not thrown" (as phpdocs only mention the abstract base class). I don't think that can be solved without explicitly adding all possible API errors to all methods.

Yes, sorry, I should have said that it "partially" solves the issue. Previously I had annotated the methods with ExceptionInterface, which would also cause warnings for the non-API error exceptions. That's no longer the case. Besides, with the exception of CardException, all API methods can potentially throw any API error exception.

I like only being forced to handle errors that come from the Stripe API, so I think this solution is probably the best middle-ground, with the problem still being that if you catch something like InvalidRequestException, PHPStorm will tell you that it's never thrown, which is wrong and is only because we've deliberately only annotated the abstract base class and ignored all the "missing exception tag" warnings that show up in the library now - those we can't follow because of traits. Edit: I don't know why I basically just repeated myself. I apologize.

[ob-stripe]

Okay, I think I've made all the changes we wanted. I've also updated the first post with the current state of the PR.

ptal @brandur-stripe

[brandur-stripe]

Thanks for the broad changes Olivier. LGTM!

[ob-stripe]

Thanks Brandur and Nick!