Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WIP] [Console] Add basic support for automatic console exception logging#19382
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[WIP] [Console] Add basic support for automatic console exception logging#19382
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jameshalsall commentedJul 20, 2016
ping@Tobion - be good to get your feedback on this before I do any more |
| $event->setExitCode(255); | ||
| } | ||
| $message = sprintf('Command `%s` exited with status code %d'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this looks something else than what the title of this PR suggests. moreover, I'm not sure it's forth logging a non zero status code: it's the command's responsibility to know if it's worth logging of not, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The idea of this PR is to add automatic exception logging "out of the box" with zero configuration. Maybe I'm misunderstanding but surely anything other than a 0 exit code would result in some kind of log entry, unless there's a fatal PHP error before Symfony kernel bootstraps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- something is missing in the sprintf
- I would add the current argument/options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated
jameshalsall commentedJul 21, 2016
After thinking about this a little more, I still can't help but think this is not a good idea:
I was against this originally in#10895, but thought it might offer some value for people. Now I'm not so sure. Curious to see what@Tobion thinks as he was the Symfony member who originally was for this change. |
| $message = sprintf('Command `%s` exited with status code %d'); | ||
| $this->logger->warning($message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would puterror.
lyrixx commentedAug 9, 2016
👍 |
| } | ||
| if ($exitCode > 255) { | ||
| $event->setExitCode(255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, this should not be done here.
And BTW, it's already done therehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Application.php#L141-L147
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great point, I will remove this
6382bf1 to6acd944Compare| $exitCode | ||
| ); | ||
| $this->logger->error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We prefer single line here, same above. And in fact I think the intermediate $message is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 my bad
b8209e7 to2874431Compare| $this->logger->error('Command `{command}` exited with status code {code}',array('command' =>implode('',$_SERVER['argv']),'code' =>$exitCode)); | ||
| $input =newArgvInput(null,$event->getCommand()->getDefinition()); | ||
| $this->logger->error('Command `{command}` exited with status code {code}',array('command' => (string)$input,'code' =>$exitCode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we really need the backtick ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably not, I'll remove them
| $exception = $event->getException(); | ||
| $this->logger->error($exception->getMessage(), array('exception' => $exception)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sorry to be very picky, but the log message is not good. I prefer something like that:'Exception thrown while running command: "%s". Message: "%s".
It's better because if the RDBMS goes down (for example), many commands or consumers will fail. So you will receive N time the exact same message. It's better to have N messages because all commands are different and you must know what command fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No problem, I'll update this later
| "symfony/process":"~2.8|~3.0", | ||
| "psr/log":"~1.0" | ||
| "psr/log":"~1.0", | ||
| "phpunit/phpunit":"~4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We never add phpunit dependency in symfony.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure why that's showing, I reverted that change last night - was committed by mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I saw that after commenting. Thanks.
fabpot commentedSep 14, 2016
@jameshalsall What's the status of this PR? |
jameshalsall commentedSep 14, 2016
I will finish this off soon :) James Halsall Sent from On Wed, 14 Sep 2016 at 22:55 Fabien Potencier <
a, pre, code, a:link, body { word-wrap: break-word !important; } https://github.com/jameshalsall — You are receiving this because you were mentioned. Reply to this email directly, |
chalasr commentedNov 6, 2016
It would be great to see this happen. |
jameshalsall commentedNov 6, 2016
I'll pick this up again this week, had forgotten about it. |
chalasr commentedJan 15, 2017
Finished in#21003, thanks@jameshalsall |
fabpot commentedJan 15, 2017
Closing in favor of#21003 |
…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#7373Continues#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 testsLog 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 ExceptionListener9896547 Add basic support for automatic console exception logging
haroldiedema commentedAug 4, 2017
Exit codes are used to communicate with the processes that spawned a process (e.g symfony command). In our case, we use exit codes to communicate back to bash processes to indicate what a spawned process did. Now our 'error'-logs are being spammed for no reason since In short: An exit code other than |
chalasr commentedAug 5, 2017
@haroldiedema Would you mind to open a PR for changing the log level? |
Uh oh!
There was an error while loading.Please reload this page.
This is a quick initial concept for automatic exception logging, it currently takes the 2 listeners outlined in the docs (http://symfony.com/doc/current/cookbook/console/logging.html) and brings them into the standard edition.
I'd like to get some feedback before writing tests and updating the docs in a separate PR.
TODO: