Dbal: MasterSlaveConnection was BC Broken in 2.11.0 affecting doctrine/doctrine-bundle

[kralos]

Bug Report

| Q | A
|------------ | ------
| BC Break | yes
| Version | 2.11.0

Summary

Doctrine\DBAL\Connections\MasterSlaveConnection has changed how it handles $params (__construct arg 1). Specifically, it now unsets the legacy parameters master / slaves. This is a BC break because Doctrine\DBAL\Connection::getParams is public. Anything relying on these parameters will now break however the major version of doctrine/dbal was not bumped.

Current behaviour

For example doctrine/doctrine-bundle's Commands use the following code to extract connection params:

$params = $connection->getParams();
if (isset($params['master'])) {
    $params = $params['master'];
}

These commands now fail:

03:26:50 ERROR     [console] Error thrown while running command "{command}". Message: "{message}" ["exception" => InvalidArgumentException { …},"extra" => ["command" => "-v doctrine:database:create","message" => "Connection does not contain a 'path' or 'dbname' parameter and cannot be created."]]

In CreateDatabaseDoctrineCommand.php line 82:

  [InvalidArgumentException]                                                         
  Connection does not contain a 'path' or 'dbname' parameter and cannot be created.  

How to reproduce

$masterParam = [/***/];
$slavesParam = [/***/];
$conn = new \Doctrine\DBAL\Connections\MasterSlaveConnection([
    'master' => $masterParam,
    'slaves' => $slavesParam,
]);
\assert($conn->getParams()['master'] === $masterParam); // fails, used to pass in doctrine/dbal 2.10.4
\assert($conn->getParams()['slaves'] === $slavesParam); // fails, used to pass in doctrine/dbal 2.10.4

Expected behaviour

Doctrine\DBAL\Connections\MasterSlaveConnection should not unset($params['master']); / unset($params['slaves']);

[greg0ire]

On the one hand, this is indeed a BC-break, on the other hand, as pointed out by @stof, Connection::getParams() is marked as internal:

https://github.com/doctrine/dbal/blob/6ff72e4596ad8c597dedd49ac3249d727d71ce62/lib/Doctrine/DBAL/Connection.php#L221-L228

It's like this since 2.11.0 though, maybe that should have been done in 3.0.0? Marking a method as internal sounds a bit like a BC-break. Anyway, we might revert this, but any piece of code relying on getParams() will have to be migrated in the future.

[kralos]

If it was marked @internal in 2.10.x I would have instead raised an issue in doctrine/doctrine-bundle. I don't think we need to revert, just bug fix the BC break by removing the unsets.

[greg0ire]

I think it makes sense, can you send a PR?

[morozov]

Fixed by #4295.

[mvorisek]

Fixed by #4295.

by https://github.com/doctrine/dbal/pull/4308 :)