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][FrameworkBundle] Log console exceptions #21003

Merged
merged 2 commits into from Jan 23, 2017

Conversation

Projects
None yet
7 participants
@chalasr
Copy link
Member

commented Dec 20, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10895
License MIT
Doc PR symfony/symfony-docs#7373

Continues #19382, fixing some issues including:

  • ability to display the input string for any InputInterface implementation (cast to string if possible, use the command name otherwise)
  • if the input can be casted as string, cleanup the result (from command "'command:name' --foo=bar" to command "command:name --foo=bar")
  • made ExceptionLister::$logger private instead of protected
  • changed methods name from onKernel* to onConsole* (e.g. onConsoleException) and removed unnecessary doc blocks
  • Added more tests

Log for an exception:

[2016-12-22 00:34:42] app.ERROR: Exception thrown while running command: "cache:clear -vvv". Message: "An error occured!" {"exception":"[object] (RuntimeException(code: 0): An error occured! at /Volumes/HD/Sites/tests/sf-demo-3.2/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:61)","command":"cache:clear -vvv","message":"An error occured!"} []

@chalasr chalasr changed the title Automatic console exception logging [Console][FrameworkBundle] Log console exceptions Dec 20, 2016

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 2 times, most recently from 6824d97 to 07a2966 Dec 20, 2016

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 26, 2016

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 2 times, most recently from f0f0717 to 9153eb1 Jan 6, 2017

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch from 9153eb1 to 3e55c69 Jan 15, 2017

<service id="console.exception_listener" class="Symfony\Component\Console\EventListener\ExceptionListener">
<tag name="kernel.event_subscriber" />
<argument type="service" id="logger" on-invalid="null" />
</service>

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jan 15, 2017

Member

I think it could be useful to add a channel. for example console.

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 15, 2017

Author Member

It makes sense, console channel added

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch from 3e55c69 to 95dd011 Jan 15, 2017

@lyrixx

This comment has been minimized.

Copy link
Member

commented Jan 15, 2017

👍

@@ -27,7 +27,7 @@
"psr/log": "~1.0"
},
"suggest": {
"symfony/event-dispatcher": "",
"symfony/event-dispatcher": "For using the console events and exception listener",

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 15, 2017

Member

I would not change this.

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 15, 2017

Author Member

Reverted

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 2 times, most recently from 518a4d3 to b175619 Jan 15, 2017

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

What about adding an entry to the changelog?


<services>

<service id="console.exception_listener" class="Symfony\Component\Console\EventListener\ExceptionListener">

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 17, 2017

Member

could be private

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 17, 2017

Author Member

Since it is an event listener, it needs #21312 to be merged first (see https://travis-ci.org/symfony/symfony/jobs/192665057#L2971)

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
* Console exception listener.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 17, 2017

Member

this description doesn't add much value IMO

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 17, 2017

Author Member

I agree, removed

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 3 times, most recently from 9ea7465 to 735f100 Jan 17, 2017

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

Changelog updated

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch from 735f100 to 3399e7f Jan 18, 2017

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@xabbuh The listener is private now

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

The event-dispatcher does not allow private listeners before 3.3... Can we let it public, or shall I update the framework's event-dispatcher requirement to ~3.3?
Edit: Reverted to public, it's not worth it IMHO.

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 2 times, most recently from 3981130 to 6217caa Jan 18, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

@chalasr I think most people upgrade all Symfony components from one version to the next, especially as many just use symfony/symfony, which does not provide any flexibility. Having broader constraints help with compatibility with other libs using Symfony though.

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

Makes sense, thank you for the hint @fabpot

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

👍

Status: Reviewed

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

ping @fabpot

@@ -81,6 +81,7 @@ public function load(array $configs, ContainerBuilder $container)
}
$loader->load('fragment_renderer.xml');
$loader->load('console.xml');

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2017

Member

I would only registered this when the Console component is actually available.

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 23, 2017

Author Member

Fixed

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch 2 times, most recently from 9ff4b70 to c5e671f Jan 23, 2017

@@ -82,6 +83,10 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('fragment_renderer.xml');
if (class_exists(Application::class)) {

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2017

Member

You should add a FileExistenceResource resource so that if the console component is installed, the cache is automatically rebuilt.

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 23, 2017

Author Member

Sorry but I don't understand. We are checking a class existence here, FileExistenceResource is about the filesystem, right?

This comment has been minimized.

Copy link
@chalasr

chalasr Jan 23, 2017

Author Member

Got it! You mean ClassExistenceResource :) done

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

Build failure is unrelated.

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch from c5e671f to 156e264 Jan 23, 2017

Add Console ExceptionListener
Handle non string-castable inputs

Cleanup input for display

Naming changes

InputInterface doesnt have a toString()

Logger must be private

Remove useless doc blocks

Tweak tests

