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

add previous exception messages to plain text output #609

Merged
merged 2 commits into from Jun 21, 2019

Conversation

tflori
Copy link
Contributor

@tflori tflori commented Dec 3, 2018

fixes #608

Can be disabled via PlainTextHandler::addPreviousToOutput(false) and
adds lines between the exception and the trace containing the exception
and line of creator.

The method Inspector::getFrames() already appends all frames from the
previous exceptions. So there is no need in adding the trace for each
previous exception.

IMO the test tests something that it should not test: the output of the
line number. But that should be in a separate PR. I've at least added
constants for the line numbers so that you only have to change them once
for all tests.

Can be disabled via `PlainTextHandler::addPreviousToOutput(false)` and
adds lines between the exception and the trace containing the exception
and line of creator.

The method `Inspector::getFrames()` already appends all frames from the
previous exceptions. So there is no need in adding the trace for each
previous exception.

IMO the test tests something that it should not test: the output of the
line number. But that should be in a separate PR. I've at least added
constants for the line numbers so that you only have to change them once
for all tests.
@tflori
Copy link
Contributor Author

tflori commented Dec 3, 2018

I just realized how phpunit outputs the trace - they have some more line breaks:

Exception: Outer exception message
<trace>

Caused by
Exception: Inner exception

<trace>

The trace for later exceptions is just a subset of the last but dividing the output with a line break and the Caused by keyword makes it easy to find specific exceptions with egrep '^MyException:' app.log.

What do you think?

@staabm
Copy link
Contributor

staabm commented Dec 3, 2018

Go for it.

/**
* @var bool
*/
private $addPreviousToOutput = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this switch? Do you know a case where we dont want the previous exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe someone don't want it in the logs. or they show it to the client and want to hide it there (for example the message could contain internal information that should only be visible to developers).

@denis-sokolov denis-sokolov merged commit 96b5407 into filp:master Jun 21, 2019
@denis-sokolov
Copy link
Collaborator

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlainTextHandler does not show previous exceptions
3 participants