Allow PHPStan to match with @throws phpdoc

[rvanlaak]

PHPStan 0.10 was released last Weekend 🎉 , and will throw the following exception starting from level 2:

PHPDoc tag @throws with type GuzzleHttp\Exception\GuzzleException is not subtype of Throwable

As all exceptions eventually extend \RuntimeException, and as we in userland want to rely on the interface, letting GuzzleException interface extend \Throwable would be preferable.

also see #1971

[Nyholm]

I do really much like this idea and I think this is the correct way forward. However, Guzzle still supports PHP5.5 so we cannot merge this PR until we drop support for PHP5

[rvanlaak]

@Nyholm found out that as well, are there plans for Guzzle 7 on php: ^7.1 ?

Any idea on how to cover Guzzle under PHPStan ^0.10? I can imagine that others will start having this problem as well.

@throws \GuzzleHttp\Exception\GuzzleException|\RuntimeException

Using the above as phpdoc won't solve the phpstan error either, so the only thing I can think of at the moment is to let PHPStan exclude it 👎 :

# phpstan.neon
parameters:
    ignoreErrors:
        - '#PHPDoc tag @throws with type GuzzleHttp\\Exception\\GuzzleException is not subtype of Throwable#'

What about a polyfill?

[Majkl578]

Adding this to ignoreErrors is the correct solution for legacy (pre-PHP7) code. Note that throwing around non-throwables/non-exceptions is a violation of phpDoc since ever:
https://docs.phpdoc.org/references/phpdoc/tags/throws.html#description

The type provided with this tag MUST represent an object of the class Exception or any subclass thereof.

[Majkl578]

Note that PHP 5.5 is dead for 2 years already. PHP 5.6 and PHP 7.0 are security-only and will die at the end of this year.
http://php.net/supported-versions.php

Supporting anything below 7.1 is basically non-sense from maintainer's PoV nowadays. You should migrate to fully typehinted PHP 7.1 code. 😊 (Let me know if I can help with this, did quite a lot of similar work on (not only) Doctrine projects.)

[rvanlaak]

The type provided with this tag MUST represent an object of the class Exception or any subclass thereof.

That actually is an interesting one, as all Guzzle's GuzzleException implementations actually do. They at the end have \RuntimeException as supertype. The only thing is that the GuzzleException interface itself doesn't 😉

[Majkl578]

Yes, that's correct, unfortunately using interfaces that don't extend Throwable was never supported and was merely a convention of some projects.
Anyone can implement such interface in non-Exception/non-Error class, making it basically pointless marker interface.

This was also brought up in the PHPStan PR: phpstan/phpstan#1001 (comment)
I personally believe that now, in 2018, it's almost an edge case (and you can simply add one ignoreErrors for legacy code) and PHP 5.x code can safely be ignored as it's not a primary target group.
OTOH it already found a bug elsewhere: https://github.com/doctrine/migrations/pull/739/files#diff-f2788aa62fbd5343d8695bd6cff10804L33

[Majkl578]

Ad legacy PHP, see: #2098

[sagikazarmark]

Supporting anything below 7.1 is basically non-sense from maintainer's PoV nowadays

No, it's non-sense from a consumer point of view. From a maintainer point of view you have to consider the user base (you can't leave them alone), other elements of the roadmap, etc.

You should migrate to fully typehinted PHP 7.1 code.

I fully agree, but for the reasons above we just can't do that yet.

[janedbal]

The user base won't be left alone. They will still have the 6.x version, don't they? You can still maintain that branch if you want...

[Majkl578]

No, it's non-sense from a consumer point of view. From a maintainer point of view you have to consider the user base (you can't leave them alone), other elements of the roadmap, etc.

Not really true. As a maintainer, you have to maintain compatibility with a huge range of PHP versions. This itself makes things much more complex and greatly increases possibilities of bugs in some PHP versions.

As said by @janedbal, releasing 7.0 with PHP 7.1 requirement does not mean leaving anyone alone, you still have 6.x with 5.5 support and bugfixes coming in.

OTOH people stuck on dead PHP versions block whole ecosystem in past.

[sagikazarmark]

I was arguing that statement out of context. As a maintainer the dropping PHP versions might/should not be the first thing to consider when thinking about a roadmap.

[rvanlaak]

If there would be a date when PSR 18 would get released, then that might be Guzzle 7's release goal? At least, based on #2098 (comment) I assume that would be a solid argument to define the future roadmap? As PSR 18 is still in draft, I don't see this happening within now and 2 years though. @Nyholm any plans on the Readiness vote?

This all doesn't mean work can't get started on a develop branch yet. It's most likely that Guzzle wil support PSR 18 asap after it's released, meaning that PHP7 will be the minimum. The remaining now would be to decide whether supporting abandoned versions of PHP is enough reason to formulate a roadmap / date when Guzzle 5.x would become EOL.

[Majkl578]

Sorry, I just don't see any correlation between PSR release and Guzzle 7. Those are two totally different milestones...
Also I absolutely don't understand why some unfinished PSR (with one single-method interface) should block all the work on PHP 7.1 migration?

[mtdowling]

Does Drupal still rely on Guzzle? Are they still locked at PHP 5.5 or have they raised their minimum version requirement to 7+?

[Majkl578]

How is that relevant?
Can Drupal use Guzzle 6? Yes,
Why should Drupal block evolution of whole ecosystem? Isn't that selfish/biased?

[wirwolf]

Psalm send me this error

ERROR: InvalidThrow - src/Infrastructure/Query/Http/ProgramQuery.php:67:16 - Class supplied for @throws GuzzleHttp\Exception\GuzzleException does not implement Throwable
     * @throws GuzzleException

[GrahamCampbell]

Doesn't symfony have a polyfil that will allow this code on PHP 5?

[rvanlaak]

Doesn't symfony have a polyfil that will allow this code on PHP 5?

see symfony/polyfill#101

[GrahamCampbell]

Oh, yeh, they only have Error polyfilled... https://github.com/symfony/polyfill/blob/master/src/Php70/Resources/stubs/Error.php.

[rvanlaak]

@Nyholm fixed this in #2375 so this PR can be closed 👍