Mark ExceptionInterfaces throwable #2

[ostrolucky]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This has been reverted in beta of 4.1 because of lack of support in prophecy, which has been fixed since then (incl. release). Can be merged again.

References:
#26702
#27420
#27419
phpspec/prophecy#412

ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes

[dkarlovi]

Can we verify the same usecase is OK with PHPUnit?

[ostrolucky]

Seems support has been added in sebastianbergmann/phpunit-mock-objects#402

[fabpot]

This has been reverted because it breaks BC as demonstrated by Prophecy and PHPUnit. There could be other packages/projects out there with the same issues.

[nicolas-grekas]

I also think this provides little to no benefit and the BC risk is proven already.

[dkarlovi]

It provides benefits if you're using a static code analysis tool which notes you're trying to catch non-throwable exceptions.

I'd argue that the BC issue highlighted deeper issues in Prophecy and PHPUnit, not that it was Symfony's fault.

[fabpot]

What is a non-throwable exception? Any is throwable, the interface does not add any more "power" to the exception class... or I'm missing something.

[dkarlovi]

If you're using the exception marker interface as intended (to catch any package's exception), you're doing something like:

try {
} catch (ExceptionInterface $exception) {
}

This code is marked invalid by static code analysis as the interface is not throwable, as in the fact it can be thrown at all is circumstantial.

Relying on the concrete implementation to make the interface throwable doesn't rely on the interface but the concrete implementation, making the marker interface pretty much useless.

Real example: https://github.com/dkarlovi/xezilaires/blob/60ca197333b0974d543c2e4a88f94e7d1878d3d7/src/Xezilaires/Metadata/Mapping.php#L61

Requires:
https://github.com/dkarlovi/xezilaires/blob/60ca197333b0974d543c2e4a88f94e7d1878d3d7/psalm.xml.dist#L30-L35

[ondrejmirtes]

I don't think it's exactly a BC break, it just showed there's a bug in Prophecy that didn't allow mocking Throwable interfaces...

[fabpot]

Sorry, but unfortunately I'm too dumb to understand the issue. I will let the other @symfony/deciders team members decide.

[javiereguiluz]

@ostrolucky I'm also having problems trying to understand the necessity of this change. Could you please explain very briefly (or with a small code example) what does this change allow to do that wasn't possible before? Thanks!

[ondrejmirtes]

Basically this: https://phpstan.org/r/d90d2b30f1bd1eeb90255e92927e4fdf

You're putting something that might not be an exception into places like @throws tag or catch block. This prevents user from making an unwanted error because he possibly wanted to write something else.

[ondrejmirtes]

So if you intend some interfaces only to be used by exceptions, you can enforce that by extending the interface from Throwable.

[dkarlovi]

Basically this: https://phpstan.org/r/d90d2b30f1bd1eeb90255e92927e4fdf

As opposed to https://phpstan.org/r/e4e2c496c15be8d8ec2fcc55e7b29051

[dkarlovi]

Same issue with Psalm:

https://getpsalm.org/r/c6195f9342

[stof]

The benefit of this PR is indeed for any tool doing static analysis. So this concerns phpstan, psalm, but also IDEs (PHPStorm does not currently has such inspection in its stable version to highlight a mistake, but autocompletion in catch clauses is restricted to throwable types).

It won't make a huge difference for developers themselves (if we forget the impact on their tooling), as the only change this makes for PHP is forbidding silly mistakes (implementing something called ExceptionInterface on a class not being an exception is certainly a mistake)

[nicolas-grekas]

OK, let's do this. I don't agree with the "error" level: a "warning" would be more appropriate to me unless of course one explicitly enabled a rule to make this an error. But that's not a reason against this PR :)
About the BC break, it's true the issues were bugs actually, highlighting the fact that Throwable is very special since the only way to implement it is by extending Exceptionor Error.

Should we add a line in UPGRADING-4.2/5.0.md?

[fabpot]

Ok, understood now. Thanks for the additional info.

[dkarlovi]

It won't make a huge difference for developers themselves (if we forget the impact on their tooling)

I'd argue the "impact on their tooling" is the crucial point here: since the tooling is becoming as prevalent in a modern PHP toolbox. Looking at the pace and value the tools generate, this trend will only increase and changes like these will be just as important as any other bugfix / feature in the future.

[fabpot]

Thank you @ostrolucky.

[stof]

I'd argue the "impact on their tooling" is the crucial point here:

I totally agree on that. but I wanted to point out the other impact, as I already detailed the tooling impact previously in my comment