Add error as well as exception handling to Mysqli drivers #2704
Conversation
|
Overall, patch looks good, but the try-catch error handler restoring should be tested. A test would do something like: $handler = function () { self::fail('Never expected this to be called');}
set_error_handler($handler);
try {
$doTheThingThatFails();
self::fail('An exception was supposed to be raised');
} catch (MysqliException $e) {
// assert on the exception message
}
self::assertSame($handler, restore_error_handler(), 'handler stack was restored'); |
|
@Ocramius I have added the missing test. |
|
A bunch of failures popped up - see https://travis-ci.org/doctrine/dbal/jobs/225133976#L276-L303 |
|
@Ocramius Right, sorry about that. It seems to be OK now. |
| if ( ! $this->_conn->real_connect($params['host'], $username, $password, $dbname, $port, $socket, $flags)) { | ||
| try { | ||
| if ( ! $this->_conn->real_connect($params['host'], $username, $password, $dbname, $port, $socket, $flags)) { | ||
| throw new MysqliException($this->_conn->connect_error, @$this->_conn->sqlstate ?: 'HY000', $this->_conn->connect_errno); |
There was a problem hiding this comment.
You could use $this->_conn->sqlstate ?? 'HY000' instead.
There was a problem hiding this comment.
I could, but it would break compatibility with PHP 5.
There was a problem hiding this comment.
You're targeting master branch, which already have dropped the support for PHP 5.
There was a problem hiding this comment.
Replacing ?: with ?? in this case doesn't make sense. Why do we suddenly need to ignore missing property sqlstate inside $this->_conn? I for one would like code generate error and not silently continue in such a case.
| @@ -28,11 +28,31 @@ protected function setUp() | |||
| ->getMockForAbstractClass(); | |||
| } | |||
|
|
|||
| protected function tearDown() | |||
| { | |||
| restore_error_handler(); | |||
There was a problem hiding this comment.
This will cause restore_error_handler() to be called for every test in this class - that is a mistake, since any error handler registered by phpunit may be replaced. Only the actual test overriding the error handler should restore the error handler (by calling restore_error_handler() at the end)
There was a problem hiding this comment.
As you can see, at the end of the test there is a call to set_error_handler($default_handler), which should essentially restore the error handler to the pre-test state. However, without restore_error_handler in tearDown, it causes 7 other tests to fail from some reason.
I have changed the tearDown to restore the error handler only for this test.
| @@ -28,11 +28,33 @@ protected function setUp() | |||
| ->getMockForAbstractClass(); | |||
| } | |||
|
|
|||
| protected function tearDown() | |||
| { | |||
| if ($this->getName() == "testRestoresErrorHandlerOnException") { | |||
There was a problem hiding this comment.
The issue here is that you have to do restore_error_handler() twice in the testRestoresErrorHandlerOnException test :-)
…restoring twice in the test instead
|
@develancer I manually merged this into |
Ocramius
changed the title
This patch restores the error handler if mysqli_real_connect throws an exception.