allow using multiple connections for CLI commands

[dmaicher]

Q	A
Type	feature
BC Break	no
Fixed issues	-

Summary

follow-up for #3892

This

• adds a ConnectionProvider interface and a simple implementation for it
• uses this provider in a backwards-compatible way for the console commands
• deprecates the ConnectionHelper that was providing a single connection

[dmaicher]

Actually this change would also affect doctrine/migrations and possibly doctrine/orm cli tools. They also use this console command helper approach

[morozov]

Looks good for the most part 👍 Just needs some cleanup.

[dmaicher]

@stof I think we don't need this change for the doctrine dbal-bundle.

Its actually only one single command that we need to extend/proxy:

https://github.com/doctrine/dbal-bundle/pull/1/files#diff-3c45b0c1750eeaf9333a6f76e86e91adR15

So @morozov I'm fine if we close this. Not sure the introduced deprecations and overhead for needed fixes on related bundles/libraries are justified. If you still think this feature is useful let me know and I can fix the conflicts and we can finish this PR.

[morozov]

I think we don't need this change for the doctrine dbal-bundle.

No offense, but this is exactly why I was against the original patch in #3892 (comment):

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

Whether it's adding or non-adding a feature, should be defined by whether it belongs to the project, not by whether the other project needs or doesn't need it.

If you still think this feature is useful let me know and I can fix the conflicts and we can finish this PR.

I believe it makes the functionality more extensible (i.e. the possibility to use one or more connections in CLI). Even if the bundle doesn't need it, other projects still may.

[morozov]

@dmaicher could you squash everything, please?

[dmaicher]

@morozov done 😉

[morozov]

Thanks, @dmaicher.

[morozov]

@dmaicher would you mind submitting a pull request that removes the deprecated code from 3.0.x?

[dmaicher]

@morozov see #4059 😉

[morozov]

It's nice working with you, sir.

[PowerKiKi]

I might be missing something, but I think this break the implicit compatibility with doctrine/orm when using cli-config.php to configure CLI tools of both dbal and orm.

If I want to avoid ConnectionHelper, which has been deprecated by this PR, it seems I have to return directly a ConnectionProvider instead of a HelperSet. But orm package still require a HelperSet. That means that the following two commands cannot work at the same time anymore:

./vendor/bin/doctrine-dbal --version # will work
./vendor/bin/doctrine --version    # will throw exception

Moving forward what is the plan to support a single configuration file for both tools ?

[morozov]

@PowerKiKi please file an issue in the ORM repository. There isn't much we can do about it here.

[PowerKiKi]

I forgot to mention that it also affects doctrine/migrations, since ./vendor/bin/doctrine-migrations --version will fail in the same way as ORM.

So I don't think it's a problem with ORM, nor with Migrations. But it is a problem with DBAL itself, who broke compatibility with all others. Because DBAL now refuse to accept "too much" configuration, then DBAL cannot be used with other tools anymore.

Also I don't see how ORM, or Migrations, could be made to work with only a ConnectionProvider. It requires other things, such as em for ORM and configuration for Migrations. Because of this PR (and even more so with #4059), it became nearly impossible to share a single config file that return different things depending on who is using that config.

I think this PR went overboard. And instead of replacing HelperSet with a ConnectionProvider, it should have kept the HelperSet and accept a ConnectionProvider in that HelperSet. This would allow for the improvement originally intended while preserving compatibility across the doctrine ecosystem.

Would you accept a PR that restore the HelperSet as the only right way to configure DBAL (and ORM and Migrations), while preserving the new ConnectionProvider in that HelperSet ?

[morozov]

Also I don't see how ORM, or Migrations, could be made to work with only a ConnectionProvider. It requires other things, such as em for ORM and configuration for Migrations.

ConnectionProvider is an interface that defines the API required by the DBAL. Apart from the API defined by the interface, the implementing class can implement any other API required by any other use cases. What's the problem in implementing such classes for the ORM and Migrations?

[PowerKiKi]

Fair enough, it can be made to work. I somehow missed the fact that it was an interface, and it could be implemented in other packages too.

Still it seems it would have been better if other packages had similar PR at the same time. Right now there is no way to use latest DBAL with ORM or Migrations, without using something deprecated.

[PowerKiKi]

I created the following issues to track progress in their respective packages:

• cli-config.php compatibility with DBAL  orm#8327
• cli-config.php compatibility with DBAL #8327 migrations#1070