Add DBAL Connection Registry

[dmaicher]

Q	A
Type	feature
BC Break	no
Fixed issues	-

Summary

I am currently working on extracting the DBAL part out of DoctrineBundle and into https://github.com/doctrine/dbal-bundle. See doctrine/dbal-bundle#1.

I discussed some details with @stof and @alcaeus on the last SymfonyHackday in Amsterdam.

The new dbal-bundle will need a ConnectionRegistry to retrieve registered dbal connections. Since the existing ConnectionRegistry in the persistence namespace is a bit generic (can return any object and is not specificly for DBAL connections) we propose to add this new registry interface (and a PSR-11 container implementation) here.

@stof maybe you can elaborate some more on how this could potentially also be used to simplify the DBAL commands?

[stof]

Well, if we want to simplify the management of commands in the bundle, the idea would be to move the support of ``--connection` allowing to select a connection from the registry into DBAL itself (once the registry is there, there is no issue depending on it in commands, especially if we also provide another "dumb" implementation for the case where you need only one connection that can be wrapped in this registry for compat with commands)

[stof]

should we throw a generic InvalidArgumentException or an exception defined in DBAL ?

[dmaicher]

For now I used this generic exception similar to

https://github.com/doctrine/persistence/blob/master/lib/Doctrine/Persistence/AbstractManagerRegistry.php#L98

But we can also create a more specific ConnectionNotFoundException or so

[morozov]

Why is it needed in the DBAL? Would it make sense to maintain it in the bundle instead?

[dmaicher]

Why is it needed in the DBAL? Would it make sense to maintain it in the bundle instead?

@morozov see comment from @stof above: #3892 (comment)

It could be useful here to allow selecting a connection for the dbal commands.

Some quick changes to show how:

diff --git a/lib/Doctrine/DBAL/Tools/Console/Command/RunSqlCommand.php b/lib/Doctrine/DBAL/Tools/Console/Command/RunSqlCommand.php
index c7e51933f..4d00db2bc 100644
--- a/lib/Doctrine/DBAL/Tools/Console/Command/RunSqlCommand.php
+++ b/lib/Doctrine/DBAL/Tools/Console/Command/RunSqlCommand.php
@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Doctrine\DBAL\Tools\Console\Command;
 
+use Doctrine\DBAL\ConnectionRegistry;
 use Doctrine\DBAL\Tools\Dumper;
 use LogicException;
 use RuntimeException;
@@ -23,6 +24,17 @@ use function stripos;
  */
 class RunSqlCommand extends Command
 {
+    /**
+     * @var ConnectionRegistry
+     */
+    private $connectionRegistry;
+
+    public function __construct(ConnectionRegistry $connectionRegistry)
+    {
+        parent::__construct();
+        $this->connectionRegistry = $connectionRegistry;
+    }
+
     protected function configure() : void
     {
         $this
@@ -30,6 +42,7 @@ class RunSqlCommand extends Command
         ->setDescription('Executes arbitrary SQL directly from the command line.')
         ->setDefinition([
             new InputArgument('sql', InputArgument::REQUIRED, 'The SQL statement to execute.'),
+            new InputOption('connection', null, InputOption::VALUE_REQUIRED, 'The database connection', 'default'),
             new InputOption('depth', null, InputOption::VALUE_REQUIRED, 'Dumping depth of result set.', 7),
             new InputOption('force-fetch', null, InputOption::VALUE_NONE, 'Forces fetching the result.'),
         ])
@@ -44,7 +57,7 @@ EOT
      */
     protected function execute(InputInterface $input, OutputInterface $output)
     {
-        $conn = $this->getHelper('db')->getConnection();
+        $conn = $this->connectionRegistry->getConnection($input->getOption('connection'));
 
         $sql = $input->getArgument('sql');
 
diff --git a/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php b/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php
index a7a1e987e..1936da9d2 100644
--- a/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php
+++ b/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Doctrine\DBAL\Tools\Console;
 
 use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\ConnectionRegistry;
 use Doctrine\DBAL\Tools\Console\Command\ReservedWordsCommand;
 use Doctrine\DBAL\Tools\Console\Command\RunSqlCommand;
 use Doctrine\DBAL\Tools\Console\Helper\ConnectionHelper;
@@ -33,23 +34,23 @@ class ConsoleRunner
      *
      * @param array<int, Command> $commands
      */
-    public static function run(HelperSet $helperSet, array $commands = []) : void
+    public static function run(ConnectionRegistry $connectionRegistry, array $commands = []) : void
     {
         $cli = new Application('Doctrine Command Line Interface', Versions::getVersion('doctrine/dbal'));
 
         $cli->setCatchExceptions(true);
-        $cli->setHelperSet($helperSet);
+        //$cli->setHelperSet($helperSet);
 
-        self::addCommands($cli);
+        self::addCommands($connectionRegistry, $cli);
 
         $cli->addCommands($commands);
         $cli->run();
     }
 
-    public static function addCommands(Application $cli) : void
+    public static function addCommands(ConnectionRegistry $connectionRegistry, Application $cli) : void
     {
         $cli->addCommands([
-            new RunSqlCommand(),
+            new RunSqlCommand($connectionRegistry),
             new ReservedWordsCommand(),
         ]);
     }

When the cli-config.php script then returns a ConnectionRegistry and not a HelperSet like:

<?php

use Doctrine\DBAL\Driver\PDOMySql\Driver;
use Doctrine\DBAL\Psr11ConnectionRegistry;
use Symfony\Component\DependencyInjection\Container;

$container = new Container();
$container->set('default', new \Doctrine\DBAL\Connection([], new Driver()));

return new Psr11ConnectionRegistry(
    $container,
    'default',
    ['default']
);

Then we have native multi-connection support on the dbal commands and not only when wrapped by the doctrine bundle 😊

Does this make sense to you?

[stof]

And this also means we don't need to extend all commands in bundles anymore to add support for multiple connections (which will allow the possibility to actually make them final in a future major version if needed).

[morozov]

And this also means we don't need to extend all commands in bundles anymore to add support for multiple connections […]

It doesn't sound like valid reasoning to me. An API should be extensible and extended according to the open-close principle. Not modified.

[…] which will allow the possibility to actually make them final in a future major version if needed

We can make the commands explicitly extensible (not via inheritance) if projects need. Or at least agree to not make them final.

Just adding a bunch of new code because another project needs it looks like a bad idea to me.

[alcaeus]

@morozov handling multiple connections in the commands is very painful at the moment, since the connection helper provided by DBAL only knows a static connection. Right now, DoctrineBundle (and future DBALBundle and ORMBundle) need to extend the command to add a connection argument, then hook into the execute method to inject the selected connection into the helper.

The helper provided by DBAL can only handle a single connection, which is why we have this very helpful suggestion for users of DBAL to get the console commands running (source):

// replace with the mechanism to retrieve DBAL connection in your app
$connection = getDBALConnection();

We're telling the user to handle the connection, but we're not giving them any tools to do so. For a long time, we've used the DoctrineBundle to handle these shortcomings, at the expense of anyone not using the bundles to integrate ORM or DBAL into their projects (e.g. because they are not using Symfony).

Having a connection registry that can be used to handle connections along with an CLI argument to select a connection would improve usability for all users, by allowing us to document how to add connections to this registry and how they can be selected in commands (e.g. by passing a connection CLI argument).

Just adding a bunch of new code because another project needs it looks like a bad idea to me.

Sorry, but no. We're not adding a "bunch of new code" because another project needs it. We're adding it because DBAL knows about connections and is technically able to handle multiples of them, but doesn't provide any tools working with them. This is not any project needing this, we're talking about our own projects here that ensure 90% of our users get a sensible set of tools to work with.

Keeping this code in the bundle doesn't make any sense: it will always be Doctrine people maintaining it, but the maintenance burden is increased because any change that should affect a single project (e.g. adding a new CLI command in DBAL) requires work in the bundle (e.g. extending that command with boilerplate code to inject the selected connection). This doesn't make any sense.

[greg0ire]

It doesn't sound like valid reasoning to me. An API should be extensible and extended according to the open-close principle. Not modified.

That's not how I understand the open-closed principle at all. This PR only adds new types, so there's no OCP violation here IMO. Or do you consider adding a new type modification of the package? To me, it changes nothing to existing behavior here: the API is extended, not modified, so much so that it looks completely BC to me. @dmaicher, you should target a lower branch IMO.

Also, as a developer, if I have several databases, I would very much like to be able to name connections to them and retrieve them from my DI container.

[stof]

@morozov there is no clean way to support multiple connections in commands right now, because DBAL commands are about built around a single connection and don't have an option to select it.
Extending the command in the bundle is the only way we have currently to add that support in the command. And being able to manage multiple connections and choosing the one for which the command is executed is a common need in projects.

I'm all for the open-closed principle. And that's precisely the reason why I suggested to move the support for connection switching in commands to DBAL itself, because adding it from the bundles forces us to break that principle.

[morozov]

Currently, DBAL doesn't have any notion of multiple connections. This is true. And adding a registry here doesn't add this notion. If you want to implement a feature of managing multiple connections, please come up with a proposal and we can discuss it. Just adding a registry solves the problem of the bundle and the projects that use it but it doesn't belong to the DBAL.

[stof]

@morozov the goal is to have a follow-up PR moving the support for --connection to the console commands implemented in DBAL itself, instead of having to extend all commands in the bundle to support that.

AFAIK, that's the only part of DBAL where it is relevant, because that's the only part of DBAL needing to get the connection. Everything else is happening from the connection already (other projects built on top of DBAL may have places where this is relevant though, and having the registry in DBAL rather than the bundle might also make it easier for them to support it).

[dmaicher]

@morozov @stof I will try to come up with another PR that will use the registry on the commands

[morozov]

the goal is to have a follow-up PR moving the support for --connection to the console commands […] AFAIK, that's the only part of DBAL where it is relevant

If that's the case, then why do you need a registry at all? Each command invocation will use at most one command given the connection name, e.g. function (string $name) : Connection. Please do not over-engineer this unless you get paid for LOC.

[stof]

@morozov The advantage of the registry over a callable ?string $name => Connection is that the signature of the callable cannot be enforced by PHP.
Also, for people using the Symfony DI container, wiring a callable is more complex (we would have to implement the same object, except that we would have __invoke rather than getConnection).

[morozov]

@stof let's not over-think this and solve one problem at a time. Please. If the problem at hand is the CLI command, let's fix it by introducing the necessary minimum of code. I do not see a registry or any API exposed from the DBAL needed to solve the CLI command problem.

[dmaicher]

@morozov so you would prefer something like this?

https://github.com/doctrine/dbal/compare/2.11.x...dmaicher:commands_use_registry?expand=1

[morozov]

Yeah, something like that but I don't get it who's going to inject the registry into the command. Right now, the command gets the DB from the helper. Could the helper implement getDefaultConnection() and getConnection($name) instead?

I still fail to see the need for a dedicated registry in the DBAL if it's not going to use it anywhere else than in CLI.

[morozov]

Okay… as far as I understand, an application that depends on DBAL and uses its CLI command may manage the DB connections itself and may want to inject those connections into the command explicitly. In this case, instead of a registry, I'd introduce only an interface in DBAL like:

namespace Doctrine\DBAL\Tools\Console;

interface ConnectionProvider
{
    function getDefaultConnection() : Connection;
    function getConnection(string $name) : Connection;
}

If the application manages connections in an PSR-11 compatible registry, it could wrap it into the interface above and inject. I still don't see much point in implementing the registry in DBAL or supporting PSR-11 for connections. It's a general-purpose DI container while we can afford a more specific interface.

[dmaicher]

@morozov yes we can also create a more simplified interface for this. Seems better than using callbacks.

We should then deprecate the ConnectionHelper, or? We could have a BC layer with deprecations in 2.11 and then make it mandatory to inject a provider in 3.0?

[morozov]

We should then deprecate the ConnectionHelper, or? We could have a BC layer with deprecations in 2.11 and then make it mandatory to inject a provider in 3.0?

Why is it “or”? It's all of the above. The connection provider and the BC layer are implemented, and the helper is deprecated in 2.11, then the BC layer is removed in 3.0.

[alcaeus]

Why is it “or”?

Because it's a very common German thing to end a sentence with "or?" to get confirmation, similar to "right?" in English. So it wasn't really a "this or that" thing, but rather "this and that, if you agree"

[morozov]

Then we're on the same page, or?

[dmaicher]

@morozov yes I will propose a new PR 👍