Log recoverable an fatal error an stop the execution of the script.

[mdeletter]

Fixed handling of recoverable fatal errors in the same way as Drupal do.

• Error will be logged as an error instead of a notice.
• Execution of the script will be stopped.

[mdeletter]

Due to the fixed error handling, other (un-related) errors are now blocking the build.

[weitzman]

A seldom used feature of Drush is that commands can implement rollback functions that happen if a command logs an error. I suspect thats why we keep running after such an error.

[mdeletter]

So I need to implement a rollback hook for each drush command?
Are the rollback hooks also triggered by these errors (E_RECOVERABLE_ERROR)?

But still, this kind of errors need to be logged as 'error' instead of a notice, correct ?
Drupal does also stop executing of the script. See #1459 for additional information.

This is a serious problem, take a look a this example code:

function drush_yourmodule_testerror() {

  $baz = new Baz();

  // Inject the wrong class
  $baz->needs(new Bar());


  echo "please fail" . PHP_EOL;
}

class Foo
{
  public function doStuff() {
    echo "FOO" . PHP_EOL;
  }
}

class Bar
{
  public function doStuff() {
    echo "BAR" . PHP_EOL;
  }
}

class Baz
{
  public function needs(Foo $foo) {
    $foo->doStuff();
  }
}

To prevent serious issues please stop/breaking out when this errors occurred.
Btw, without a 'custom' error handler in plain PHP the execution of the script is also stopped.

Hopefully this comment clarifies this fix.

[carlwiedemann]

Friendly bump.

This is a very serious error, because functions that have type hints that happen to be called in drush aren't throwing exceptions.

See https://gist.github.com/c4rl/0ebff1f9695fb95e5c8c

[weitzman]

This fix proposed here is goes against the design of Drush IMO. When a recoverable error occurs, run the rollback function(s) that exist for the current command.

[carlwiedemann]

Howdy @weitzman !

If Drush does not fatally exit, then a function invoked via Drush possibly contradicts how Drupal (as added in as added in https://www.drupal.org/node/304924#comment-1043193) and PHP by default normally behave.

Without inspecting logs throughly for every Drush-invoked command, one won't know when these errors occur. Every Drush command then potentially needs overly verbose logging, manual type-checking (which can only check local variables, not those possibly created in nested Drupal code the command invokes), and potentially a rollback function which must somehow determine the proper steps to rollback whatever was done. This rollback could be cumbersome depending on the nature of the method body that required the type declaration because that method body likely has little logging/validation/safeguard against bad types since it was relying on type declaration in the first place.

Was there discussion as to making Drush's error handler behave this way on purpose? I'd be interested in seeing it. :)

[weitzman]

No, there isn't any discussion online since the rollback has been in Drush since the very beginning (2.x I think).

[carlwiedemann]

So the "design of Drush" was to ignore type declaration fatals? I think that's buggy.

If you're not convinced, would it make sense to at least implement this as an option that could be passed? I'd prefer it be a default option, but I'd be willing to accept non-default as a compromise.

# Defaults to fatal on type mis-match, pass option to ignore them.
$ drush @alias foo-bar --ignore-recoverable-fatals

# Non-default to fatal on type mis-match, pass option to show/invoke them.
$ drush @alias foo-bar --allow-recoverable-fatals

[greggles]

A seldom used feature of Drush is that commands can implement rollback functions that happen if a command logs an error.

I think that if a feature is seldom used and contrary to the way people reasonably expect the code to work (halting processing on an E_RECOVERABLE_ERROR, just like Drupal core) then it makes sense to make the seldom-used feature be possible, but not default. @C4rL's proposal about making it a command line flag makes sense to me and seems easy to implement if acceptable.

[mdeletter]

It's possible to influence the error handling of recoverable errors with the following option:

/**
 * Specify the error handling of recoverable errors (E_RECOVERABLE_ERROR).
 * Defaults to "ignore". Will stop further execution of Drush, when set to "halt"
 * (same behaviour as Drupal core).
 */
# $options['recoverable-errors-handling'] = 'halt';

@weitzman is this option acceptable ?

[greg-1-anderson]

My thoughts:

