Catch Throwable in Connection::transactional() #2390
Conversation
fprochazka
force-pushed
the
php7-throwable
branch
from
e30c77d
to
db06fce
Compare
| } catch (\RuntimeException $expected) { | ||
| $this->assertEquals(0, $this->_conn->getTransactionNestingLevel()); | ||
| } | ||
| } | ||
|
|
||
| public function testTransactionalWithThrowable() | ||
| { | ||
| if (version_compare(PHP_VERSION, '7.0', '<')) { |
There was a problem hiding this comment.
I'm not sure about the convention for skipping tests in older php versions. If this is not good I can change it.
There was a problem hiding this comment.
You can also use @requires php 7 in an annotation on this method
There was a problem hiding this comment.
@Ocramius thanks, updated
There was a problem hiding this comment.
@fprochazka did you push? I still see the if block
There was a problem hiding this comment.
Yeah I did, and it failed hard #2390 (diff)
fprochazka
force-pushed
the
php7-throwable
branch
from
db06fce
to
884d3b0
Compare
| @@ -773,7 +774,7 @@ public function prepare($statement) | |||
| { | |||
| try { | |||
| $stmt = new Statement($statement, $this); | |||
| } catch (\Exception $ex) { | |||
| } catch (Exception $ex) { | |||
There was a problem hiding this comment.
I changed this because it was incosistent in this file (didn't look at the other files tho).
fprochazka
force-pushed
the
php7-throwable
branch
from
884d3b0
to
db06fce
Compare
|
This is completly PHP7 related, isn't it? We cannot add this before |
|
@deeky666 this is backward compliant too |
|
Reference implementation: https://github.com/zendframework/zend-stratigility/pull/49/files |
|
@fprochazka thanks! Responsibilities go out to @Ocramius :P |
|
I'm ready, should this go wrong |
|
Thank you :) |
I was considering adding it to other places, that use "catch-all", but none of them can invoke user-provided code, so it makes little sense adding the catch there.