phpdocs for exceptions

[nickdnk]

Hello guys

As I'm sure most of you maintainers are aware of, I'm a big fan of phpdocs and use it extensively in my code. I am well aware that annotation changes nothing at runtime, but I find it very helpful in determining if I am missing a return type, not handling an exception, passing a wrong type and so on when I'm coding and reviewing my code.

Now, after you made the change to use traits (I think it was major version 5?), the phpdocs for thrown exceptions are all out of whack because of the shared methods used across (as far as I can tell) most requests.

For me, this results in a lot of this:

} /** @noinspection PhpRedundantCatchClauseInspection */ catch (\Stripe\Error\Base $e) {

    // I have to include above notation to avoid warnings.

}

And if I don't include this comment, PHPStorm gives me a "redundant catch phrase" error, which, if I erroneously should accept this suggestion, would result in a working catch block being removed entirely. Not so great.

So, I tried to "start from the bottom" (of traits) to include all exceptions everywhere they are thrown. This of course results in all requests throwing almost all exceptions - which is simply incorrect; such as retrieving a customer then claims it can throw \Stripe\Error\Card. Not a good solution.

Would it be possible to somehow at the very least have InvalidRequest, Card, and Base added in cases where these errors can be thrown? So that we have to handle (or silence) Base, which I'm under the impression any endpoint can throw, and which would of course also catch any other unexpected exception (RateLimiting, Authentication etc.). If so, how could we do this? I would be happy to put in the work.

[nickdnk]

I'm thinking that you could do something like this, which would also fix return types:

use ApiOperations\Retrieve {
    retrieve as protected static __retrieve;
}

And then for \Stripe\Customer as an example:

/**
 * @param string     $id
 * @param null|array $opts
 *
 * @throws Error\InvalidRequest
 * @throws Error\Permission
 * @throws Error\RateLimit
 * @throws Error\Base
 *  and so on for the object/endpoint in question...
 *
 * @return Customer <- Also missing currently.
 */
public static function retrieve($id, $opts = null) {

   return self::__retrieve($id, $opts);

}

This is a lot of work and maintenance, but would work without actually changing or breaking anything. I am willing to make this change on all resources if you think this could be a solution.

With regards to return types, we could avoid a lot of this:

/** @var Customer $customer */
$customer = Customer::retrieve($customerId);

[remi-stripe]

@nickdnk Thanks a lot for providing so much details, this will definitely help us investigate a good solution.

The problem here is that as soon as you do that, changing the type of exception that can be returned becomes a breaking change. We had that problem with stripe-java and it is quite tricky to solve. We usually recommend that, in all cases, you always catch StripeException since most errors are likely to always be handled the same way.

I'm going to tag as future and CC a few people to think through this.

cc @ob-stripe @brandur-stripe @mickjermsurawong-stripe

[nickdnk]

@remi-stripe

It would be great if we could solve this. It's been bothering me for a while.

With regards to your concern: Since this is annotation only, it can't ever actually break anything? It is going to give off a lot of new warnings for people that don't catch their exceptions, sure, but everything should continue to work in the exact same fashion as before. I don't know if you consider warnings a breaking change, but then of course this could be included in the next major version.

I understand that this is an issue in Java, but those are checked exceptions, not annotations.

And if you upgrade your API version, any exception/error changes are going to be introduced into your code regardless of your library or phpdocs. Similarly: If you then choose not to update your API version, but to update the library to include a new/changed exception, you could forcefully silence the new warning(s), which is not different from what you currently have to do if you use phpdocs.

[Nek-]

Many points may be improved here:

• Using XXException is already self-describing (and is a PHP convention)
• Exceptions should inherit from LogicException and RuntimeException
• Doc about their existence must be added
• Base as a name for an exception (even error) is disturbing

I suggest some changes for the next major and you should already think to deprecate stuff. Hope it helps.

[ob-stripe]

Hey everyone, thanks a lot for the feedback. We're considering releasing a new major version soon, so this is very helpful.

@Nek- I'm considering making the following changes:

• rename the Error namespace to Exception
• add Exception as a suffix to all exception classes
• replace the Base parent class with an ExceptionInterface interface
• have all classes inherit from RuntimeException (I don't think any of the exceptions really qualify as LogicException)

I think this would be more idiomatic and in alignment with most PHP developers' expectations. Do you agree?

Doc about their existence must be added

Could you elaborate a bit more? Are you talking about documentation in the library itself, like PHPDoc annotations initially discussed here?

@nickdnk First, sorry for the late reply! I agree that the use of traits makes the PHPDoc annotations somewhat less useful since they have to be very generic. While your proposed solution would work, I'm against it for the time being: the major motivation for switching to traits was to facilitate maintenance of the library.

That said, we're currently experimenting with autogenerating parts of our client libraries' source code directly from the OpenAPI specification for Stripe's API. This is already live in stripe-java. We won't get to stripe-php immediately, but we will get to it eventually, and at that point all API resources and API request methods will have complete, up-to-date documentation.

For now, I think just adding a @throws Error\Base to all API request methods is probably the best short-term fix. From a static analysis point of view, any API request method can potentially throw any of the exception classes. Would that work for you?

[nickdnk]

@ob-stripe I'm fine with adding a @throws Base everywhere. At least that rids me of a lot of the error suppression notation I currently have to implement.

[Nek-]

@ob-stripe I meant add a part in the README that says what exception will be throw when. (because this is complicated to understand ATM)

👍 changes you planned sound nice

[nickdnk]

@ob-stripe @Nek- If you use RuntimeExceptions, PHPStorm will ignore the exceptions entirely: LogicException and RuntimeException derivatives are ignored by default. Maybe not the best idea if you want to properly implement and use PHPDocs, as it effectively silences the "missing @throws"-warning.

Enabling detection of \RuntimeException causes warnings to pop up in many places because of the way these are implemented in PHP. For instance, mysqli_sql_exception is a RuntimeException.

I would suggest you stick to \Exception as the parent class - as it's currently implemented.