Closed (works as designed)
Project:
Drush
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Issue tags:
Reporter:
Created:
17 Aug 2009 at 17:21 UTC
Updated:
24 Jan 2010 at 05:54 UTC
Jump to comment: Most recent file
In a command's hook_drush_init I've added a check that sets an error if a php extension is not available. Afterwards I realized that hook_drush_init and hook_drush_exit is executed for all commands (all commands that doesnt define explicitly a callback and consequently pass through drush_command()). It breaks all commands if the requirements to run a single command are not met. I think that theese hooks should only be called for current command and not for all those that provide such a hook.
My rationale is that if a command depends upon the initialization of others it must initialize the other explicitly in it's own hook. It saves unneeded initializations on each command and can make execution a millionsec faster.
Attached is a patch to accomplish this. In addition the patch makes drush_command_hook() to return the hook function name or false. I know,... making traversal changes is not the best.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | hook_init_exit_1.patch | 1.7 KB | jonhattan |
| hook_init_exit.patch | 1.69 KB | jonhattan |
Comments
Comment #1
moshe weitzman commentedWe already have a validate step before command execution. See drush_invoke(). Does that not resolve the need to check for extension before a given command? I have not looked at your patch yet.
Comment #2
jonhattanOk I can move the check to validate. Anyway this issue can be considered independently of my concrete problem.
Comment #3
jonhattanA little note with no enough body to fill a new issue: drush_invoke() do
twothree calls to function_exists. The second one is not needed.Comment #4
adrian commentedThe drush_init and drush_exit hooks are similar to the drupal init and shutdown hooks.
they are basically used to bootstrap the command file. (ie: set up defaults, etc.)
If they are removed there is no other way to do this other than define those functions for every single command you want to have.
so it moves from 1 function definition to the same function definition for how ever many commands you have.
you can more easily write an if () statement in the hook to only do the initialization code for the commands you want to override,
and you have _validate , _pre and _post hooks for each command already.
Comment #5
jonhattanadrian: what I actually mean is to only execute drush_init and drush_exit of the commandfile the command to run is defined in (in oposition to executing drush_init for all commandfiles each time). I think it's better expressed in the patch :)
same patch attached with a fix to indentation.
Comment #6
moshe weitzman commentedany comment, adrian?
Comment #7
jonhattanFixing the title to be more concise.
I've realised that also a `hidden` thing (feature or bug¿?) is that those hooks are only invoked if the command definition does not explicit set a 'callback' key so it go through drush_invoke. If a 'callback' key is set, the callback will be called directly.
Comment #8
jonhattanI've provide a patch for provision to check and not run its hook_inits if the command is not provision => #562164: Check for the command being run in provision_mysql_drush_init() and provision_apache_drush_init().
It is a design thing to do it as I propose in this issue or do the other way, that is, each commandfile that implements a hook_init should check to only run it for the commands it defines. It would be documented as a best practice. It also would be docummented the fact that any hook_init or _exit would be run if a command defines explicitly a callback as I said in previous command.
changing status to needs review and tagged with `design`, as it's now more of a design question :)
Comment #9
moshe weitzman commentedIMO there is no more to do here. invoke hooks are reasonable documented in api.drush.inc