Command getting run twice

In HEAD and alpha1, drush shows all watchdogs. crank up your debug level with -v or -d to see em all.

It might have something to do with this:

      if (($oldfunc != $func) && (function_exists($oldfunc))) {
        drush_log(dt("The drush command hook naming conventions have changed; the function !oldfunc must be renamed to !func.  The old function will be called, but this will be removed shortly.", array('!oldfunc' => $oldfunc, '!func' => $func)), "error");
        // TEMPORARY:  Allow the function to be called by its old name.
        $functions[] = $oldfunc;
      }
      if (function_exists($func)) {
        $functions[] = $func;
      }

I don't see how that could be going wrong, but it seems a likely culprit. The code above could be better-written like this:

      if (function_exists($func)) {
        $functions[] = $func;
      }
      else if (($oldfunc != $func) && (function_exists($oldfunc))) {
        drush_log(dt("The drush command hook naming conventions have changed; the function !oldfunc must be renamed to !func.  The old function will be called, but this will be removed shortly.", array('!oldfunc' => $oldfunc, '!func' => $func)), "error");
        // TEMPORARY:  Allow the function to be called by its old name.
        $functions[] = $oldfunc;
      }

If that does the trick, then this one was definitely my fault; sorry. I don't have time to test this right now, but maybe that will do you.

Edit: This has nothing to do with the backwards compatibility code; the conflict is caused by the function name simplification algorithm.

Okay, the names you picked are causing drush's new function name simplification routine to stomp on each other. To rephrase what you said:

If you have mod.drush.inc and mod.sub.drush.inc, then for any command in mod.sub.drush.inc called mod-sub-foo:

Function drush_mod_mod_sub_foo() simplifies to drush_mod_sub_foo().
Function drush_mod_sub_mod_sub_foo simplifies to drush_mod_sub_foo().

Commands in the file mod.sub.drush.inc are supposed to start with mod-sub-, so this problem will be common any time you add drush commands to both a module and its submodule. We can't just say that naming your drush include files like this is deprecated, because the naming convention comes from Drupal module names which are pre-existing.

Attached is a patch with the suggested in_array test (which also removes backwards compatibility for function names), but note the limitation that has been introduced, not here, but by the original hook function simplification patch:

For any module mod.drush.inc where one or more mod.sub.drush.inc also exists, it is not possible for mod to hook into any hooks for functions (appropriately named mod-sub-foo) in mod.sub, because if it does, it will introduce a function name conflict. I think this is okay, because a module typically should not hook its submodule.

The converse situation is not a problem. For any command in mod.drush.inc called mod-bar:

Function drush_mod_mod_bar() simplifies to drush_mod_bar().
Function drush_mod_sub_mod_bar simplifies to drush_mod_sub_mod_bar().

This is good, because it means that a submodule can hook into commands in its parent module, which one might expect would often be desired. I therefore believe that the limitation is reasonable, and the fix is okay.

I'm marking this issue as critical because if the limitation above is not deemed to be reasonable, then the only alternative would be to yank out the hook function simplification and rename drush_pm_list() to drush_pm_pm_list(), etc. I for one am not really inclined to do that, but there should be consensus.

Tough to follow all the mod dancing here, but I agree with " I think this is okay, because a module typically should not hook its submodule.". Seems safe to proceed with this.

Committed.

I can open another issue if you prefer, but...

I was just writing Drush commands for searchAPI module (whose functions are search_api_NAME()). And this problem recurs (even with core Search module disabled).

If I change the functions to drush_searchapi_NAME() from drush_search_api_name(), then things work fine.

Anything we can do here?

What is your drush command file named? Please see sandwich.drush.inc in the examples folder, and also `drush topic docs-commands`. You can also run with -d --show-invoke, and drush will print out the complete list of command functions it is looking to call.

See #1133864: Drush integration for the file. Viewable here as well: http://drupal.org/files/issues/search_api.drush_.inc_.txt

