Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move CLI tools into a separate package #5076

Closed
derrabus opened this issue Dec 2, 2021 · 13 comments
Closed

Move CLI tools into a separate package #5076

derrabus opened this issue Dec 2, 2021 · 13 comments

Comments

@derrabus
Copy link
Member

derrabus commented Dec 2, 2021

I'm moving this discussion into a separate issue to make it more visible and split it from the code review of #5070.

Original comment by @morozov:

During the discussion of the conflict attribute, I recalled that despite being a development dependency of the DBAL itself, the CLI command could be used during the development of other applications. So it's not just a development dependency. It's sort of an "optional" dependency but it's not declared as such.

Given that the command line tool relies on a very small and stable subset of the DBAL APIs and it's not a production dependency, would it make sense to extract it into a separate package? E.g. doctrine/dbal-cli. This way, the package could explicitly declare its dependencies and we could get rid of the dependency on symfony/console here which may cause conflicts like #4967 (comment).

Additionally, similar to the Dumper component, its primary purpose is development experience while it's a much relatively complex and controversial topic which is out of the scope of the DBAL.

Currently, the CLI does two things:

  1. Execute a query, for which there are millions of more convenient ways (e.g. PhpStorm).
  2. Display reserved keywords. The need to care about the keywords will eventually go away once we fix quoting identifiers.

Maintaining a tool and its dependencies for such edge cases seems overkill to me.

UPD: this isn't meant to block the work on this PR. I just want to hear your opinions and depending on those work on this issue separately.

Originally posted by @morozov in #5070 (comment)

Comment by @greg0ire:

I think you're making good points, but I wonder if we should deprecate these commands instead… I've personally rarely used them.

To add on what you said about the primary purpose being DX, it also feels like a liability because it allows developers to use the apps credentials to run arbitrary SQL queries in production… it's kind of a backdoor. Having it as a separate package would allow teams to avoid having this backdoor deployed to production.

Originally posted by @greg0ire in #5070 (comment)

@morozov
Copy link
Member

morozov commented Dec 2, 2021

Personally, I haven't used these commands a single time except for the cases when I had to modify their code. I would be fine with just deprecating them. If we extract the CLI to a separate package, I will have no interest in maintaining it.

The only significant contribution in the recent past I recall was #3956. It shows that some people may be interested in it being maintained but it doesn't mean it should be the Doctrine organization that owns it.

@derrabus
Copy link
Member Author

derrabus commented Dec 2, 2021

Execute a query, for which there are millions of more convenient ways (e.g. PhpStorm).

There are certainly more convenient tools to run a query. Compared to dedicated clients, this command looks like a toy. If I wanted a sophisticated UI, PhpStorm/DataGrip is the better choice. If wanted a CLI, my database vendor probably provides one that is better integrated and way more capable than our command.

I usually use the DBAL inside Symfony applications. Here, the DBAL and ORM commands are integrated into the applications console by DoctrineBundle. It totally makes sense to have such a command there as a debug tool. It allows me to access the database with exctaly the same connection settings as my application is configured to use. And that alone makes the command valuable for me. I rarely use it, but when I do, I'm glad that it's there.

Display reserved keywords. The need to care about the keywords will eventually go away once we fix quoting identifiers.

Database vendors have documentation for that purpose, right? I've never used that command and would be happy to remove it.

What bugs me more is this rudimental console applications that DBAL and ORM throw into my vendor/bin directory. They don't work out of the box. I have to place a clumsy cli-config.php file to some magic location to make it work. The very existence of two apparently broken doctrine binaries confuses developers who have never worked with Doctrine before. This is the part of the console integration, that I personally would love to get rid of.

We could document a code snippet for a console binary that developers could paste into their projects instead of placing a config file they don't understand to some magic place to make a vendor-provided console work. They could adjust that script to their needs and would be in full control.

Taking all of this into account, what's left is a single command class, that a developer or a framework integration may wire into a Symfony console application if they need it. And honestly, I don't need a dedicated package for a single class that does no harm if it lies around dormant.

@morozov
Copy link
Member

morozov commented Dec 3, 2021

I usually use the DBAL inside Symfony applications. Here, the DBAL and ORM commands are integrated into the applications console by DoctrineBundle. It totally makes sense to have such a command there as a debug tool. It allows me to access the database with exctaly the same connection settings as my application is configured to use. And that alone makes the command valuable for me. I rarely use it, but when I do, I'm glad that it's there.

Does this functionality depend on the tools provided by the DBAL package (i.e. bin/doctrine-dbal and src/Tools) or there an independent implementation?

We could document a code snippet for a console binary that developers could paste into their projects instead of placing a config file they don't understand to some magic place to make a vendor-provided console work. They could adjust that script to their needs and would be in full control.

I'd love to see it done this way. Unless somebody expresses their interest in maintaining the new package, it will be abandoned since its inception.

@morozov
Copy link
Member

morozov commented Dec 3, 2021

@derrabus if you could document the code snippet you have in mind (as you're the one who has at least some experience using them), we could deprecate this functionality.

@derrabus
Copy link
Member Author

derrabus commented Dec 3, 2021

Will do.

@stof
Copy link
Member

stof commented Dec 3, 2021

@morozov DoctrineBundle depends on the Command classes in the Tools namespace, but not on the bin/doctrine-dbal file (it registers the commands to expose them through bin/console of the Symfony project.

I admit that in 10 years, I have never used any of these 2 DBAL commands (the ORM commands are a different matter)

@morozov
Copy link
Member

morozov commented Mar 28, 2022

So, if I understand you @derrabus correctly, you find the dbal:run-sql command useful in certain cases. Since it's primarily used from within a Symfony application and depends on Symfony Console, would it make sense to remove the entire Tools\Console namespace from the DBAL and reinstate the bits that are still useful in DoctrineBundle?

This way, it will still be available to Symfony users and will let us remove the dependency of the DBAL on Symfony Console. In general, a storage-level library that depends on a presentation-level library looks odd.

@derrabus
Copy link
Member Author

DoctrineBundle is an integration package for Symfony full-stack applications. The console component however is used standalone quite often.

Moving the classes there would make them less accessible for non-Symfony applications. I Don't think that this is the right place for it. As I said earlier, I'd just keep them here.

@morozov
Copy link
Member

morozov commented Mar 29, 2022

So we resolve this issue as "Won't Fix"?

@derrabus
Copy link
Member Author

If that's okay for you, yes.

@morozov
Copy link
Member

morozov commented Mar 30, 2022

Well, it doesn't solve the originally reported problem that the DBAL depends on something that it shouldn't depend on. The only alternative is to move the namespace to a separate package. Similar to the bundle, it's used for development, and isn't part of the database abstraction.

@derrabus
Copy link
Member Author

The DBAL does not depend on Symfony Console; it works very well without it. Symfony Console can optionally be installed to use two console commands which are an absolutely non-essential feature. I wouldn't create a separate package for two tiny command classes and a helper class.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants