Remove the doctrine-dbal binary
#5337
Conversation
|
The PHPCS error is unrelated to this PR. I can reproduce it on all maintained branches. |
|
This should be a side effect of the unlocked indirect dependencies. Must have been fixed in phpstan/phpdoc-parser 1.4.1. |
| $commands = []; | ||
| $connectionProvider = require $configFile; | ||
|
|
||
| ConsoleRunner::run($connectionProvider, $commands); |
There was a problem hiding this comment.
@derrabus so now that this file is removed, what's the role of the ConsoleRunner class? I see that it still exists but it's not used in the code. It doesn't seem to be used in DoctrineBundle either.
From the details you posted earlier in #5076 (comment)
[...] the DBAL and ORM commands are integrated into the applications console by DoctrineBundle.
It doesn't look like the runner is used there. It seems to have been used only by this file which no longer exists.
Should ConsoleRunner be deprecated and removed entirely?
UPD: I see it's used in the documentation. But it's not tested, so it's still mostly dead code.
In my understanding, it's a boilerplate code used to bootstrap a Symfony Console application. It may be useful only if the application that depends on the DBAL doesn't bootstrap one itself. Why should the DBAL maintain the code that should be possible to copy-paste from the documentation?
There was a problem hiding this comment.
In my understanding, it's a boilerplate code used to bootstrap a Symfony Console application.
Yes.
It may be useful only if the application that depends on the DBAL doesn't bootstrap one itself.
No. If your application bootstraps it's own console application, you can call ConsoleRunner::addCommands() to add the DBAL's commands to it. If it doesn't, ConsoleRunner::run() bootstraps the application for you and runs it.
Why should the DBAL maintain the code that should be possible to copy-paste from the documentation?
The idea is that ConsoleRunner::run() will remain a stable piece of boilerplate, even if we add/remove/rename command classes. Removing the class would complicate bootstrapping the console application unnecessarily.
But it's not tested, so it's still mostly dead code.
Let's add tests then.
There was a problem hiding this comment.
No. If your application bootstraps it's own console application, you can call
ConsoleRunner::addCommands()to add the DBAL's commands to it.
There's currently just one command (the other is deprecated). ConsoleRunner::addCommands() accepts the same parameters as Application::addCommands(), i.e. requires a ConnectionProvider to be passed. What value does it provide this way?
The idea is that ConsoleRunner::run() will remain a stable piece of boilerplate [...]
That's what the documentation for the CLI framework is for. As soon as a user wants to make a change in their CLI application behavior (e.g. change the name, implement their own error handling), they will have to copy-paste our code. It's better to have them do that from the beginning.
There was a problem hiding this comment.
The current code violates the SRP since it acts both as a runner and a DI container for commands. I'd agree to keep the DI container part if there was a variety of commands and a complex dependency resolution but it's just one line of code that does nothing.
There was a problem hiding this comment.
The current code violates the SRP
I really fail to see why this is a problem for bootstrap code, tbh.
I'm sorry, but assumed we've settled the discussion around the CLI tools already. The amount of time and energy I can invest in open source work these days is limited, so I'd rather not spend it on defending a piece of boostrap logic that I haven't even written myself.
Please remove ConsoleRunner if it really bugs you that much.
The problem is that we may want to continue the support of a larger component just for the sake that a part of it may be useful (which practically isn't useful either). I'm sorry that you had to waste your time. It is my fault that I didn't take a closer look at this code when we discussed it previously.
Will do. |
Summary
This PR removes the
doctrine-dbalbinary deprecated with #5084.