@chalasr chalasr force-pushed the chalasr:automatic-console-exception-logging branch from 156e264 to 919041c Jan 23, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 23, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 919041c into symfony:master Jan 23, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 23, 2017

feature #21003 [Console][FrameworkBundle] Log console exceptions (jam…
…eshalsall, chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Console][FrameworkBundle] Log console exceptions

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10895
| License       | MIT
| Doc PR        | symfony/symfony-docs#7373

Continues #19382, fixing some issues including:
- ability to display the input string for any `InputInterface` implementation (cast to string if possible, use the command name otherwise)
- if the input can be casted as string, cleanup the result (from `command "'command:name' --foo=bar" ` to `command "command:name --foo=bar"`)
- made `ExceptionLister::$logger` private instead of protected
-  changed methods name from `onKernel*` to `onConsole*` (e.g. `onConsoleException`) and removed unnecessary doc blocks
- Added more tests

Log for an exception:

> [2016-12-22 00:34:42] app.ERROR: Exception thrown while running command: "cache:clear -vvv". Message: "An error occured!" {"exception":"[object] (RuntimeException(code: 0): An error occured! at /Volumes/HD/Sites/tests/sf-demo-3.2/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:61)","command":"cache:clear -vvv","message":"An error occured!"} []

Commits
-------

919041c Add Console ExceptionListener
9896547 Add basic support for automatic console exception logging

@chalasr chalasr deleted the chalasr:automatic-console-exception-logging branch Jan 23, 2017

fabpot added a commit to symfony/symfony-standard that referenced this pull request Feb 1, 2017

minor #1044 Don't log console errors in console output (chalasr)
This PR was merged into the 3.3-dev branch.

Discussion
----------

Don't log console errors in console output

Since symfony/symfony#21003, console errors are logged, and since the `console` channel is not ignored by the monolog console handler, the log is written to stderr.

IMHO it's useless since the ran command already write the message into, it's just having two traces for the same error, which doesn't make sense to me.

I propose to fix that.

__Before__
![before](http://image.prntscr.com/image/16a2773542ee4a0caeaf626b119d5c18.png)

__After__
![after](http://image.prntscr.com/image/facb6641ffdf4df5af90d203c6327fb5.png)

If one wants the trace, then one can run the command in verbose mode or look at its log file.

Commits
-------

2bf1249 Don't log console errors in console output

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Feb 3, 2017

feature #7373 [Console] Update the doc for automatic console logging …
…(chalasr)

This PR was merged into the master branch.

Discussion
----------

[Console] Update the doc for automatic console logging

In symfony/symfony#21003 we are going to provide automatic error/exception logging for the console, similar to what is done in an HTTP context.

In the docs, there is a whole chapter dedicated to this topic. Right now I just removed it, maybe it should be done differently or mentioned somewhere that error/exceptions are automatically logged on the `console` channel?

Thanks

Commits
-------

76d2c91 Update the doc for automatic console logging

@nicolas-grekas nicolas-grekas modified the milestone: 3.x Mar 24, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

chalasr added a commit that referenced this pull request Aug 8, 2017

bug #23828 [Console] Log exit codes as debug messages instead of erro…
…rs (haroldiedema)

This PR was merged into the 3.3 branch.

Discussion
----------

[Console] Log exit codes as debug messages instead of errors

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

Fixes PR #21003

This patch stops logging `exit_codes != 0` to _error_ and logs it to _debug_ instead, since it's not an error. The console application exited without uncaught exceptions, and the console application deliberately returned a specific exit code, therefore it shouldn't be logged as an error.

A valid use case would be to let a caller script in bash (the script spawning the console command) know what action to take based on the exit code. More specifically, exit codes other than 1, 2, 127+n isn't necessarily considered an error.

Monolog is hooked to our mailbox, so we can see what is happening in our production environment. However, it's currently being spammed for no reason because of #21003.

tl;dr: user-programmed exit codes (return <int> in `execute()`) should NOT log an error message to monolog. I find it a bit iffy to leave the `console.terminiate` event listener in since it now logs to `debug` instead. I'm unsure if people want/need this, so I might as well remove the terminate listener entirely.

Commits
-------

cadbed3 [Console] Log exit codes as debug messages instead of errors

chalasr added a commit that referenced this pull request Sep 3, 2017

feature #21414 [Console] Display file and line on Exception (arno14)
This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Display file and line on Exception

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

When an exception occured:
If you have not set a verbose mode when running the command, you don't have information about the file and the line on which the exception occured.
So you must run a second time the command but the exception may not occured this time (if based on some databases datas which have changed) and it's a waste of time if you process is very long and the exception occured at the end.

The feature #21003 solve the problem if you are in a standard Symfony edition, if you leave the default settings and if you are in dev mode

Commits
-------

484d278 [Console] Display file and line on Exception
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.