Simplify Driver::connect() signature#4081
Conversation
greg0ire
left a comment
There was a problem hiding this comment.
Are there parts of this that could be backported to 2.11 to make the upgrade path smoother?
src/Driver/Mysqli/Driver.php
Outdated
| * @param array<int,Initializer> $initializers | ||
| * @param array<int,mixed> $options | ||
| * | ||
| * @return array<int,Initializer> |
There was a problem hiding this comment.
Should we use list<> for those?
| * @param string $username | ||
| * @param string $password | ||
| * @param mixed[] $driverOptions | ||
| * @param Initializer[] $preInitializers |
There was a problem hiding this comment.
iterable seems to be enough, we don't really need those to be arrays
There cannot be an FC-layer provided for the changes in the Do you have something else in mind? |
No I read this just before getting to work and did not have the time to figure something out. Not sure if that's a big deal though, I don't think |
greg0ire
left a comment
There was a problem hiding this comment.
As a great man asked me recently,
Does this removal need an upgrade note?
It's absolutely not a big deal. Unless someone maintains their own driver, they shouldn't be concerned about this change at all. |
API design considerations:
Driver::connect()is an adapter between the DBALConnection::connect()and theDriver\Connection::__construct()APIs. It should do all conversion of the parameters and not leakarray $paramsinto the connection.Driver\Connection::__construct()is a tiny wrapper over the underlying driver's "connect" function. Its API should be as close to the "connect" function as possible but not too close to have to leak the underlying connection resource back to the driver like it happens in some PDO drivers:dbal/src/Driver/PDOPgSql/Driver.php
Line 38 in ae8c720
dbal/src/Driver/PDOSqlite/Driver.php
Lines 43 to 47 in ae8c720
In order to avoid the leakage but not to have too many parameters in MySQLi connection constructor either, a concept of connection initializers has been introduced. It can be thought of as an implementation of the Builder or Functional Options pattern.
Additional changes:
$connection->sqlstatein case of a connection failure (should help fixing mysqli::$sqlstate is inaccessible on a failed/non-initialized connection since PHP 8 #3947).$supportedDriverOptionshave been removed from the MySQLi connection. Whether the option is supported will be determined by the call tomysqli_options(). The previous approach led to improper error handling aroundmysqli_options()because an error was impossible to reproduce.Although the code implementing secure MySQLi connection has been modified, it's not tested as it never was. It should be possible to set up a build job using cyprien/mysql-tls on Travis later.
Fixes #4069