Explicit callback functions with special names

I committed this since it does fix user-create. Hopefully Greg can take a look and validate it still.

What we have is that in a commandfile user.drush.inc we have a function with a callback drush_user_create. Drush wants all of the callbacks in the "user" commandfile to begin with drush_user. In the past, the callback for user-create in user.drush.inc would be drush_user_user_create, but this was shortened in drush 3.x to drush_user_create for readability purposes. The problem that arises here is that when we have an explicit callback function (e.g. drush_user_create), we need to derive the command hook. In this case, the command hook should be "user_create", but because of the overlap between "drush_user_" and "user-create", we get "create".

The correct solution is to insure that the length of the command hook is never less than the length of the command.

I recommend backing out the submitted patch. I object slightly to the constant "6" (strlen("drush_")), but my larger issue is this line:

if ($commandfile = substr($command['callback'], 6, strlen($commandfile))) {

That "=" should be an "==". As written, the first branch of the if always executes. That could interfere with some people's hook functions. The other problem is that commandfile is assigned to, which could have unpredictable consequences depending on the contents of $command['callback'].

The patch might be close enough if you change the = to ==.

Update: The correct solution is to use the command name (e.g. "user-create") as the command hook iff the last N characters of the callback match the command name. If they do not match, then the code should be as it currently exists (where it takes the last part of the callback minus the required prefix).

The command name must be adjusted to have "_" instead of "-" before comparing or using as a command hook.

I appreciate the extra patch; unfortunately, I was somewhat wrong in #5, and the real solution is somewhat more complex. #5 is correct insofar as it goes, but it misses a few cases. Suppose someone has a commmand file foo with commands bar and baz, both with callbacks. Imagine one callback is drush_foo_set. By #5, the command hook would be "set", but it is really much more desirable to have the command hook be "foo_set".

This means that we have come full circle, and the patch in #1 is actually much more correct than #5, as it actually handles all situations. #5 was clearer, but not fully correct, and therefore a red herring. Sorry.

I committed a slight modification to what was previously accepted; the condition in #1 is always true, so now we simply strip off the "drush_" from the callback whenever the required prefix matches its first N characters, so now the code is a bit simpler, and still correct.

That was sort of my reasoning when I suggested #5 initially. When the callback is something like drush_core_cache_clear, perhaps there is little technical difference between using core_cache_clear or cache_clear as the hook name, so we could choose what we like. cache_clear might be better, hence the suggestion in #5.

Unfortunately, if the callback is drush_COMMANDFILE_verb, where verb is some simple term like create or set, then we must use COMMANDFILE_verb as the hook name, lest two different drush commands pick the same verb for different functions.

I felt that there was no non-ambiguous way to determine if verb is "simple" (likely to conflict), ergo I felt we had to reject #5.

@luchochs: You are absolutely correct. If we follow the convention, then we can avoid ambiguous command names, and in fact we can remove the 'callback' item completely, as drush knows what it "should be". If we always followed the convention, then we could even remove support for the 'callback' item, and the code under discussion could be removed completely.

Given that this code exists explicitly to handle callback function names that do not follow the convention, I think that it is best to be conservative about how much reduction to do. Too much reduction is what caused field_create to conflict with user_create in the first place, after all. (n.b. This particular problem also could have been fixed simply by removing the 'callback' item from user_create, since the function name followed the convention. Fixing it that way, though, would have left this bug around waiting to bite the next set of functions that happened to choose the same verb for their function name...)

Hope that makes sense.

We have definitely migrated away from using explicit callback functions. However, even though there are only a couple still in use in drush core (and those could certainly be fixed), completely removing this feature could potentially break a lot of contrib modules with drush commands. Maybe someday.

More documentation on the drush hook mechanism would be welcome. See drush.api.php.

We still want that explicit callback feature. Sometimes you have a situation where you want a handful of commands with different 'callback arguments' or other variances in hook_command. And they need to all point to same callback. For example, core-readme uses drush_print_file() as its callback and I suspect other commands will do that as well.

I think that is an improvement...

Committed.