Skip to content

[Console] Send the right exit code to console.terminate listeners #28545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 23, 2018

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Sep 21, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

When a Console command throws an exception without a status code, Application::run() takes care of setting the exit code to 1 when the exception does not provide a code itself.

This happens slightly too late, as console.terminate event listeners that are called from within Application::doRunCommand() are given the plain exeception code, before this conversion.

The result is that console.* event listeners that you might be using to log exit code e. g. for cron jobs will see a 0 code instead of the real value used to terminate the script.

Todo:

  • Make sure we've got tests covering this, i. e. do not mock out doRunCommand().

@mpdude
Copy link
Contributor Author

mpdude commented Sep 21, 2018

3.4 does not have this bug, most likely due to #22441

@mpdude mpdude changed the title [Console] RFC: Send the right exit code to console.terminate listeners [Console] Send the right exit code to console.terminate listeners Sep 21, 2018
@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Sep 22, 2018
@nicolas-grekas
Copy link
Member

at least tests fail :)

@mpdude
Copy link
Contributor Author

mpdude commented Sep 22, 2018

@nicolas-grekas Will work on tests if you agree we should fix it on 2.8.

@nicolas-grekas
Copy link
Member

We might also just wait for 2.8 to fade out if you don't have time to work on this :)

@mpdude
Copy link
Contributor Author

mpdude commented Sep 22, 2018

I was expecting this :-)

Personally, I‘ll probably have to deal with 2.8 for another year so I‘d be glad to have this fixed.

@mpdude
Copy link
Contributor Author

mpdude commented Sep 22, 2018

@nicolas-grekas Tests added – does it make sense to omit the Exception type hint to be PHP 5 and 7 compatible (no \Throwable on PHP 5 but possibly have to handle \Error on PHP7)?

@chalasr chalasr force-pushed the fix-exit-code-for-listeners branch from 79066c8 to b90a3f1 Compare September 23, 2018 10:04
@chalasr
Copy link
Member

chalasr commented Sep 23, 2018

Thank you @mpdude.

@chalasr chalasr merged commit b90a3f1 into symfony:2.8 Sep 23, 2018
chalasr pushed a commit that referenced this pull request Sep 23, 2018
…steners (mpdude)

This PR was squashed before being merged into the 2.8 branch (closes #28545).

Discussion
----------

[Console] Send the right exit code to console.terminate listeners

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

When a Console command throws an exception without a status code, `Application::run()` takes care of setting the exit code to `1` when the exception does not provide a code itself.

This happens slightly too late, as `console.terminate` event listeners that are called from within `Application::doRunCommand()` are given the plain exeception code, before this conversion.

The result is that `console.*` event listeners that you might be using to log exit code e. g. for cron jobs will see a `0` code instead of the real value used to terminate the script.

***Todo:***

- [x] Make sure we've got tests covering this, i. e. do not mock out `doRunCommand()`.

Commits
-------

b90a3f1 [Console] Send the right exit code to console.terminate listeners
@mpdude mpdude deleted the fix-exit-code-for-listeners branch September 23, 2018 17:21
This was referenced Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants