PdoDriver: fix pdo connection in exception mode (target >=4.1.0)

[jkuchar]

• new feature
• BC break? no (as original behaviour should be considered as broken)

see #292

[jkuchar]

I do not like this handling of error cases. Is is done twice, any ideas?

[dg]

Is your intention to make it work regardless of ATTR_ERRMODE, ie. with ERRMODE_WARNING and ERRMODE_EXCEPTION together?

Maybe you can use finally:

try {
    $res = $this->connection->query($sql);
    ...

} catch (\PDOException $e) {
} finally {
	throw $this->createException(isset($e) ?  $e->errorInfo : $this->connection->errorInfo(), $sql);
}

[jkuchar]

Finally will be executed also on successful case, I have extracted this in main body of the function.

[jkuchar]

I do not like that I need to pass $previous all around. Any ideas how to do better?

[dg]

I think the PDOException does not contain any additional information, so there is no need to pass on the previous one.

[jkuchar]

Hmm, as it is implementation detail, no code ever should depend on that. Gonna remove that

[jkuchar]

There is one more problem to fix, there is also WARNING mode in which dibi probably should mute warnings and handle them by iteself consistently = throwing exception.

I would like to write few notes into comments into this function because it handles three things simulatanously and it is not obvious what is does for the first look.

[jkuchar]

One more thing - I will need to help a little with tests. What approach to go? Run all tests also in warning mode and exception mode? Or create just unit tests for this one function. I'm not shure what happend whan connection dies before ->rowCount() is called.

[dg]

Run all tests in warning mode and exception mode seems better to me.

I'm not shure what happend whan connection dies before ->rowCount() is called.

It's hard to test, I would probably let it go.

[jkuchar]

Run all tests in warning mode and exception mode seems better to me.

How to do that? Tester does not support running more @dataProviders. I will probably go for unit test specially for PdoDriver::query() and running that with more configurations.

[jkuchar]

Running all tests with different configuration can be made by passing evironemnt variables to tester runner and passing that to individual tests. These evironment variables can be then listened in boostrap.php and there it can be decided with way to setup PDO.

[jkuchar]

@dg It is more complicated then it looked at first time. There are still unhandled getAttribute, startTransaction, ... calls which leaks exceptions. What about wrapping PDO connection into evelope which normalizes errors. Current approach would make PdoDriver quite complex. Alternative approach is to wrap pdo calls in "javascript way".

self::try(function() {return $this->connection->getAttribute();}, function () {success}, function() {failure});

However this leads into weakly typed code as PHP does not support generics.

[dg]

Another option is to have two drivers, one for exception mode and one for warnings mode. And forbid silent mode (it is nonsense mode).

And another one option: to switch PDO to warning mode and back before and after each operation.

[jkuchar]

As PDO can provide consistent API when configured, I will try to go for switching error modes. Hopefuly that will hot have any side effects. There should be none - as long as there will be no nesting or callbacks from inside of the driver.

BTW problem with silent mode is that it is default. Current implementation of PdoDriver relays on silent mode.

API that changes by configuration is hell🔥.

[PavelJurasek]

Hi, can I somehow help with this to get it merged? Would be pretty helpful in a project of mine.

[jkuchar]

@PavelJurasek You can go through the comments and try to solve problems that are left.

As I see this now from bigger perspective. Could you please try to implement three PdoDrivers, for each mode? They each have different API and it is huge mess to try to implement it in one class. If you wish I can go through the current proposal if you wish.

[PavelJurasek]

@jkuchar hi, there is a rough draft of split 67b32f9

I really don't like "Base" keyword in abstract driver name, but dunno how to name it 🤷‍♂️

On the bright side - default mode is exception mode since php8.0 🙂

[jkuchar]

@jiripudil Could you please go through these changes? I'm quite busy this week. :)

[PavelJurasek]

Bump

[PavelJurasek]

Rebased and fixed some of the tests in https://github.com/PavelJurasek/dibi/tree/fix-pdo-connection-in-exception-mode

@dg @jkuchar is there something else I could do to push this forward?