[DBAL-2954] Removed error suppression in IBM DB2 Statement

[morozov]

Fixes #2954.

[Majkl578]

Looking into the source, it looks like there is more different errors, not just conversion related but also binding-related errors and execution failure errors. Aren't these the reason for @? The next check for false suggests it's possibly the reason, to avoid warnings and to properly escalate them as exceptions.

[Majkl578]

One solution could be: keep @, keep check for $retval === false and add another check using error_get_last() (cleared with error_clear_last() before db2_execute()) that escalates to exception as well. Is it worth it?

[morozov]

Aren't these the reason for @? The next check for false suggests it's possibly the reason, to avoid warnings and to properly escalate them as exceptions.

Most probably that's the case.

One solution could be: keep @, keep check for $retval === false and add another check using error_get_last() (cleared with error_clear_last() before db2_execute()) that escalates to exception as well. Is it worth it?

IMO, not all errors should be promoted to exceptions. If an error didn't cause the method to return false, it shouldn't be converted (e.g. it could be a deprecation notice). Ideally, we should suppress those errors which we convert to exceptions but let the others trigger.

In a general case, a single call to db_execute() may produce multiple non-fatal and zero or one fatal error at a time. Not sure how handling those could be properly implemented.

[morozov]

One solution could be: keep @ […]

With this solution, an internal fatal error will cause silent script termination. Happy debugging.

[Majkl578]

Happy debugging.

Right, because catchable fatal isn't actually catchable by try...catch... :/ (Was renamed to recoverable fatal in 7.1+: https://3v4l.org/Wea6g).
But it is catchable by custom error handler: https://3v4l.org/vJ0Ea without causing script termination.

[morozov]

@Majkl578 so you're suggesting something like this?

set_error_handler(function ($errno, $errstr, $errfile, $errline, $errcontext) {
    throw new DB2Exception($errstr);
});

try {
    $retval = db2_execute($this->_stmt, $params);
} finally {
    restore_error_handler();
}

It is better than error suppression but it may introduce false positives in the cases which did trigger errors but didn't cause the false result. Are we okay with that?

I didn't explore the extension source code but something makes me think that all possible notices are not run-time but logical (revealing bugs in the userland code). Having them also displayed as native PHP notices doesn't look like a big deal to me.

[Ocramius]

/cc @asgrim can you maybe read through this? You got much more DB2 suffering experience than me

[asgrim]

I think this is less a discussion about DB2 itself, and really it's "how should errors from the driver be propagated"... PHP 7 now having \Throwable that all errors implement suggests a shift towards preferring exceptions anyway. That is also my personal preference, but it is really up to the Gods of DBAL to decide.

Though, playing devil's advocate, thinking back to when we first started implementing the DB2-based project, I remember the @ operator being a bit of a pain trying to get things working. I simply removed it whilst initially figuring things out and then re-pulled the deps with Composer when done.

That said, especially if DBAL still supports PHP 5.x, I think @Majkl578's solution of setting an error handler would work universally and nicely (convert everything to exception: if it's a notice that the consumer doesn't care about, that can be handled that in the consumer code).

[Ocramius]

@asgrim this is for PHP 7.1+ only though. Unless security issues pop up in 2.5.x, nothing will be touched for PHP 5 support ;-)

[asgrim]

In that case, would it be so bad to remove the @ operator here, as (almost) everything would be catch-able anyway, right?

[Majkl578]

In that case, would it be so bad to remove the @ operator here, as (almost) everything would be catch-able anyway, right?

Not quite, it can still throw a fatal that is not catchable by try...catch, that's why I came up with set_error_handler workaround:

$stmt = $conn->executeQuery('SELECT * FROM users WHERE user_name = ?', [new stdClass()]);

It is better than error suppression but it may introduce false positives in the cases which did trigger errors but didn't cause the false result. Are we okay with that?

In this case, we'd have to store errors in set_error_handler and throw them conditionally. I'm not a fan of either of these solutions, but I guess it's about what's less evil in the end.
Assuming that it can throw multiple notices/warnings/errors, something like this would be needed:

<?php

$errors = [];
set_error_handler(function ($errno, $errstr, $errfile, $errline, $errcontext) use (&$errors) {
    $errors[] = $errstr;
});

try {
    $retval = db2_execute($this->_stmt, $params);

    if ($retval === false && count($errors) !== 0) {
        throw DB2Exception::fromMultiple($errors);
    }
} finally {
    restore_error_handler();
}

Don't like this solution either. :/

[morozov]

Don't like this solution either. :/

