Introduce an ExceptionHandler class in the database API, deprecate Connection::handleQueryException

Comment moved to MR.

All code changes look good to me.
The added deprecation has testing and a CR.
The patch fixes the problem from the IS.
For me it is RTBC.

@mondrake: Great work.

Added comment on MR - need to set this back to needs review to get an answer...

Back to needs work for the 2 unresolved threads.

Back to RTBC.

    catch (\Exception $e) {
      $this->exceptionHandler($e)->handleStatementException($query, $options);
  [..]
  public function exceptionHandler(\Exception $exception) {
    $class = $this->getDriverClass('ExceptionHandler');
    return new $class($exception);

class ExceptionHandler {
  [..]
  public function __construct(\Exception $exception) {
  [..]
  public function handleStatementException(string $sql, array $options = []): void {

I don't like this "new $class()".
Why can't we use the capabilities of OOP how it is meant to be used?

  catch (\Exception $e) {
    $this->exceptionHandler->handle($e, $query, $options);

?>

class ExceptionHandler {
  [..]
  public function __construct() {
  [..]
  public function handleStatementException(\Exception $e, string $sql, array $options = []): void {

Perhaps there could also be an interface for DatabaseExceptionHandler.

-----------------

  public function prepareStatement(string $query, array $options): StatementInterface {
    [..]
    catch (\Exception $e) {
      $this->exceptionHandler($e)->handleStatementException($query, $options);
    }
  }

This is meant to return StatementInterface, but here you potentially return void|null.

Why instantiating an exceptionHandler object every time to inject it, when probably 99% of the times is not used? Like this it's only instantiated when an exception has to be handled.

I think the advantage will be quite small. The object will be very light-weight, without any properties. And we would only have one instance per request (one per db connection, or even one shared among db connections). The main benefit would be to avoid loading the class file with autoload.

We pay for it by using what I would call a Drupal anti-pattern of "new $class()", which prevents static code analysis.
I don't even see any class_exists() and then instanceof check.

But I see now that the ->getDriverClass() is already a common pattern in Connection.
This makes it hard for me to defend my point.
So don't let me stop you then.

I did a review in gitlab with some points that need addressing. See ^^

(sorry for the mix of issue comments vs MR comments)

@mondrake
Quoting myself in #18:

But I see now that the ->getDriverClass() is already a common pattern in Connection.
This makes it hard for me to defend my point.
So don't let me stop you then.

I see now that the ->getDriverClass() pattern is only used for the mutable builder classes like Select, Insert, etc.
Here is a grep (or rather, ag):

1061:    $class = $this->getDriverClass('Select');
1082:    $class = $this->getDriverClass('Insert');
1100:    $class = $this->getDriverClass('Merge');
1118:    $class = $this->getDriverClass('Upsert');
1139:    $class = $this->getDriverClass('Update');
1160:    $class = $this->getDriverClass('Delete');
1178:    $class = $this->getDriverClass('Truncate');
1192:      $class = $this->getDriverClass('Schema');
1210:    $class = $this->getDriverClass('Condition');
1367:    $class = $this->getDriverClass('Transaction');

I think it somewhat makes sense to use this pattern for these classes, because we need a new instance every call.
The exception handler, on the other hand, only needs a single instance, which can be easily injected in the constructor. Or a mechanism with a setter and a lazy default initialization.

Overall, I think instead of a pluggable exception handler, it would be better to agree on strict contracts for every method, and stick to them.

E.g. ->prepareStatement() should either return a statement object, or throw an exception.
We should agree on the exception type, and document it with a @throws doc.

We should move away from conditional / parameterized contracts.
If we want an alternative implementation that returns FALSE or NULL instead of throwing, we should make it a separate method.

#21 I agree, but it's difficult while we have #3189680: [PP-1] Deprecate the 'throw_exception' option in the Database API I think... we can deprecate that and in D10 be strict.

Actually, this option is currently only used for ->query() / ->handleQueryException().
Which is somewhat ok, because ->query() does not have a native return type hint, and already advertises that it can return NULL if 'throw_exception' is false.
The try/catch in the ->query() method contains a call to $this->prepareStatement(), but ->prepareStatement() itself does not have any behavior that would be conditioned upon the 'throw_exception' parameter.

In fact I would argue that ->prepareStatement() is currently broken if no statementWrapperClass was provided.
This could probably be verified with a test.

Conclusion:
There is no BC reason to consider this parameter anywhere within ->prepareStatement().

My proposal:

Fix ->prepareStatement():
Make sure that it will always return a value consistent with its native return type hint.
Agree on an exception type that ->prepareStatement() is allowed/expected to throw.
Throw an exception in ->prepareStatement(), if $this->wrapperClass is empty. This is not a BC break, because this case is already broken.

For #3177660: Remove public properties from StatementInterface implementations:
Use try/catch in calling code, e.g., the ->execute() methods of the wrapper classes.
Make Connection->handleQueryException public, and call it from the wrapper classes, OR inject an exception handler into the wrapper classes.

The only unresolved thread has been addressed.
Back to RTBC.

Discussed with @catch - both of us have looked at the code and didn't haven't anymore to add.

Committed 5478a95 and pushed to 9.2.x. Thanks!