Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Driver::connect() signature #4081

Merged
merged 4 commits into from
Jun 17, 2020
Merged

Conversation

morozov
Copy link
Member

@morozov morozov commented Jun 16, 2020

Q A
Type improvement
BC Break yes

API design considerations:

  1. Driver::connect() is an adapter between the DBAL Connection::connect() and the Driver\Connection::__construct() APIs. It should do all conversion of the parameters and not leak array $params into the connection.
  2. 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:
    1. $connection->getWrappedConnection()->setAttribute(PDO::PGSQL_ATTR_DISABLE_PREPARES, true);
    2. $pdo = $connection->getWrappedConnection();
      foreach ($this->_userDefinedFunctions as $fn => $data) {
      $pdo->sqliteCreateFunction($fn, $data['callback'], $data['numArgs']);
      }

      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:

  1. The custom error handler in MySQLi connection has been removed in favor of an error suppression operator. The error handler would yield the same results but required maintenance and a dedicated unit test that doesn't pass on Windows. Additionally, it masked improper usage of $connection->sqlstate in case of a connection failure (should help fixing mysqli::$sqlstate is inaccessible on a failed/non-initialized connection since PHP 8 #3947).
  2. $supportedDriverOptions have been removed from the MySQLi connection. Whether the option is supported will be determined by the call to mysqli_options(). The previous approach led to improper error handling around mysqli_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

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there parts of this that could be backported to 2.11 to make the upgrade path smoother?

* @param array<int,Initializer> $initializers
* @param array<int,mixed> $options
*
* @return array<int,Initializer>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use list<> for those?

* @param string $username
* @param string $password
* @param mixed[] $driverOptions
* @param Initializer[] $preInitializers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iterable seems to be enough, we don't really need those to be arrays

@morozov
Copy link
Member Author

morozov commented Jun 17, 2020

Are there parts of this that could be backported to 2.11 to make the upgrade path smoother?

There cannot be an FC-layer provided for the changes in the Driver::connect() signature. The very fact that the drivers start using the "user", "password" and "driver_options" keys of $params that they used to ignore, even as a fallback, is a BC break.

Do you have something else in mind?

@morozov morozov closed this Jun 17, 2020
@morozov morozov reopened this Jun 17, 2020
@greg0ire
Copy link
Member

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 Driver classes are directly used downstream, are they?

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a great man asked me recently,

Does this removal need an upgrade note?

@morozov
Copy link
Member Author

morozov commented Jun 17, 2020

Not sure if that's a big deal though, I don't think Driver classes are directly used downstream, are they?

It's absolutely not a big deal. Unless someone maintains their own driver, they shouldn't be concerned about this change at all.

@morozov morozov added this to the 3.0.0 milestone Jun 17, 2020
@morozov morozov merged commit d89cf16 into doctrine:3.0.x Jun 17, 2020
@morozov morozov deleted the issues/4069 branch June 17, 2020 19:12
@morozov morozov linked an issue Jun 17, 2020 that may be closed by this pull request
@morozov morozov self-assigned this Jun 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants