Remove shutdown handling from Drush

[weitzman]

In order to simplify Drush, and integrate better with #5823, we remove our shutdown API.

I'm inclined to start a Drush 13 branch with this PR and Prompts.

[greg-1-anderson]

This will make it much harder for folks to diagnose misplaced exit calls, e.g. redirect logic in settings.php not guarded from cli use, etc.; however, I understand. I wonder if we could provide a "Drush shutdown handler extension" that folks could add when debugging? To that end, I wonder if we should leave setCompleted() in place for now?

I agree about starting a Drush 13 branch for this.

[weitzman]

The behavior change for those folks is that they no longer see "Drush terminated abnormally". You think thats very helpful? A redirect during a Cli request is a fatal error and the exit code will be a failure. The php fatal should be printed in most cases. Or I'm mistaken? How is it much harder to debug the redirect case without this code?

[greg-1-anderson]

Current behavior, without this PR, after adding exit(0) to settings.php, to simulate bad redirect logic:

$ drush status
 [warning] Drush command terminated abnormally.
$ echo $?
1

With this PR:

$drush status
$ echo $?
0

The main issue here is with support; it's harder for a remote support agent to know why Drush is not doing anything.

The user can run again with --debug:

$ drush status --debug
 [preflight] Config paths: /Users/ga/Code/open-source/drush/drush.yml,/Users/ga/Code/open-source/drush/drush
 [preflight] Alias paths: /Users/ga/Code/open-source/drush/sut/drush/sites,/Users/ga/Code/open-source/drush/drush/sites
 [preflight] Commandfile search paths: /Users/ga/Code/open-source/drush/src,/Users/ga/Code/open-source/drush/sut/drush,/Users/ga/Code/open-source/drush/sut/sites/all/drush
 [info] Starting bootstrap to max [0.77 sec, 11.41 MB]
 [debug] Trying to bootstrap as far as we can [0.77 sec, 11.41 MB]
 [info] Drush bootstrap phase: bootstrapDrupalRoot() [0.77 sec, 11.41 MB]
 [info] Change working directory to /Users/ga/Code/open-source/drush/sut [0.77 sec, 11.41 MB]
 [info] Initialized Drupal 10.1.4 root directory at /Users/ga/Code/open-source/drush/sut [0.77 sec, 11.46 MB]
 [info] Drush bootstrap phase: bootstrapDrupalSite() [0.77 sec, 11.91 MB]
 [debug] Could not find a Drush config file at sites/default/drush.yml. [0.78 sec, 12.07 MB]
 [info] Initialized Drupal site default at sites/default [0.78 sec, 12.07 MB]
 [info] Drush bootstrap phase: bootstrapDrupalConfiguration() [0.78 sec, 12.07 MB]

We could document in the FAQ that a sudden silent exit after bootstrapDrupalConfiguration() is likely indicative of bad redirect logic in settings.php. Bad redirect logic in modules might have different symptoms, though.

Perhaps this is sufficient. I guess the other question, though, is what additional effects are caused when mixing #5823 with the shutdown handler? It might be helpful for ISPs to install their own shutdown handler when running Drush, if the side effects of doing so wouldn't be too great. It would be useful to be able to maintain the error message for support purposes, and running interactive commands in On-Server Development mode is less common.

[weitzman]

I'm not sure that exit(0) test says much. Nobody does that. I have also seen sites using redirects like you said. I added a header() call to settings.php and it was did nothing in my test. I guess PHP CLI ignores this function nowadays.

An ISP can certainly add own shutdown handler via a custom commandfile.

[greg-1-anderson]

I added a header() call to settings.php and it was did nothing in my test.

I believe the pattern a lot of folks use involves calling exit(0) immediately after header() to avoid further processing. Pantheon used to have sample code recommending this in their docs, but it doesn't seem to be there any longer.

An ISP can certainly add own shutdown handler via a custom commandfile.

Yes, and this would work even better if we retained the setCompleted() calls for now, as I suggested above.

[weitzman]

We would have to keep too much of our shutdown api. Its not just setCompleted().

I dont think there are unusual difficult side effects for ISPs who add own shutdown handler. They will receive UserAbortException cases as well as your exit() example. The exit(1) is what Laravel Prompts does after a user cancels the prompt.

[greg-1-anderson]

We would have to keep too much of our shutdown api. Its not just setCompleted().

I am suggesting keeping the setCompleted function implementation itself, and the three calls that we make to it.

The exit(1) is what Laravel Prompts does after a user cancels the prompt.

I am not entirely opposed to removing the shutdown handler as a simplification / modernization move, but it makes me sad that the reason we're doing it because the Laravel code is calling exit instead of throwing an exception. I guess they want it to be "easy", but this also makes their code untestable. I guess they don't care about testing user cancelled events. Still makes me sad.

I'd like to suggest that we just make the shutdown handler quiet in Drush 12 if the exit code is non-zero (since presumably any code that calls exit(1) should usually print an error message itself before quitting), and just think about removing it completely in Drush 13 (don't rush to make the new major release for this). I don't know if that passes the backwards-compatible sniff test for Drush 12, though.

[greg-1-anderson]

I have a couple comments on what I would prefer, but you have my 👍 to move forward with modernization if you don't like the alternatives.

[weitzman]

I am suggesting keeping the setCompleted function implementation itself, and the three calls that we make to it.

Done. Lets sit with this for a bit and see if more changes are desired.

[greg-1-anderson]

Is this PR targeting Drush 12 or Drush 13?

[greg-1-anderson]

If we remove the shutdown handler in Drush 12, then it might be hard to know whether an external shutdown handler is needed. If this PR targets Drush 13, then maybe the comment should say v14?

[weitzman]

I'm still leaning towards opening a 13 branch now. I dont feel strongly about it. Anyone have a preference? The Prompts PR has a minor backward compat issue with the text() method now being a prompt.

[greg-1-anderson]

I prefer putting this in a Drush 13 branch, because otherwise it would be hard to know whether to add an auxiliary shutdown handler or not.

[weitzman]

Prompts no longer needs this removal so lets think about this PR a bit more. No rush.