I noticed that I got a TRUE status from drush_shell_exec_interactive() even though the executed command ended in error.

I looked up the code and found that what is returned is "the status of being able to start an execution", not the result of the execution itself.

Maybe there is a good reason for wanting to know the first, but it doesn't really make sense to me to do it this way...

CommentFileSizeAuthor
#2 drush-1343488-drush_shell_exec_interactive_returns_wrong.patch1000 byteshelmo

Comments

jdonson’s picture

jdonson commented

Seems useful for exec debug mode, but I see your point.

helmo’s picture

helmo commented
Status: Active » Needs review
StatusFileSize
new drush-1343488-drush_shell_exec_interactive_returns_wrong.patch1000 bytes

Here's a patch to review, I've also added a drush_log() line which logs the actual exit code when using debug mode.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs review » Needs work

Yes, good catch. It would be even better to have interactive mode call drush_shell_proc_open, and then return ($result === 0) ? TRUE : FALSE;.

helmo’s picture

helmo commented
Status: Needs work » Needs review

@greg I just tried.

But I don't understand why the code below exists in drush_shell_proc_open()

if (drush_get_context('DRUSH_VERBOSE') || drush_get_context('DRUSH_SIMULATE')) {
    drush_print("Calling proc_open($cmd);");
    return 0;
}

--debug trigers DRUSH_VERBOSE right? But why should that prevent executing the command?

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs review » Needs work

drush_shell_proc_open is kind of new. The issue you point out in #4 is indeed a bug. 'return 0' should happen on DRUSH_SIMULATE, but not DRUSH_VERBOSE. The drush_print should happen for both.

helmo’s picture

helmo commented

@greg: why drush_print and not drush_log? It seems to me that checking for DRUSH_VERBOSE should be done elsewhere.

Apart from a single white-space error this patch seems ok, drush_shell_proc_open could be a separate issue. Or extra patch here.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Closed (duplicate)

Fine, I'll fix this in #1342940: Simplify documentation for shell alias replacements per #3.