1. In Drush core, the only function that has ever implemented rollback is pm-updatecode. I don't think that I've ever seen a contrib module use it, but I have never surveyed for this, so it might happen somewhere.
2. Rollback happens when any command hook returns FALSE or calls drush_set_error. I have not tested, but I don't think that pm-updatecode depends on the current behavior of the Drush error handler in order to roll back after the kinds of errors that usually cause rollbacks in that command (can't untar a file, etc).
3. The rollback function pre-dates my involvement with Drush, which started late in the Drush 2.x cycle; however, it is my general impression that, when the current behavior was implemented ("let it keep running"), it was generally thought that this would be useful for rollbacks, but I don't think that it is actually necessary or correct to ignore errors this way. It's also likely that it simply was not considered in this context at all; hard to say at this point.
4. The one instance that I can think of where we would want to continue execution after an E_RECOVERABLE_ERROR is when Drush is called with --backend specified. If you use your local Drush to call a remote Drush, and the remote Drush suddenly terminated, then the calling Drush would not receive any results other than the status result code. This is not necessarily desirable.

I think it would be generally an improvement to cause Drush to halt when serious errors are logged. Our error handler / shutdown function should be modified to test for backend mode, and emit the appropriate data structure to inform the caller of the fate of the aborted script. I don't think it is necessary to provide a flag to turn this behavior on and off; we should be able to just do the right thing. I don't think it is even necessary to remove pm-updatecode or the rollback feature, although some updates to the handling of error conditions mght be needed. Some testing of the failure cases of pm-updatecode (and ideally some unit tests that simulate different errors to test this) before we change the behavior would be prudent, as it's entirely possible that I might be overlooking something.

[greg-1-anderson]

According to http://php.net/manual/en/function.set-error-handler.php, E_ERROR, E_PARSE, E_CORE_ERROR, and E_COMPILE_ERROR cannot be handled by a user error handler, so I'm not sure why we are testing for these.

[greg-1-anderson]

The shutdown handler already handles cleaning up in backend mode; my inclination is that this PR should be merged as-is.

[weitzman]

Would be great if @ergonlogic, @cweagans or others from Aegir team commented on how important rollback is to them. Thats the main user of this API. The PR proposed here changes nothing until an option is set. But I'm curious if we can start ripping out the rollback code.

[ergonlogic]

As far as I'm concerned, E_RECOVERABLE_ERROR should definitely be logged as an error. It was only added in PHP 5.2.0, which this code pre-dates. I'm pretty sure that the fact that we don't handle it properly yet is just an oversight, and not a design decision regarding rollbacks.

That said, I'm not sure what the benefit would be to immediately halting execution on (only) such errors. Assuming it is logged as an error, then Drush already shouldn't proceed any further with the execution of the command. Blocking rollback hooks and other shutdown behaviour doesn't seem warranted. Bear in mind that if no rollback hooks were defined for the command, then none will be called (for the command in question).

Aegir uses rollbacks quite extensively. They're a very convenient way to clean up artefacts from failed operations.

[greg-1-anderson]

I think we should get rid of the flag, or at least make it default to on. I don't think we're too likely to see these sorts of errors in the wild if it "breaks on the bench". If we ignore them, then we likely will start to see this sort of thing show up as a runtime error, and cause rollback problems.

It's not good to let execution continue after this sort of a coding error. I don't think it is necessary to disable rollbacks entirely if we stop execution when one of these is encountered. Heck, we could even try to run the rollback handlers conditionally on this error, if we really did think it might happen in the wild.

[weitzman]

Deferring to @greg-1-anderson on this one.

[mdeletter]

@greg-1-anderson should I remove the flag and revert it to the orginal PR?

[greg-1-anderson]

@mdeletter: Perhaps switching the flag to default to halt would be the most conservative option. Define --halt-on-error (the default), and rely on --halt-on-error=0 to continue execution on a recoverable error. (n.b. Drush will now automatically convert --no-halt-on-error to --halt-on-error=0)

[greg-1-anderson]

This flag also needs to be added to the list of global options in includes/drush.inc.

[mdeletter]

I am not completely sure if the propagate setting is needed.

[greg-1-anderson]

'propagate' controls whether or not this option is passed on to subprocesses, when Drush exec's another instance of Drush.

[mdeletter]

That's what we need.

[mdeletter]

Remove the unrelated bugfix, update upstream and rebased the feature branch.

[mdeletter]

@greg-1-anderson , please review.

[greg-1-anderson]

propagate-cli-value would be better here. With propagate, you are more likely to cause Drush to pass this value to a remote Drush, which might be older and not recognize the setting. With propagate-cli-value, the option is only included if it appears on the command line. Presumably, the value should be read again by the subprocess, if it came from a configuration file in the first place.

[greg-1-anderson]

Tested; seems right to me. Merging.

[carlwiedemann]

🎉