8000 [WIP] [Console] Add basic support for automatic console exception logging by jameshalsall · Pull Request #19382 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 15 commits into from
Closed

[WIP] [Console] Add basic support for automatic console exception logging #19382

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4284161
Add basic support for automatic console exception logging
jameshalsall Jul 18, 2016
17ef8db
Fix coding standards
jameshalsall Jul 18, 2016
3d6b22b
Simplify the log entry for console exceptions
jameshalsall Jul 21, 2016
0961f8c
Remove unnecessary exit code handling in ExceptionListener
jameshalsall Aug 9, 2016
5bd799e
Fix onKernelTerminate message logging
jameshalsall Aug 9, 2016
80c3ac3
Fix coding style
jameshalsall Aug 9, 2016
f7a7048
Fix code style
Aug 17, 2016
c21d206
Fix code style, add TODO
Aug 17, 2016
2874431
Update console error log with placeholders and context
Aug 17, 2016
bcbc8cb
Replace global usage with ArgvInput
Aug 17, 2016
da278e6
Remove backticks around command name in log message
jameshalsall Aug 17, 2016
44a0822
Add test for onKernelTerminate
jameshalsall Aug 17, 2016
5dc8b61
Update console ExceptionListener messaging, add test
jameshalsall Aug 18, 2016
b196557
Add tests for onKernelTerminate, fix log message
jameshalsall Aug 18, 2016
e6c4994
Revert change to composer.json
jameshalsall Aug 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove unnecessary exit code handling in ExceptionListener
  • Loading branch information
@jameshalsall
jameshalsall authored and James Halsall committed Aug 17, 2016
commit 0961f8cfc881364170a02b2ec354c89bea458736
6 changes: 1 addition & 5 deletions src/Symfony/Component/Console/EventListener/ExceptionListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*
* Attempts to log exceptions or abnormal terminations of console commands.
*
* @author James Halsall <james.t.halsall@googlemail.com>
* @author James Halsall <james.t.halsall@googlemail.com>
*/
class ExceptionListener implements EventSubscriberInterface
{
Expand Down Expand Up @@ -62,10 +62,6 @@ public function onKernelTerminate(ConsoleTerminateEvent $event)
return;
}

if ($exitCode > 255) {
$event->setExitCode(255);
}

$message = sprintf('Command `%s` exited with status code %d');
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. something is missing in the sprintf
  2. I would add the current argument/options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated


$this->logger->warning($message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put error.

Expand Down
0