And it doesn't solve the original problem. The notice in the example will still be suppressed since it doesn't cause db2_exec() to return false.

A complete solution would contain the else block which would re-trigger all collected errors without throwing the exception. And it would have to convert all collected E_* types to E_USER_* types. That's… kinda… insane.

[Ocramius]

@Netmosfera job for you?

[Wes1262]

@morozov

A complete solution would contain the else block which would re-trigger all collected errors without throwing the exception. And it would have to convert all collected E_* types to E_USER_* types. That's… kinda… insane.

if you are not throwing the exception, you are collecting the errors for... what?

EDIT: nvm, I understand now.

[Wes1262]

function retrigger_error(string $message, int $type){
    $conversions = [
        E_ERROR      => E_USER_ERROR,
        E_WARNING    => E_USER_WARNING,
        E_NOTICE     => E_USER_NOTICE,
        E_DEPRECATED => E_USER_DEPRECATED,
    ];
    
    foreach($conversions as $from => $to){
        if($type & $from){
            $type = $type & ~$from;
            $type = $type | $to;
        }
    }
    
    trigger_error($message, $type);
}

EDIT: I think that will work for most cases except when there is an E_STRICT in the original error, which cannot be triggered manually. In other words if the error we get is: E_NOTICE | E_STRICT and we convert it to E_USER_NOTICE | E_STRICT, trigger_error is going to fail "Warning: Invalid error type specified". In that case we could get rid of E_STRICT and add it to the $message perhaps.

EDIT 2: since it's not actually a mask, the code would be this:

function retrigger_error(string $message, int $type){
    switch($type){
        case E_ERROR:      $type = E_USER_ERROR;      break;
        case E_WARNING:    $type = E_USER_WARNING;    break;
        case E_NOTICE:     $type = E_USER_NOTICE;     break;
        case E_DEPRECATED: $type = E_USER_DEPRECATED; break;
    }
    trigger_error($message, $type);
}

[morozov]

@Majkl578 @Netmosfera with the approach of a custom error handler, we will tremendously increase the code complexity in order to overcome the bad design of the ibm_db2 extension regarding handling errors the userland code.

Are you sure you want to proceed this route?

I still believe just remove the suppression is fine. The worst thing which can happen is in some (which exactly?) cases, the same error may be reported as error and exception. Unless it's a runtime error, I believe, it's perfectly fine.

[Wes1262]

Personally I don't, I would just remove the error suppression, let the notices be, and just check for the return value.

[Majkl578]

There is one more issue with just removing the suppression: BC. Because if the function suddenly starts throwing notices/warnings, strict/debug tools will immediately catch them and convert into exception. And since original code depended on DB2Exception but now receives i.e. ErrorException, that could break code.
So at least, it should be in try...catch + convert it into DB2Exception + set previous exception to the caught one?

[Wes1262]

why are you converting notices to ErrorException?

[Wes1262]

For correctness' sake and in case you decide to go with that route, I've just been informed that E_* are not meant to be triggered combined with each other. So E_NOTICE | E_STRICT can never happen.

[Majkl578]

why are you converting notices to ErrorException?

Because even notice means bug. See for example https://github.com/nette/tracy#visualization-of-errors-and-exceptions.
If not, then @ is exactly what handles it, typical example is @fopen(...) and then checking for false - it's even encouraged in PHP documentation.

[Wes1262]

programming bugs though

[morozov]

Because if the function suddenly starts throwing notices/warnings, strict/debug tools will immediately catch them and convert into exception.

This is fine because they will indicate bugs in the userland code. Otherwise, we need an example of an error which doesn't produce false and is not caused by a bug in the userland code.

And since original code depended on DB2Exception but now receives i.e. ErrorException, that could break code.

It will receive ErrorException from the other error handler, not from DBAL.

So at least, it should be in try...catch + convert it into DB2Exception + set previous exception to the caught one?

You mean we'll be catching Throwable from the method which is not supposed to trigger any exceptions just to support custom error handlers? IMO, the code which uses such handlers should be ready to get ErrorException from anywhere.

[Wes1262]

I'm lost again :-P

[morozov]

So, is there a strong disagreement with the current implementation? I still believe that a simple solution which sometimes will report the same error twice is better than a complex one or the one which will suppress (essentially, lose) some errors.

[morozov]

@Ocramius do you still have any concerns/reservations?

[Ocramius]

No, I'm fine with letting the brokenness of the DB2 adapter exposed. Silencing was a much worse solution indeed.

[Ocramius]

Note: this only targets 2.7, as some users may still be affected due to custom error handlers.