Add command to ping the database #3697
Conversation
mcfedr
force-pushed
the
ping-command
branch
2 times, most recently
from
e998f16
to
95473de
Compare
|
@greg0ire Commands shouldn't be final as they are wrapped in DoctrineBundle - this bundle also provides the arguments to allow connection switching. |
Would it be possible to do the wrapping using decoration? This could avoid issues with type declarations. |
I'm sure it would, but it seems outside of the scope of this PR, rather some refactoring to be done, first in DoctrineBundle, and then here with all the commands, and in the similar commands in ORM.
What issues? |
|
@greg0ire I think all the issues have been addressed. |
Changes in type declarations in parent classes can be incompatible with the parent, and vice versa, and disappear if you use decoration. For instance, phpunit has been adding the If you choose decoration, I would not require you to refactor the doctrine bundle and the dbal. Maybe documenting extension as a deprecated way of doing things in the project or as a github issue would be sufficient. |
|
I'm just looking at what it would take to decorate this command in bundle, it feels like a much more brittle solution in this case - for example I'd have to get all the options, and maybe the (currently) empty arguments and recreate them - the only non-private parts of the command is the interface inherited from |
|
Ok, if it's not straightforward let's stay with extension I guess. I think the command in the bundle will not be just about renaming this command and using a different description is it? Could you please open the PR on the bundle to show us how it will look like? I'm asking because |
| } catch (Throwable $e) { | ||
| $output->writeln(sprintf('Connection error: <error>%s</error>', $e->getMessage())); | ||
|
|
||
| return 2; |
There was a problem hiding this comment.
What difference does it make for the caller? The ping either succeeded or didn't. Note that in DBAL v3, ping will throw an exception on failure, so it's not clear how this would have to be reimplemented w/o breaking the existing behavior.
There was a problem hiding this comment.
It probably makes little difference to the caller, but as there are two different failures it doesnt do any hard to be more informative - note that this becomes the exit code from the process
There was a problem hiding this comment.
Ok, I've changed the text to just be 'ping failed', so it won't be misleading when DBAL 3 throws on different failure types - anyway the caught exception will say its a connection error or something else.
There was a problem hiding this comment.
I still don't understand the meaning of the two different return codes and how they will be supported in 3.0.
mcfedr
force-pushed
the
ping-command
branch
3 times, most recently
from
38cad58
to
0f6b0f0
Compare
mcfedr
force-pushed
the
ping-command
branch
2 times, most recently
from
a4d7ade
to
994ef21
Compare
|
@greg0ire I've now opened doctrine/DoctrineBundle#1035 with the command added there. |
mcfedr
force-pushed
the
ping-command
branch
3 times, most recently
from
dab0979
to
007119d
Compare
|
@greg0ire I think all is good now? |
No problem :) |
|
Pinging a database is no longer supported (#4128). |
Summary
Adds a useful command to ping the database, it can do this in a loop to wait for it to be available.
Particularly useful when you want to run commands linked to docker containers, and you need to wait for the database to be up, and you don't have
mysqlclient installed, plus its more useful than that because it uses your existing doctrine config and can connect to any database.Also great for tests when you need to provision and spin up a db server on the fly.