drush pm:security -n wrong status-code

Describe the bug
drush returns a statuscode indicating that there is an error, although there is none.
This wil terminate good scripts tasked with updating a drupal-site in the moment when they need to run flawlessly.

To Reproduce
Install a Drupal-Site expecting a core-update e.g. 8.6.1.
Run drush pm:security -n ; echo $? .

Expected behavior
I expect to see a non-zero status-code only when there is an error causing drush not to perform the task requested, because of varying conditions.
Running drush pm:security -n should not return non-zero when there is an update as getting an update is expected behaviour and not an error.

Actual behavior
A script written to stop when an error occures as to not do any damage will not be able to perform an update.
e.g.

`
function handleErrors() {
$statusCode=$?;
......
exit $statusCode;
}
trap handleErrors ERR

.....
......drush pm:security -n --pipe......
..... update-action never executed.........
`

Workaround
Disable Trap with trap - ERR.
This has the drawback that you will not be able to respond to an actual error happening.

System Configuration

Linux
Drupal 8.6.1
Drush Commandline Tool 9.2.3
PHP 71

Additional information
None.

[weitzman]

I think current behavior is also fine, but lets keep this open for a while to collect opinions/feedback.

[greg-1-anderson]

I don't quite recall the history, but this is perhaps by design.

We could consider introducing an option to allow the client to override the default error reporting strategy. We shouldn't change the default until a major release, as that would be a breaking change.

I don't quite recall the history, but this is perhaps by design.

We could consider introducing an option to allow the client to override the default error reporting strategy. We shouldn't change the default until a major release, as that would be a breaking change.

That would be awesome, because working with it in a bash-like-manner is not really reliable that way.
As there is a corner-case popping up all of the sudden and handling it completely separate from the rest just because of the status-code makes it all the more complicated.

//edit I can also reproduce it for D7, so a backport to older versions of Drush would be much appreciated.

[greg-1-anderson]

A pull request to kick things off would be welcome.

[weitzman]

I'm not so sure that this is something I'd like to make configurable. I'm hesitant to add switches for core plumbing.

Also, the exit code of this command was deliberately chosen - #3087. Its convenient for CI to work this way.

I'm not so sure that this is something I'd like to make configurable. I'm hesitant to add switches for core plumbing.

Also, the exit code of this command was deliberately chosen - #3087. Its convenient for CI to work this way.

Hi, not sure about this but apparently I cannot find anything in regards to the exit-code.
Only a question about how to set it on returning a RowsOfFields() and in a commit-message.
stating that noone expects a exit-code of 1.
Now I do not know that statement is in favor or against status-code 1.

If it is deliberately chosen then I would like to know about the reasons/use-case for this, because on the CLI every status-code that is non-zero is an error, and finding a core-update cannot possibly be an error.Also note that the first thing you would get to update(regularly) are modules and looking at drush core is treated like a module in the sense of an update. So you test it against modules you roll it out and then all of the sudden drush shows unexpected behavior.
In that light it sounds like a bug to me.

As for the switches for core-plumming:
core-plumming indicates that it is tied in with the internal workings of drupal.
For D8 it is said (in the ticket referenced) that drush will not be the tool to be used for updates anymore, and in D7 as in D8 you treat core as a module as far as updates and listings are concerned.
So viewing Drupal as a package to be updated(although being the most important one) is not related to core but to update-operations only.
So can you help me understand this?

@greg-1-anderson
As so often,, one has to make time for this, so I currently don't have time,but if I find the time, I first like to make sure that, if done properly it will be used, or if done not so properly that s.o. else can build on it and the result will then be used.

[greg-1-anderson]

I'm not so sure that this is something I'd like to make configurable. I'm hesitant to add switches for core plumbing.

There was precedence for this in the past. Drush 8 would force a status code of 1 iff the status code was 0, --strict=2 was selected, and there was at least one [warning] emitted in the logs. This feature was intended for use with git bisect. It did not move forward to Drush 9.

on the CLI every status-code that is non-zero is an error

I disagree that this is necessarily the case. From the Wikipedia page on the exit status:

The specific set of codes returned is unique to the program that sets it. Typically it indicates success or failure. The value of the code returned by the function or program may indicate a specific cause of failure.

It is not required that a cli program return a zero status code for non-error conditions. Having pm:security return a non-zero status allows scripts such as:

#!/bin/bash

set -x

# Do some things that might fail

set +x
drush pm:security -n || ... act on security release

set -x

# Continue with the script

That said, the current behavior is admittedly inconvenient for folks who use trap. I guess as a workaround you could have a Bash function to turn on the trap so that you can turn it off and then turn it back on consistently as needed.

if I find the time, I first like to make sure that, if done properly it will be used

Makes sense. @weitzman will have to agree for this to move forward. I am still inclined to think it's a good idea, but I care 3 out of 10.

Agreed let's wait on a decision.
One thing I cannot help but to point out though is(in the wikipedia-article linked above):
Exit-status is a broader topic, and further down the article itself says that everything non-zero is to be considered an error in the context of a shell.

Shell scripts typically execute commands and capture their exit statuses.
For the shell’s purposes, a command which exits with a zero exit status has succeeded. A nonzero exit status indicates failure. This seemingly counter-intuitive scheme is used so there is one well-defined way to indicate success and a variety of ways to indicate various failure modes. When a command is terminated by a signal whose number is N, a shell sets the variable $? to a value greater than 128. Most shells use 128+N, while ksh93 uses 256+N.
If a command is not found, the shell should return a status of 127. If a command is found but is not executable, the return status should be 126.[2] Note that this is not the case for all shells.
If a command fails because of an error during expansion or redirection, the exit status is greater than zero.

Looking at man bash it says the same thing.
While I agree that a program can return whatever it wants as it is in the domain of the program, considering the environment it runs in/is mostly used in, shouldn't be left out.

However leaving default behaviour as is, even a switch that makes drush behave in a way expected by most/all(?) shell-commands would be very nice, and I would be happy to try my hands on it.

// edit
A little update.
I appears that the --pipe-switch makes it return 0 when a core-update is found, which is expected
That is a bit confusing, when I run pipe I expect to see the shell-code 0 in our case as described in this issue
I run the same command without --pipe so I can get the message for later delivery (together with the -n switch) and this returns non-zero when a core-update is present.

[weitzman]

I noticed that we already have a config for this. See

drush/includes/batch.inc

Line 233 in 79d169f

$halt_on_error = \Drush\Drush::config()->get('runtime.php.halt-on-error', TRUE);

. I'm fine with switching exit code based on that config.

[greg-1-anderson]

I'm not sure that it would work well to use that value after all.

runtime.php.halt-on-error changes the way that PHP handles certain logical errors when they are encountered. The usual expectation is that execution should stop on such errors. However, back when we defined this constant, there were still a lot of Drupal 7 contrib modules that would abort out with this set, and run fine without it.

In any event, I am unsure that any given user's preference for how runtime.php.halt-on-error should behave would necessarily be the same as their preference for the exit code value for pm-security. If we add a preference here, I think that it should be a separate setting.

I don't think there's any harm in adding another configuration setting for this.

[weitzman]

I notice that composer outdated has an option for this but the name collides with one we already use - "strict".

      --strict                   Return a non-zero exit code when there are outdated packages