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:

dbal/lib/Doctrine/DBAL/Connection.php

Lines 221 to 228 in 6ff72e4

	* @internal
	*
	* @return mixed[]
	*/
	public function getParams()
	{
	return $this->params;
	}

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 #4308 :)

[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.