DBAL 3.0 : improvements to the Driver\Connection interface

[BenMorel]

I can see on the develop branch that the PDOConnection class now wraps PDO instead of inheriting from it, which is a very nice improvement, as the Driver\Connection class can now evolve away from the PDO interface! 👍

Building on this, I would like to suggest a few changes to this interface:

Stop returning false on error

Pretty much all PDO methods (query(), beginTransaction(), ...) can return false on error. This is actually only true, as far as I know, when using the PDO::ERRMODE_SILENT or PDO::ERRMODE_WARNING modes.

Because Doctrine forces PDO::ERRMODE_EXCEPTION, it makes no sense IMO to return false on error in any method; we'd rather enforce always throwing exceptions when something goes wrong, which would make the interface consistent with the methods that already guarantee to throw on error, such as prepare() and query().

So I would make the following methods return void and throw an exception on failure:

• beginTransaction()
• commit()
• rollBack()

As it stands, these methods in the PDOConnection can never return false, they always return true or throw an exception, so the bool return type makes no sense in this driver.

Furthermore, I can see that many implementations actually don't respect the current contract, and don't return any value in these methods (DBAL\Connection, MasterSlaveConnection, DB2Connection, ...), so this change makes even more sense.

Remove the errorCode() and errorInfo() methods

If we're always going to throw exceptions on error, then the error code and error info should be accessible from the exception. Therefore, I think it's redundant to offer this information in the Connection interface.

Strongly type wherever possible

• public function quote(string $input, int $type = ParameterType::STRING) : ?string
  As far as I can see, the input is always a string not sure about this, it looks like we're accepting other types here. Is this really needed? Can't we just use quote(string $input) : string?
  ParameterType constants are always integers. actually, I forgot that this accepts DBAL string types as well. Is this really the responsibility of the Driver, though? Shouldn't the driver only deal with native types, and the main Connection class maybe handle the higher level, DBAL types (and stop implementing Driver\Connection)?
  The return type of PDO::quote() is always a string, unless the driver does not support quoting, in which case it returns false. I would suggest we switch to returning string|null here, or just string and throw an exception if there is no driver support.
• lastInsertId(string $name = null) : string
  I can see no reason not to use strong types here either.
• beginTransaction|commit|rollBack() : void
  If you agree with my first point.

If you agree with these points, I can submit a PR. Looking forward to your feedback!

[BenMorel]

I actually went ahead and submitted a first PR, as #3335, for the easy stuff.
Fiddling with the code raised a few questions about the remaining points:

Connection::quote()

quote() currently accepts more than a string. Is this really needed? Shouldn't we just quote strings and nothing else? Current implementations convert everything else to a string with a simple type-casting, so I can't see any value for this feature at the moment.

PDO::quote() does have a $type parameter, but the documentation clearly states that the first parameter is a string. So I'm not sure what the $type parameter is for: "Provides a data type hint for drivers that have alternate quoting styles." Does anyone have a use case for non-string quoting? If not, what about simplifying the interface to just quote(string) : string?

Driver exceptions

Exceptions that can be thrown by drivers are currently poorly documented: the only methods that declare throwing an exception are query() and exec(), and they declare throwing a generic DBALException, and not a specific DriverException as I would expect: DriverException has getErrorCode() and getSQLState() methods, DBALException does not.

If by any chance you want to go ahead with my suggestion above: removing the Connection::errorCode() and errorInfo() methods, it would be nice to get all Connection methods to be allowed to throw only DriverExceptions.

This way, the consumer of the Driver\Connection always has access to the full error information from the driver whenever an error occurs. What do you think?

Side question: why are there two DriverException classes? Can't we just keep one, to remove confusion?

[morozov]

Stop returning false on error

There's work in progress in this regard (#3154). I haven't touched it in a while but one of the issues I encountered there is that for PDO drivers, besides being able to wrap the native PDO exceptions, we should be able to throw our own ones, which requires either rework of the DBAL's PDO exception API, or segregating driver exceptions generated by the underlying driver and DBAL-level driver exceptions.

[morozov]

Closing this as effectively resolved by #3348, #3480, #3488 and #3505. Please file a more specific issue if something still needs to be improved.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.