publicfunction indexAction(Request$request)    {$this->get('logger')->info('A log message with an exception', ['exception' =>new \Exception('this exception will be logged')]);error_reporting(0);for ($i=0;$i <15;$i++) {if ($i ==5) {error_reporting(E_ALL);            }if ($i ==10) {error_reporting(0);            }trigger_error("Trigger error avec E_USER_NOTICE",E_USER_NOTICE);        }error_reporting(E_ALL);        @trigger_error("trigger_error avec E_USER_DEPRECATED",E_USER_DEPRECATED);trigger_error("trigger_error avec E_USER_DEPRECATED (not silent)",E_USER_DEPRECATED);// ...

carsonbot added Status: Needs Review Debug labels

Aug 8, 2016

lyrixx force-pushed thebetter-error-logging branch 3 times, most recently fromc17f213 to64b6a4eCompare

August 8, 2016 15:13

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels

Aug 9, 2016

lyrixx force-pushed thebetter-error-logging branch 8 times, most recently from816e296 to29cb0a7Compare

August 10, 2016 16:38

fabpot reviewed

Aug 11, 2016

View reviewed changes

src/Symfony/Component/Debug/ErrorHandler.php Outdated

		// E_ERROR fatal errors are triggered on HHVM when
		// hhvm.error_handling.call_user_handler_on_fatals=1
		// which is the way to get theirbacktrace.
		// which is the way to get theirbacktraces.

Copy link

Member

Choose a reason for hiding this comment

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

Previous sounds correct to me.

lyrixx force-pushed thebetter-error-logging branch 10 times, most recently from4d60d08 to335f47bCompare

August 11, 2016 16:00

Copy link

MemberAuthor

lyrixx commentedAug 11, 2016

Status: Needs review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Aug 11, 2016

Copy link

Member

nicolas-grekas commentedAug 11, 2016•
edited
Loading

Looks like a really nice improvement to me.
Having a typed exception in the log contexts will allow handling them in more useful ways.
And adding configuration empowers people (seehttps://twitter.com/nicolasgrekas/status/762915798969098240, such a tie justifies the options).

👍
Status: reviewed

mickaelandrieu reviewed

Aug 15, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig Outdated

		{%ifstack %}
		<buttonclass="btn-link text-small sf-toggle"data-toggle-selector="#{{stack_id }}"data-toggle-alt-content="Hidestack trace">Showstack trace</button>
		{%iftrace %}
		<buttonclass="btn-link text-small sf-toggle"data-toggle-selector="#{{trace_id }}"data-toggle-alt-content="Hidetrace trace">Showtrace trace</button>

Copy link

Contributor

mickaelandrieuAug 15, 2016

Choose a reason for hiding this comment

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

Show trace / Hide trace, right ?

Copy link

MemberAuthor

lyrixxAug 15, 2016

Choose a reason for hiding this comment

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

Good catch. Thanks.

lyrixx force-pushed thebetter-error-logging branch from335f47b tof19fe82Compare

August 15, 2016 10:55

Copy link

Member

nicolas-grekas commentedAug 16, 2016

Unless someone would like to review this PR and needs more time, I plan to merge this one by the end of the day.

Copy link

MemberAuthor

lyrixx commentedAug 16, 2016

👍

Copy link

Member

fabpot commentedAug 16, 2016

I understand the need for thethrow configuration, but not sure it the other ones make sense.

Copy link

MemberAuthor

lyrixx commentedAug 16, 2016

log also is very important to me. It allow us to manage logging of PHP errors with PHP itself. So all logs are managed in the same way. ie: there are no need to "capture" the output of php fpm.

Thescream option could be interesting for people who wantreally high performance.

For the record, I did not add thetrace option because with the new system we always have the trace.

Copy link

Member

fabpot commentedAug 16, 2016

Forlog, I think that having all logs managed the same way is the only sensible configuration, and should have been the case since the beginning :) So, the question is more about when would you want it to befalse?

Forscream, we are only talking about dev, right? If so, performance is not a concern IMHO.

Copy link

Member

nicolas-grekas commentedAug 16, 2016•
edited
Loading

I think we can dropscope andscream as config options.
I'd be fine with settinglog to true and remove the config, but wouldn't this be a behavioral bc break for people who deal and expect log files generated by php?

Copy link

MemberAuthor

lyrixx commentedAug 16, 2016

Yes, we can not change the value of log to true as it's a kind of BC. I'm gonna remove the 2 others options.

lyrixx force-pushed thebetter-error-logging branch fromf19fe82 to89feb7fCompare

August 16, 2016 15:38

fabpot reviewed

Aug 16, 2016

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated

		->booleanNode('log')
		->info('Use the app logger instead of the PHP logger for logging PHP errors.')
		->defaultValue($this->debug)
		->treatNullLike($this->debug)

Copy link

Member

fabpotAug 16, 2016

Choose a reason for hiding this comment

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

I don't understand the default value. For me, the value does not depend on the debug flag. It should betrue by default (most sensible value) and devs can switch it tofalse to keep BC. But then, what about having this as a temporary setting to keep BC that needs to be removed in 4.0?

Copy link

MemberAuthor

lyrixxAug 16, 2016

Choose a reason for hiding this comment

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

But then, what about having this as a temporary setting to keep BC that needs to be removed in 4.0?

👍 It was ou initial idea.

Copy link

MemberAuthor

lyrixxAug 17, 2016

Choose a reason for hiding this comment

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

I added a deprecation notice.

Copy link

MemberAuthor

lyrixxAug 17, 2016

Choose a reason for hiding this comment

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

And I reverted it. I cause too many troubles with tests that can not be fixed.

Copy link

Member

fabpotAug 17, 2016

Choose a reason for hiding this comment

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

You reverted the deprecation, but the value is still at debug, which looks very wrong to me still.

Copy link

MemberAuthor

lyrixxAug 17, 2016

Choose a reason for hiding this comment

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

the value is nowfalse.

lyrixx force-pushed thebetter-error-logging branch 3 times, most recently from20585ae to8829998Compare

August 17, 2016 13:24

[Debug] Better error handling

8f24549

1. Send the raw exception in the log context instead of custom formatting2. Add config option to log in Symfony all PHP errors

lyrixx force-pushed thebetter-error-logging branch from8829998 to8f24549Compare

August 17, 2016 15:20

lyrixx mentioned this pull request

Aug 17, 2016

Log all PHP errors with the default loggersymfony/symfony-standard#988

Merged

Copy link

Member

nicolas-grekas commentedAug 17, 2016

Thank you@lyrixx.

nicolas-grekas merged commit8f24549 intosymfony:master

Aug 17, 2016

nicolas-grekas added a commit that referenced this pull request

Aug 17, 2016

feature#19568[Debug] Better error handling (lyrixx)

f2200f7

This PR was merged into the 3.2-dev branch.Discussion----------[Debug] Better error handling| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | -| Fixed tickets | -| License       | MIT| Doc PR        |symfony/symfony-docs#68701. Send the raw exception in the log context instead of custom formatting2. Add config option to log/throw in Symfony all PHP errors3. Always use an exception when a PHP error occurs4. Expand exception in the log context in the web developer toolbar5. Use the dumper to dump log context in the web developer toolbar---I used the following code to produce screenshots:```phppublic function indexAction(Request $request)    {        $this->get('logger')->info('A log message with an exception', ['exception' => new \Exception('this exception will be logged')]);        error_reporting(0);        for ($i=0; $i < 15; $i++) {            if ($i == 5) {                error_reporting(E_ALL);            }            if ($i == 10) {                error_reporting(0);            }            trigger_error("Trigger error avec E_USER_NOTICE", E_USER_NOTICE);        }        error_reporting(E_ALL);        @trigger_error("trigger_error avec E_USER_DEPRECATED", E_USER_DEPRECATED);        trigger_error("trigger_error avec E_USER_DEPRECATED (not silent)", E_USER_DEPRECATED);// ...```![screenshot16](https://cloud.githubusercontent.com/assets/408368/17582279/2c4239b0-5fab-11e6-8428-2eaa7372cce3.png)![screenshot17](https://cloud.githubusercontent.com/assets/408368/17582287/30cad1ea-5fab-11e6-9b0b-de0fa9f3913b.png)![screenshot18](https://cloud.githubusercontent.com/assets/408368/17582291/348bb574-5fab-11e6-83b0-5bfaac080838.png)Commits-------8f24549 [Debug] Better error handling

lyrixx deleted the better-error-logging branch

August 17, 2016 16:06

fabpot added a commit to symfony/symfony-standard that referenced this pull request

Aug 18, 2016

feature#988Log all PHP errors with the default logger (lyrixx)

4344d67

This PR was merged into the 3.2-dev branch.Discussion----------Log all PHP errors with the default loggerrelated tosymfony/symfony#19568Commits-------ba48afe Log all PHP errors with the default logger

This was referencedAug 18, 2016

[FrameworkBundle][Debug] Fix default config and cleaning of traces#19656

Merged

[HttpKernel] Use VarDumper in the profiler#19614

Merged

fabpot added a commit that referenced this pull request

Aug 22, 2016

minor#19656[FrameworkBundle][Debug] Fix default config and cleaning…

c14fff0

… of traces (nicolas-grekas)This PR was merged into the 3.2-dev branch.Discussion----------[FrameworkBundle][Debug] Fix default config and cleaning of traces| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| Tests pass?   | yes| Fixed tickets | Follow up#19568| License       | MIT| Doc PR        | -The default value of `framework.php_errors.log` must be `%kernel.debug%` to have deprecations and silenced errors logged in dev as before.Cleaning the trace was broken because a closure can't be bound to an internal class.This PR fixes both issues and enhance trace cleaning a bit by removing arguments from traces so that they take less memory when collected as part of the context of log messages.Commits-------f640870 [FrameworkBundle][Debug] Fix default config and cleaning of traces

xabbuh mentioned this pull request

Sep 27, 2016

[DX] Logging deprecations#17858

Closed

fabpot mentioned this pull request

Oct 27, 2016

Release v3.2.0-BETA1#20317

Merged

benbor mentioned this pull request

Jun 22, 2021

[ErrorHandler] fix handling buffered SilencedErrorContext#41760

Merged

lyrixx commented

Jun 14, 2023

View reviewed changes

src/Symfony/Component/Debug/Exception/SilencedErrorContext.php

		*
		* @author Grégoire Pineau <lyrixx@lyrixx.info>
		*/
		class SilencedErrorContext implements \JsonSerializable

Copy link

MemberAuthor

lyrixxJun 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I don't recall, why is itJsonSerializable? (cc@nicolas-grekas)?

It does not play well with monolog:
https://github.com/Seldaek/monolog/blob/b05bf55097060ec20f49ccec0cf2f8e5aaa468b3/src/Monolog/Formatter/NormalizerFormatter.php#L195-L197

I got this in my index:

{"_index":"monolog","_type":"_doc","_id":"sU9quogBWJfLYe0zo1VX","_score":1,"fields": {"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.type.keyword": ["->"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.class.keyword": ["Symfony\\Component\\ErrorHandler\\ErrorHandler"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.class": ["Symfony\\Component\\ErrorHandler\\ErrorHandler"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.function": ["handleError"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.function.keyword": ["handleError"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.trace.type": ["->"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.count": [1    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.severity": [2    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.file.keyword": ["Unknown"    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.line": [0    ],"context.exception.Symfony\\Component\\ErrorHandler\\Exception\\SilencedErrorContext.file": ["Unknown"    ]  }}

Labels

Debug Ready Status: Reviewed

6 participants

Movatterモバイル変換

Uh oh!

[Debug] Better error handling#19568

[Debug] Better error handling#19568

Uh oh!

Conversation

lyrixx commentedAug 8, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyrixx commentedAug 11, 2016

Uh oh!

nicolas-grekas commentedAug 11, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedAug 16, 2016

Uh oh!

lyrixx commentedAug 16, 2016

Uh oh!

fabpot commentedAug 16, 2016

Uh oh!

lyrixx commentedAug 16, 2016

Uh oh!

fabpot commentedAug 16, 2016

Uh oh!

nicolas-grekas commentedAug 16, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

lyrixx commentedAug 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedAug 17, 2016

Uh oh!

lyrixxJun 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lyrixx commentedAug 8, 2016•
edited
Loading

nicolas-grekas commentedAug 11, 2016•
edited
Loading

nicolas-grekas commentedAug 16, 2016•
edited
Loading

lyrixxJun 14, 2023•
edited
Loading