Skip to content
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

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

Merged
merged 1 commit into from Sep 23, 2018

Conversation

Projects
None yet
4 participants
@mpdude
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

at least tests fail :)

@mpdude

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

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

@mpdude

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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 mpdude:fix-exit-code-for-listeners branch from 79066c8 to b90a3f1 Sep 23, 2018

@chalasr

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

Thank you @mpdude.

@chalasr chalasr merged commit b90a3f1 into symfony:2.8 Sep 23, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

chalasr added a commit that referenced this pull request Sep 23, 2018

bug #28545 [Console] Send the right exit code to console.terminate li…
…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 mpdude:fix-exit-code-for-listeners branch Sep 23, 2018

This was referenced Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.