Drush command fails silently in 4.5

cross post. :)

Interesting. Looks like that code exists in drush 4.5. If I print out $all_available_hooks there, I get a huge array of functions that don't exist. Looks like this code is the culprit (notice the else statement):

      if (function_exists($func)) {
        $functions[] = $func;
        $all_available_hooks[] = $func . ' [* Defined in ' . $filename . ']';
        $available_rollbacks[] = $func . '_rollback';
        $completed[] = $func;
        $result = call_user_func_array($func, $args);
        _drush_log_drupal_messages();
        if (drush_get_error() || ($result === FALSE)) {
          $rollback = TRUE;
          // break out of the foreach variations and foreach list
          break 2;
        }
      }
      else {
        $all_available_hooks[] = $func;
      }

In light of this, switching to a bug report, since it sounds like this is supposed to work as I expected it to. Also, if I'm reading things right, it looks like this might be a bug in the 5.x branch as well.

Updating title, and working on a patch now.

Attached patches fix it for me.

Hmm, I tried running the tests against this, and I get errors for all sorts of core commands. For example:

No hook functions were found for pm-download.                                              [warning]
Available drush_invoke() hooks for pm-download:                                            [warning]
drush_archive_pm_download_validate
drush_cache_pm_download_validate
drush_core_pm_download_validate
drush_docs_pm_download_validate
drush_field_pm_download_validate
drush_help_pm_download_validate
drush_image_pm_download_validate
drush_pm_download_validate [* Defined in /usr/local/share/drush/commands/pm/pm.drush.inc]
drush_search_pm_download_validate
drush_site_install_pm_download_validate
drush_sitealias_pm_download_validate
drush_sql_pm_download_validate
drush_test_pm_download_validate
drush_topic_pm_download_validate
drush_upgrade_pm_download_validate
drush_user_pm_download_validate
drush_variable_pm_download_validate
drush_watchdog_pm_download_validate
drush_archive_pre_pm_download
drush_cache_pre_pm_download
drush_core_pre_pm_download
drush_docs_pre_pm_download
drush_field_pre_pm_download
drush_help_pre_pm_download
drush_image_pre_pm_download
drush_pm_pre_pm_download

Also, what is the difference between $functions and $completed? I don't see them being used anywhere else. It seems like $completed should only contain functions that didn't fall into here:

  if (drush_get_error() || ($result === FALSE)) {

But, I don't see that happening anywhere. If I'm reading it right, $functions and $completed would contain the exact same thing.

Yeah, now that I'm using this patch more, it seems there's something else wrong with the logic in here. Here's some output on a drush cc all call:

'all' cache was cleared                                                                  [success]
No hook functions were found for cache-clear.                                            [warning]
Available drush_invoke() hooks for cache-clear:                                          [warning]
drush_archive_cache_clear_validate
drush_cache_clear_validate
drush_core_cache_clear_validate
drush_devel_cache_clear_validate
drush_devel_generate_cache_clear_validate
drush_docs_cache_clear_validate
drush_features_cache_clear_validate
...

Ah, nevermind, I see the difference between $functions and $completed, and I see why my patch isn't working.

Setting to needs work, per moshe: "Lets remove residual code."

Also, I think we should add in a test for this, so this doesn't happen again. I'm going to work on this some today.

Umm.. that kinda sucked. :)

I had assigned this to me, and said I was working on a patch. Then I go to create the patch and get conflicts b/c you already committed a patch. The commit message also only says your name in the credits.

If you're going to work on something that someone has assigned to them, I would at least try to find them in IRC and tell them to stop working on it. :)

Now that I'm getting into the test, I'm thinking the exit code should be 1 when this happens. Setting this to needs work again based on that. I'm working on a patch for this. No really, I am. :P

Ok, here goes.

At a very basic level, why wouldn't the exit code be 1 for a drush command that didn't actually run?

Also, I'm not sure how to write the test without testing on the exit code. $this->getOutput() doesn't have anything in it, so I can't assert anything there.

Yeah, I understand. That's actually what got me here in the first place, though. The command that started this issue is actually a command that calls other commands. If commands call other commands and those commands fail, the outer command should determine how to deal with the failure. I understand that may be a lot to ask, and maybe that's why you say "Drush 6". In my opinion, a command that can't find a callback function is a very big problem, and may be even more so when you are talking about nested commands. If I have an outer custom migration command (which is how this all started), and one of those commands fails, that can be the cause of some SERIOUS problems down the line. :)

But aside from that, can someone give me some tips on how I would write a test for this if exit status is 0?

Greg, thanks for the awesome feedback, and no worries about the earlier commit. You can't win if you have people complaining about committing something too fast, eh? :)

I'll work on this and re-roll the patch.

Changes include setting $return to drush_set_error(), and combining the subsequent warning into the error message.

Any feedback on this latest patch?