But that copy of the file "fixes" the problem by replacing 'search_api' with 'searchapi'.

Without that fix, here's a sample debug output.

rickard@saverem:~/sandbox/archiveportal/www$ drush sapi-l -d --show-invoke
Bootstrap to phase 0. [0.02 sec, 2.99 MB]                            [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drush() [0.03 sec, 3.28 MB] [bootstrap]
Loading drushrc "/shared/usr/local/drush-palantir/drushrc.php" into "drush" scope. [0.03 sec, 3.29 MB]       [bootstrap]
Loading drushrc "/shared/home/rickard/.drush/drushrc.php" into "home.drush" scope. [0.03 sec, 3.31 MB]       [bootstrap]
Bootstrap to phase 5. [0.07 sec, 9.06 MB]                                                                    [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_root() [0.07 sec, 9.06 MB]                                   [bootstrap]
Initialized Drupal 7.0 root directory at /home/rickard/sandbox/archiveportal/www [0.09 sec, 10.99 MB]           [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_site() [0.09 sec, 11 MB]                                     [bootstrap]
Initialized Drupal site default at sites/default [0.1 sec, 11 MB]                                               [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_configuration() [0.1 sec, 11 MB]                             [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_database() [0.11 sec, 11.01 MB]                              [bootstrap]
Successfully connected to the Drupal database. [0.11 sec, 11.02 MB]                                          [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_full() [0.12 sec, 11.96 MB]                                  [bootstrap]
Bootstrap to phase 6. [0.44 sec, 44.79 MB]                                                                   [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_login() [0.44 sec, 44.79 MB]                                 [bootstrap]
Successfully logged into Drupal as Anonymous (uid=0) [0.45 sec, 45.64 MB]                                    [bootstrap]
Found command: search-api-list (commandfile=search_api) [0.45 sec, 45.64 MB]                                 [bootstrap]
Initializing drush commandfile: drush_make [0.45 sec, 45.64 MB]                                              [bootstrap]
Initializing drush commandfile: drush_make_d_o [0.45 sec, 45.64 MB]                                          [bootstrap]
Initializing drush commandfile: user [0.45 sec, 45.64 MB]                                                    [bootstrap]
 Id  Name                Index               Server   Type  Enabled  Limit 
 1   Default node index  default_node_index  default  node  1        100   

 Id  Name                Index               Server   Type  Enabled  Limit 
 1   Default node index  default_node_index  default  node  1        100   

Available drush_invoke() hooks for search-api-list:                                                          [ok]
drush_core_search_api_list_validate
drush_devel_search_api_list_validate
drush_devel_generate_search_api_list_validate
drush_docs_search_api_list_validate
drush_drush_make_search_api_list_validate
drush_drush_make_d_o_search_api_list_validate
drush_features_search_api_list_validate
drush_field_search_api_list_validate
drush_help_search_api_list_validate
drush_image_search_api_list_validate
drush_libraries_search_api_list_validate
drush_migrate_search_api_list_validate
drush_palantir_search_api_list_validate
drush_palantir.aliases_search_api_list_validate
drush_pm_search_api_list_validate
drush_search_api_list_validate
drush_search_api_list_validate
drush_site_install_search_api_list_validate
drush_sitealias_search_api_list_validate
drush_sql_search_api_list_validate
drush_test_search_api_list_validate
drush_topic_search_api_list_validate
drush_upgrade_search_api_list_validate
drush_user_search_api_list_validate
drush_variable_search_api_list_validate
drush_views_search_api_list_validate
drush_watchdog_search_api_list_validate
drush_zen_search_api_list_validate
drush_core_pre_search_api_list
drush_devel_pre_search_api_list
drush_devel_generate_pre_search_api_list
drush_docs_pre_search_api_list
drush_drush_make_pre_search_api_list
drush_drush_make_d_o_pre_search_api_list
drush_features_pre_search_api_list
drush_field_pre_search_api_list
drush_help_pre_search_api_list
drush_image_pre_search_api_list
drush_libraries_pre_search_api_list
drush_migrate_pre_search_api_list
drush_palantir_pre_search_api_list
drush_palantir.aliases_pre_search_api_list
drush_pm_pre_search_api_list
drush_search_pre_search_api_list
drush_search_api_pre_search_api_list
drush_site_install_pre_search_api_list
drush_sitealias_pre_search_api_list
drush_sql_pre_search_api_list
drush_test_pre_search_api_list
drush_topic_pre_search_api_list
drush_upgrade_pre_search_api_list
drush_user_pre_search_api_list
drush_variable_pre_search_api_list
drush_views_pre_search_api_list
drush_watchdog_pre_search_api_list
drush_zen_pre_search_api_list
drush_core_search_api_list
drush_devel_search_api_list
drush_devel_generate_search_api_list
drush_docs_search_api_list
drush_drush_make_search_api_list
drush_drush_make_d_o_search_api_list
drush_features_search_api_list
drush_field_search_api_list
drush_help_search_api_list
drush_image_search_api_list
drush_libraries_search_api_list
drush_migrate_search_api_list
drush_palantir_search_api_list
drush_palantir.aliases_search_api_list
drush_pm_search_api_list
drush_search_api_list [*]
drush_search_api_list [*]
drush_site_install_search_api_list
drush_sitealias_search_api_list
drush_sql_search_api_list
drush_test_search_api_list
drush_topic_search_api_list
drush_upgrade_search_api_list
drush_user_search_api_list
drush_variable_search_api_list
drush_views_search_api_list
drush_watchdog_search_api_list
drush_zen_search_api_list
drush_core_post_search_api_list
drush_devel_post_search_api_list
drush_devel_generate_post_search_api_list
drush_docs_post_search_api_list
drush_drush_make_post_search_api_list
drush_drush_make_d_o_post_search_api_list
drush_features_post_search_api_list
drush_field_post_search_api_list
drush_help_post_search_api_list
drush_image_post_search_api_list
drush_libraries_post_search_api_list
drush_migrate_post_search_api_list
drush_palantir_post_search_api_list
drush_palantir.aliases_post_search_api_list
drush_pm_post_search_api_list
drush_search_post_search_api_list
drush_search_api_post_search_api_list
drush_site_install_post_search_api_list
drush_sitealias_post_search_api_list
drush_sql_post_search_api_list
drush_test_post_search_api_list
drush_topic_post_search_api_list
drush_upgrade_post_search_api_list
drush_user_post_search_api_list
drush_variable_post_search_api_list
drush_views_post_search_api_list
drush_watchdog_post_search_api_list
drush_zen_post_search_api_list [0.46 sec, 46.07 MB]
Available rollback hooks for search-api-list:                                                                [ok]
drush_search_api_list_rollback
drush_search_api_list_rollback [0.46 sec, 46.07 MB]
Command dispatch complete [0.46 sec, 46.03 MB]                                                                  [notice]
 Timer  Cum (sec)  Count  Avg (msec) 
 page   0.356      1      355.97     

Peak memory usage was 46.16 MB [0.46 sec, 46.03 MB]                                                             [memory]

I see. Even though core search module is inactive, Drush maps a callback to it?

I don't know where that search.drush.inc file is coming from, but there is a known drush naming issue if you have a commandfile named search, then you will get conflicts if you have another commandfile named search_api; this is true for foo and foo_bar for any foo and any bar.

I think a reasonable workaround would be to rename search_api_drush.inc to searchapi_drush.inc. Perhaps this is what you already did in #15? Anyway, I think that will fix it. Reopen if it doesn't.

I renamed the functions, not the filename.

And I totally disagree. This is a crap workaround.

Drush generates the patterns for the callbacks from the name of the file, so please rename your file to searchapi_drush.inc. This will avoid the conflicts w/out requiring the use of the 'callback' item.

If you rename the include file, then how does it get registered with Drush? Is there a missing drush_info hook?

The commandfile does not have to be strictly named after the module; drush will find any *.drush.inc located anywhere inside any enabled module. This means that your foo.module could define bar.drush.inc, which would be odd, but would work. Just remember that drush commandfiles exist outside the framework defined by Drupal modules; if you have a drush commandfile in $HOME/.drush, name it whatever and it will work. The same drush mechanism is used with for commandfiles in Drupal modules, hence the non-requirement for matching names.

No. I have to rename search_api_drush_command() to searchapi_drush_command().

There has to be a better way to avoid this conflict at the drush level.

Right, I see that. And it's now looking for COMMANDFILE_drush_command(). Kinda ugly.

And that fails, because it's back to duplicating the commands.

rickard@saverem:~/sandbox/archiveportal/www$ drush sapi-l -d --show-invoke
Bootstrap to phase 0. [0.02 sec, 2.99 MB]                            [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drush() [0.03 sec, 3.28 MB] [bootstrap]
Loading drushrc "/shared/usr/local/drush-palantir/drushrc.php" into "drush" scope. [0.03 sec, 3.29 MB]       [bootstrap]
Loading drushrc "/shared/home/rickard/.drush/drushrc.php" into "home.drush" scope. [0.03 sec, 3.31 MB]       [bootstrap]
Bootstrap to phase 5. [0.07 sec, 9.06 MB]                                                                    [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_root() [0.08 sec, 9.06 MB]                                   [bootstrap]
Initialized Drupal 7.0 root directory at /home/rickard/sandbox/archiveportal/www [0.09 sec, 10.99 MB]           [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_site() [0.1 sec, 11 MB]                                      [bootstrap]
Initialized Drupal site default at sites/default [0.1 sec, 11 MB]                                               [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_configuration() [0.11 sec, 11 MB]                            [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_database() [0.11 sec, 11.01 MB]                              [bootstrap]
Successfully connected to the Drupal database. [0.11 sec, 11.02 MB]                                          [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_full() [0.12 sec, 11.96 MB]                                  [bootstrap]
Bootstrap to phase 6. [0.45 sec, 44.82 MB]                                                                   [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_login() [0.45 sec, 44.82 MB]                                 [bootstrap]
Successfully logged into Drupal as Anonymous (uid=0) [0.51 sec, 46.67 MB]                                    [bootstrap]
Found command: searchapi-list (commandfile=searchapi) [0.51 sec, 46.66 MB]                                   [bootstrap]
Initializing drush commandfile: drush_make [0.51 sec, 46.67 MB]                                              [bootstrap]
Initializing drush commandfile: drush_make_d_o [0.51 sec, 46.67 MB]                                          [bootstrap]
Initializing drush commandfile: user [0.51 sec, 46.67 MB]                                                    [bootstrap]
 Id  Name                Index               Server   Type  Enabled  Limit 
 1   Default node index  default_node_index  default  node  1        100   

 Id  Name                Index               Server   Type  Enabled  Limit 
 1   Default node index  default_node_index  default  node  1        100   

Available drush_invoke() hooks for searchapi-list:                                                           [ok]
drush_core_searchapi_list_validate
drush_devel_searchapi_list_validate
drush_devel_generate_searchapi_list_validate
drush_docs_searchapi_list_validate
drush_drush_make_searchapi_list_validate
drush_drush_make_d_o_searchapi_list_validate
drush_features_searchapi_list_validate
drush_field_searchapi_list_validate
drush_help_searchapi_list_validate
drush_image_searchapi_list_validate
drush_libraries_searchapi_list_validate
drush_migrate_searchapi_list_validate
drush_palantir_searchapi_list_validate
drush_palantir.aliases_searchapi_list_validate
drush_pm_searchapi_list_validate
drush_searchapi_list_validate
drush_searchapi_list_validate
drush_site_install_searchapi_list_validate
drush_sitealias_searchapi_list_validate
drush_sql_searchapi_list_validate
drush_test_searchapi_list_validate
drush_topic_searchapi_list_validate
drush_upgrade_searchapi_list_validate
drush_user_searchapi_list_validate
drush_variable_searchapi_list_validate
drush_views_searchapi_list_validate
drush_watchdog_searchapi_list_validate
drush_zen_searchapi_list_validate
drush_core_pre_searchapi_list
drush_devel_pre_searchapi_list
drush_devel_generate_pre_searchapi_list
drush_docs_pre_searchapi_list
drush_drush_make_pre_searchapi_list
drush_drush_make_d_o_pre_searchapi_list
drush_features_pre_searchapi_list
drush_field_pre_searchapi_list
drush_help_pre_searchapi_list
drush_image_pre_searchapi_list
drush_libraries_pre_searchapi_list
drush_migrate_pre_searchapi_list
drush_palantir_pre_searchapi_list
drush_palantir.aliases_pre_searchapi_list
drush_pm_pre_searchapi_list
drush_search_pre_searchapi_list
drush_searchapi_pre_searchapi_list
drush_site_install_pre_searchapi_list
drush_sitealias_pre_searchapi_list
drush_sql_pre_searchapi_list
drush_test_pre_searchapi_list
drush_topic_pre_searchapi_list
drush_upgrade_pre_searchapi_list
drush_user_pre_searchapi_list
drush_variable_pre_searchapi_list
drush_views_pre_searchapi_list
drush_watchdog_pre_searchapi_list
drush_zen_pre_searchapi_list
drush_core_searchapi_list
drush_devel_searchapi_list
drush_devel_generate_searchapi_list
drush_docs_searchapi_list
drush_drush_make_searchapi_list
drush_drush_make_d_o_searchapi_list
drush_features_searchapi_list
drush_field_searchapi_list
drush_help_searchapi_list
drush_image_searchapi_list
drush_libraries_searchapi_list
drush_migrate_searchapi_list
drush_palantir_searchapi_list
drush_palantir.aliases_searchapi_list
drush_pm_searchapi_list
drush_searchapi_list [*]
drush_searchapi_list [*]
drush_site_install_searchapi_list
drush_sitealias_searchapi_list
drush_sql_searchapi_list
drush_test_searchapi_list
drush_topic_searchapi_list
drush_upgrade_searchapi_list
drush_user_searchapi_list
drush_variable_searchapi_list
drush_views_searchapi_list
drush_watchdog_searchapi_list
drush_zen_searchapi_list
drush_core_post_searchapi_list
drush_devel_post_searchapi_list
drush_devel_generate_post_searchapi_list
drush_docs_post_searchapi_list
drush_drush_make_post_searchapi_list
drush_drush_make_d_o_post_searchapi_list
drush_features_post_searchapi_list
drush_field_post_searchapi_list
drush_help_post_searchapi_list
drush_image_post_searchapi_list
drush_libraries_post_searchapi_list
drush_migrate_post_searchapi_list
drush_palantir_post_searchapi_list
drush_palantir.aliases_post_searchapi_list
drush_pm_post_searchapi_list
drush_search_post_searchapi_list
drush_searchapi_post_searchapi_list
drush_site_install_post_searchapi_list
drush_sitealias_post_searchapi_list
drush_sql_post_searchapi_list
drush_test_post_searchapi_list
drush_topic_post_searchapi_list
drush_upgrade_post_searchapi_list
drush_user_post_searchapi_list
drush_variable_post_searchapi_list
drush_views_post_searchapi_list
drush_watchdog_post_searchapi_list
drush_zen_post_searchapi_list [0.52 sec, 47.06 MB]
Available rollback hooks for searchapi-list:                                                                 [ok]
drush_searchapi_list_rollback
drush_searchapi_list_rollback [0.52 sec, 47.07 MB]
Command dispatch complete [0.52 sec, 47.02 MB]                                                                  [notice]
 Timer  Cum (sec)  Count  Avg (msec) 
 page   0.419      1      418.53     

Peak memory usage was 47.3 MB [0.53 sec, 47.02 MB]                                                              [memory]

Attached is the version that works "as designed".

Do you have more than one copy of searchapi.drush.inc? Are you using the most recent version of drush-5.x-dev?

This can be fixed with an array_unique() executed smartly in the code.

Sorry, actually we're using 4.4 (on a hosted server). Maybe that's an issue.

Only one file. I keep renaming it and clearing cache.

I could not reproduce your 'called twice' problem, but I did find one bug. Please try out the attached patch, which I rolled against drush-4.4; it's only a one-line patch, though, so you could easily apply it by hand to any drush version if it does not apply cleanly (though it probably will).

Once you have this patch, then the 'search' commandfile will no longer inadvertently conflict with your 'searchapi-*' command names, and you will need to rename your functions to function drush_search_api_searchapi_list() and so on. I'm interested to know if this fix also clears up the 'called twice' bug.

Not quite. But here's a simple patch that removes the duplicate function calls and incorporates your patch.

This was rolled against 'master' but applies to 4.4 as well.

Apologies if I was rude yesterday. I don't know why this issue bothers me so much, but it's very fixable.

Actually, I was just in the process of testing the exact same fix that you included in #32. Yesterday I did not notice that #9, previously committed in #11, was accidentally removed from the code; I saw that only just this morning. This issue is somewhat more complex than implied by #29, but all the same, I'm sorry that I did not notice, despite your hint, that the current code did not match my memory of it. I committed the patch to 5.x HEAD, but I am still vaguely bothered that your particular issue does not seem to match the pattern described in #9. If you are still in a good humor about investigating this issue, please check out HEAD and comment out your in_array test, and see which files are defining your duplicate function (see adjusted debug message in latest code). I am not seeing duplicates from your commandfile on my system, and it seems that you should not get duplicates unless there is another *.drush.inc file involved.

This should be backported to 4.x. Commit is http://drupalcode.org/project/drush.git/commit/3254ba7

Not sure what is different about the latest debug (running git checkout of master). In any case, the only loaded file is:

Found command: search-api-list (commandfile=search_api) [0.45 sec, 44.44 MB]  

Without the in_array() check, I get the function called twice if named:

function drush_search_api_list() {

Which is the proper function name since the module is named search_api.

Changing to:

function drush_searchapi_list() {

Does not create the duplicate error by removing (though creating another potential) namespace issues.

drush_searchapi_list should no longer work in head. You get this function signature by combining "drush_" + search (from drush/commands/core/search.drush.inc) + "_" + searchapi-list (then transliterate "-" to "_"), and then reduce "drush_search_searchapi_list" to "drush_serachapi_list" by replacing "search_search" with "search". In head, that last reduction is now "search_search_" with "search"; "search_search_" no longer matches "drush_search_searchapi_list", so that function name is not reduced & drush_searchapi_list is no longer a valid signature.

Maybe you are not running the latest code. git checkout master will only bring you to the master branch; you need to do a git pull to get the latest code.

The latest code adds the filename that the function signature was generated from when the function name is found. With the latest code, you should only get the signature "drush_search_api_searchapi_list" from "drush_" + searchapi (from searchapi.drush.inc) + "_" + searchapi-list, and searchapi.drush.inc comes is stored in an associative array whose key is "searchapi", so it seems to me impossible that you should have a duplicate function (even w/out the added in_array) unless you have more than one *.drush.inc file participating in the function name generation. The debug from the latest code will show you that.

It should read:

        $all_available_hooks[] = $func . ' [* Defined in ' . $filename . ']';

Hm. I thought I did a pull. Anyway, HEAD now returns this.

drush sapi-l -d --show-invoke
...
drush_pm_search_api_list
drush_search_api_list [* Defined in /shared/home/rickard/drush/commands/core/search.drush.inc]
drush_search_api_list

Which eliminates the duplicate but is a false match.

File is search_api.drush.inc

Command is:

  $items['search-api-list'] = array(
    'description' => 'List all search indexes.',
    'examples' => array(
      'drush searchapi-list',
      'drush sapi-l',
    ),
    'aliases' => array('sapi-l'),
  );
/**
 * List all search indexes.
 */
function drush_search_api_list() {
  // See search_api_list_indexes()
 ...
}

If I rename the function (not the file) to drush_search_api_searchapi_list(), the same error occurs.

drush_search_api_searchapi_list [* Defined in /shared/home/rickard/drush/commands/core/search.drush.inc]
drush_search_api_searchapi_list

I _really_ don't want to name a file searchapi.drush.inc inside the search_api directory.

Subscribing.

Okay, the in_array test from #9 + #32 + #33 does not really do the trick. Try the enclosed patch instead, which removes the in_array and replaces it with a more solid fix that only simplifies command hooks for the commandfile they are defined in.

If your commandfile is drush_search_api.drush.inc and your command is search-api-list, then your command hook would be drush_search_api_list(), and if the core drush commandfile "search" wanted to hook your command, it could by defining drush_search_search_api_list().

As #9 explains, you are never really going to want a commandfile search to hook a commandfile search_api, but as this issue has illustrated, other sorts of conflicts can arise with similarly-named commandfiles, so it's good to resolve this solidly without resorting to in_array, since that solution still leaves you vulnerable to the wrong pre-hook firing, and that wouldn't be any good either.

@agentrickard: If this works for you, I'll commit it.

... and now with the patch.

Um, same as #39 but with the debug statement simplified back to what was intended.

I'll take a look on Monday. Thanks.

Yup, that one did it.

Found command: search-api-list (commandfile=search_api) [0.49 sec, 44.66 MB]     
...
drush_search_search_api_list
drush_search_api_list [* Defined in sites/default/modules/search_api/search_api.drush.inc]
drush_site_install_search_api_list

Committed to 5.x-dev; recommend backporting to 4.x-dev. While this is a rare situation, it is a serious PITA for those it does affect. Note that although this patch does change existing behavior, the behavior change should never affect any correctly-designed commandfile, except for very rare cases such as the situation above, where the search_api commandfile conflicts with the search commandfile. In those rare cases, the behavior is sharply degenerated, and the original author must have worked around the problem or come here with comments (per #18 / #20). This patch will not affect commandfiles that correctly work around the problem.

See #9 and #38 for a full explanation, but in short, I think this should be treated strictly as a bug that should be backported.

I just found out that now, affected by this patch, uinf/upwd commands are not working.

Will investigate a bit later and provide a patch, if needed.

As a matter of fact, none of user commands did work. Here's a patch that fixes this.

Thank you for the patch; that is a good improvement for the user commands, and I'll accept it. However, if I somehow broke the command callback, that needs to be fixed too. Setting to 'needs work' so that I can further investigate.

Yes, reverting http://drupalcode.org/project/drush.git/commit/2272fad49035849c960136953... enables user commands without my patch.

Okay, the problem was that 'callback' functions that do match the expected pattern of drush_commandfile_* (and ergo do participate in the command hook process) were not being correctly identified vis-a-vis the commandfile they were defined in. The key to this fix was to pass the commandfile through to drush_invoke_args like so:

-    call_user_func_array('drush_invoke', array_merge(array($command['command-hook']), $args));
+    _drush_invoke_args($command['command-hook'], $args, $command['commandfile']);

Committed here: http://drupalcode.org/project/drush.git/commit/7c582a0

Recommend backporting to 4.x. #46 not yet committed, although that is an improvement to the user commandfile that could be made independently of this fix.

The commit in #49 broke pm-update and anything else that called drush_invoke_args directly. Fixed in 7e998b5.

I came across the 'calling twice' problem while writing a drush 4.4 module .inc file.

• My module name is 'civicrm_mailchimp.module'
• My drush include file is 'civicrm_mailchimp.drush.inc'
• My drush callback is 'drush_civicrm_mailchimp_cron()'

Except that I have had to rename it to 'drush_civicrmmailchimp_cron()' to prevent it being called twice. I got this trick from #34 and am thinking that it may have something to do with two_part module names connected by an underbar.

I guess one way to fix is to upgrade to the most recent 4.dev or drush 5, but I'm stuck with drush 4.4 for now. And I want to contribute this code soon and am wondering: do I need to rename my module without the underbar so that this bug doesn't affect others? Or is there some other fix I can use in my code?

Thanks for an excellent tool and for this thread that helped me solve - at least temporarily - the issue.

Well, for one more option on how to work around this, you could look at #1133864: Drush integration. I don't endorse this technique, it's kludgy; better to rename your commandfile, in my opinion. If you just can't stand to do that, though, there is that other option.

We really need to do a drush-4.5 release so that we can get this fix into the drush-4 branch (and other fixes too).

This is one of the last of the messier backports I'm trying to put into 4.x before the next release, but it isn't a clean patch.

I'm going to work on this when I can, but if someone wanted to offer a patch against 4.x, it would definitely speed up the process. This issue builds upon #1086314: Incorrect error message when using old-style hook naming., which I can't backport, so any help resolving the differences here would be appreciated.

Ok, I tried to get all these fixes in while retaining the old command hook naming conventions, but I'm not sure it's even possible. If I'm missing something here, please let me know.

Attached patch is more or less what went into 5.x. I've tested that it fixes Dave's original issue and it passes all tests.

I'd very much like Greg or Moshe to review before I commit to 4.x.

Two questions about this patch.

1. Why does it include drush_shell_alias_replace? A mistake? Doesn't seem to be called from the patch.

2. This patch removes backwards compatibility for drush-2 function naming conventions ("drush_foo_foo_cmd" instead of "drush_foo_cmd" for the command "foo-cmd" in the commandfile foo.drush.inc). These were deprecated a year ago in drush-3, so it seems like enough time has gone by to take them out; however, drush-4.5 seems to be an odd time to do it. I could provide a modified version of the patch that preserves this behavior if desired. Any opinions on that before I take the time to do it?

If it just takes an hour or two, we should probably avoid deprecating during a minor release.

Yes, yes, modified version. I agree that we shouldn't remove it if we can help it.

I attempted it but got too confused trying to keep it in and also fix both Dave Reid and agentrickard's issues. I have a feeling Greg can knock this out a lot quicker.

Yeah, this is a pretty tricky issue; fortunately I still remember the foibles from last time. I put the backwards compatibility code back in; it only took a few minutes. Also, I diff'ed #54 against master, and I think it should work just fine, but want to test it some more. Give me an hour or so for testing, and I'll have a patch. I'll probably have time to do the testing later tonight, but maybe it will be tomorrow.

Here's the patially-tested code that I think works, in case you want a preview.

Ahhh, that makes sense. I kept trying to do the $oldfunc bit before $defined_in_commandfile == $commandfile, which was not working.

After testing, I discovered that all versions of drush-4.x, from 4.0 through 4.4, and also including #58 above, do not actually call the old function name, even though the warning message claims that it will be called.

The attached patch preserves this existing behavior ($oldfunc is not called), but the error message is still printed, only now it is adjusted to say the right thing (the old function name will not be called).

Except for the slight adjustment of the aforementioned error message, this patch is the same as #54, which works fine.

Looks good to me. Committed.

And although I'm marking this as fixed, I'm adding a "needs tests" flag since we could really use some here.

Woot! Patch in #60 worked great against v4.4. However, v5.x-dev did NOT work for me.

Oops, someone commented while I was reading.