drush-ops / drush

Drush is a command-line shell and scripting interface for Drupal, a veritable Swiss Army knife designed to make life easier for those who spend their working hours hacking away at the command prompt.
https://www.drush.org
2.34k stars 1.08k forks source link

Filter down the list of commands to be version (or Boot class) specific #1749

Open quicksketch opened 8 years ago

quicksketch commented 8 years ago

Right now Drush only has a single set of commands as specified by hook_drush_command(). All commands provided are shown for all versions of Drupal.

If you attempt to use a command not available, then BootClass::enforce_requirement() is called (excellent!) and results in something like this:

❰nate❙~/Sites/drupal7❱✔≻ drush config-edit
Command config-edit requires Drupal core version 8+ to run.          [error]
The drush command 'config-edit' could not be executed.               [error]

Likewise, commands like variable-get are shown for Drupal 8, but only work on Drupal 6/7. IMO it would be preferable to filter down the list of commands to apply only to the applicable ones for the current version when displaying the drush help. Would calling enforce_requirement() for every command have too much overhead? Or can we come up with a way to filter down that list from hook_drush_command() directly (e.g. by using the existing "core" property)?

weitzman commented 8 years ago

Historically we have wanted the help command to start printing info immediately for best user experience. So that means we discover and print core Drush commands during bootstrap phase NONE. I think it would be an interesting PR to defer this to bootstrap phase ROOT and run the version enforcement before printing. Perhaps @greg-1-anderson has an opinion here.

greg-1-anderson commented 8 years ago

I didn't time it, but I think that bootstrapping to ROOT is fast. I'm not sure about running hook requirements on all commands. I think this feature is not heavily-used outside of Drush core, so this is probably okay. However, the danger here is that the current contract guarantees that the site will be bootstrapped to its full level before it runs.

We could change the contract, but if we did that, and someone later came along and decided that the best way to "fix" their code was to call bootstrap max, then drush help would suck. Maybe we should introduce a quick requirement check API that is called early, with the bootstrap at ROOT, and switch all of Drush core to use the new API. Maybe that's too conservative; not sure here.

greg-1-anderson commented 8 years ago

Another issue here is, what do we do when the user runs --help from outside the context of any site? Site-specific commands magically show up when you enter the context of a site that contains extra commands, but this somehow seems like the wrong approach for built-in commands.

In wp-cli and terminus, the default "help" command only shows categories of commands that are available. You can only get help on one category at a time. This helps manage the length of the help output, at the cost of discoverability. We might consider something like this; if the Drupal and Backdrop commands were in separate sections, perhaps we could be more selective about bootstrapping depending on the section of commands being displayed.

quicksketch commented 8 years ago

Thanks, great ideas here. I realized that of course we wouldn't need to call enforce_requirement on every command when any command is called. This really only affects drush --help or just drush with no parameters specified. Under such circumstances, it seems like checking each requirement beforehand would be acceptable.

Another issue here is, what do we do when the user runs --help from outside the context of any site? Site-specific commands magically show up when you enter the context of a site that contains extra commands, but this somehow seems like the wrong approach for built-in commands.

I'm not sure about this. What use is variable-set or user-login if you're not already within a Drupal site directory? I'd prefer to see the list filtered down to what is actually possible in the current context, at least when running drush with no --help. Or perhaps we add a flag for --all that would show you all commands, even the ones that you can't run in the current context?

greg-1-anderson commented 8 years ago

Yes, I was speaking only about help; I think we want to avoid bootstrap max here.

Also, folks experiment with and learn about Drush even before they have installed their first site. Whatever we do, there has to at least be some indication on how to get help about framework-specific help when running drush help from a non-bootstrapped context.

weitzman commented 8 years ago

I cleaned up help command a lot. I can implement this easily now - we just have to decide what we want.

I kinda like enforcing requirements by default and adding a --all option for the curious. We would document that option somewhere prominent.

Disregard what I said above about speed and bootstrap level. help only bootstraps to CONFIGURATION phase. We used to print after each phase but I think those days are long past.

@greg-1-anderson - We display help by category if one provides --filter without a value. I dont personally think that should be the default presentation.

greg-1-anderson commented 8 years ago

Most cli commands with a large command set, such as git, follow the model that help only shows the categories or general help information. I think following the standard is slightly better here, but I can certainly live with the traditional drush model if that is preferred.