Recognise exceptions as instances of Throwable

[hkdobrev]

With --level 5, I get errors such as this one:

Parameter #1 $throwable of static method Foo::processThrowable() expects Throwable, FooException given.

Of course in PHP 7, every exception is an instance of the special Throwable interface.

It would be nice if PHPStan knows about it out of the box.

[ondrejmirtes]

This normally works, because every concrete exception class passes isSubclassOf('Throwable') in reflection, but I suspect that your case is about interfaces - which can be put in catch too and which of course do not extend Throwable. Please check if I'm right and that your FooException class is an interface.

…

On Wed, 18 Jan 2017 at 19:26, Haralan Dobrev ***@***.***> wrote: With --level 5, I get errors such as this one: Parameter #1 $throwable of static method Foo::processThrowable() expects Throwable, FooException given. Of course in PHP 7, every exception is an instance of the special Throwable interface. It would be nice if PHPStan knows about it out of the box. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#91>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuBLMJxNdTBHXS_8qeME0CojUAZCRks5rTllDgaJpZM4LnOk1> .

[ondrejmirtes]

This is what I suspect your situation is:

https://3v4l.org/EmbS4

This is what you should do :)

https://3v4l.org/LmPgi

[hkdobrev]

Indeed this is the case, the exception I'm getting is an interface.

However, it is not from my code, but from a 3rd-party library - https://github.com/thephpleague/omnipay-common/blob/b1440bd7c/src/Omnipay/Common/Exception/OmnipayException.php

So I could write a new interface extending both theirs and Throwable, and then using that in my code, but should I really have to do that for every 3rd-party exception interface?

[ondrejmirtes]

I'm gonna fix that for the next version - ClassReflection in catch block will always agree that the exception variable is a Throwable. Meanwhile uou can add the message to ignoreErrrors.

…

On Wed, 18 Jan 2017 at 20:43, Haralan Dobrev ***@***.***> wrote: Indeed this is the case, the exception I'm getting is an interface. However, it is not from my code, but from a 3rd-party library - https://github.com/thephpleague/omnipay-common/blob/b1440bd7c/src/Omnipay/Common/Exception/OmnipayException.php So I could write a new interface extending both theirs and Throwable, and then using that in my code, but should I really *have to* do that for every 3rd-party exception interface? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#91 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuNzyAmO1lHkJGzFcQjw4q-mrx-Xbks5rTmtcgaJpZM4LnOk1> .

[VasekPurchart]

I get the motivation to automatically add the Throwable type, but I think, this is a double edged sword, because that also means, that if something is in fact not an Throwable/Exception it may very well be a bug and the worst thing about this is that PHP will not throw an error about this. On the other hand in throws, this is checked by PHP: https://3v4l.org/OBHAW.

This is exactly the kind of bugs I would expect PHPStan to notify my about - the throw case you will discover when you run the code (which is not ideal but at least something), but the catch case can only be discovered by writing test for the catch excecution path you expect to happen. Here is a code sample to better illustrate what I mean https://3v4l.org/RHqI2:

<?php

class Foo
{
    
}

class FooException extends \Exception
{
    
}

try {
    throw new \FooException();
} catch (\FooExceptio $e) {
    throw new \Exception('Never called because class does not exist');
} catch (\Foo $e) {
    throw new \Exception('Never called beacause class does not match exception and never will (because it is not an exception/throwable subtype)');
}

So either the class can:

1. not exist - which is probably already checked by PHPStan (please correct me if am wrong)
2. be an existing class, which is not subtype of exception/throwable - which is checked now and this issue is about this

So I think it is still usable to check it, also level 5 should be very strict as it is, so at first I wanted to propose pushing this to a higher level, but I think it might actually be fine. So I believe this is on par with the other cases, where the type information is missing and can either be supplied in other way, or ignored.

[ondrejmirtes]

@VasekPurchart The class referenced inside the catch parentheses is checked whether is an interface or whether is a class implementing \Throwable: https://github.com/phpstan/phpstan/blob/master/src/Rules/Exceptions/CatchedExceptionExistenceRule.php#L51 So I think this is OK since any interface can be implemented by a class that extends \Exception.

This issue is about what happens inside the catch block with $e next - if I want to assign it to a property or send as an argument to a method, it should pass the implements \Throwable check because it was already caught and we can be sure about it.

To prevent the issue you're talking about, I'd have to check if some class that implements the caught interface actually extends \Exception but I think this issue is so minor it's not worth the implementation and performance cost (of discovering and creating reflections for all 1st and 3rd party classes used in the project).

[hkdobrev]

@ondrejmirtes Thank you for your efforts! I just came here to say that I'm using a method call on the exception where a Throwable is expected, but you've posted first:

class Logger
{
    /**
     * @param Throwable $throwable
     */
    public function logException($throwable) // I don't have a typehint here so it works on 5.6 as well
    {
        // log exception with trace here
    }
}

try {
    // some code here
} catch(OmnipayException $exception) {
    Logger::logException($exception);
}

I hope this context helps you and others reading the issue.

[ondrejmirtes]

But one check is truly missing – whether the type in throw expression is an object that implements \Throwable :) Will add that in the next version.

[dkarlovi]

Awesome to have an issue, come to GH to open it and find it's already there, fully explained and everything. 👍

[thePanz]

Any update on this @ondrejmirtes ?
It would fix some warnigs we're getting from a GuzzleException that we're catching: it is an interface (https://github.com/guzzle/guzzle/blob/master/src/Exception/GuzzleException.php) and a further use of $e->getMessage() is not recognized by PhpStan

[ondrejmirtes]

Right now, you can put this error you’re getting into ignoreErrors.

On Wed, 9 Aug 2017 at 11:57, Ema Panz ***@***.***> wrote: Any update on this @ondrejmirtes <https://github.com/ondrejmirtes> ? It would fix some warnigs we're getting from a GuzzleException that we're catching: it is an interface ( https://github.com/guzzle/guzzle/blob/master/src/Exception/GuzzleException.php) and a further use of $e->getMessage() is not recognized by PhpStan — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuDBITo5Xk8RXFpD6opHw_zUzpLFDks5sWYKFgaJpZM4LnOk1> .

-- Ondřej Mirtes

[ondrejmirtes]

Fixed mainly with intersection types #410 and this 480e9c8.

If you like PHPStan and use it to find bugs in your projects, I'd love if you supported it on Patreon. I have many ideas how to make PHPStan even smarter and faster but doing it over evenings and weekends is not sustainable longterm. I'm trying to make developing open-source a viable livelihood alternative. Even a $1 a month helps, but the more you contribute, the closer I am to my goal to work on it fulltime :)

[thePanz]

Thanks @ondrejmirtes!
And yes, we're planning to support the project :)