[Debug] Better error handling #19568

lyrixx · 2016-08-08T14:08:11Z

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#6870

Send the raw exception in the log context instead of custom formatting
Add config option to log/throw in Symfony all PHP errors
Always use an exception when a PHP error occurs
Expand exception in the log context in the web developer toolbar
Use the dumper to dump log context in the web developer toolbar

I used the following code to produce screenshots:

public 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);
// ...

fabpot · 2016-08-11T06:08:16Z

src/Symfony/Component/Debug/ErrorHandler.php

@@ -373,24 +376,28 @@ public function handleError($type, $message, $file, $line, array $context, array
        if (null !== $backtrace && $type & E_ERROR) {
            // E_ERROR fatal errors are triggered on HHVM when
            // hhvm.error_handling.call_user_handler_on_fatals=1
-            // which is the way to get their backtrace.
+            // which is the way to get their backtraces.


Previous sounds correct to me.

lyrixx · 2016-08-11T16:14:51Z

Status: Needs review

nicolas-grekas · 2016-08-11T16:43:03Z

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 (see https://twitter.com/nicolasgrekas/status/762915798969098240, such a tie justifies the options).

👍
Status: reviewed

mickaelandrieu · 2016-08-15T03:35:45Z

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

-        {% if stack %}
-            <button class="btn-link text-small sf-toggle" data-toggle-selector="#{{ stack_id }}" data-toggle-alt-content="Hide stack trace">Show stack trace</button>
+        {% if trace %}
+            <button class="btn-link text-small sf-toggle" data-toggle-selector="#{{ trace_id }}" data-toggle-alt-content="Hide trace trace">Show trace trace</button>


Show trace / Hide trace, right ?

Good catch. Thanks.

nicolas-grekas · 2016-08-16T09:05:59Z

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

lyrixx · 2016-08-16T09:32:59Z

👍

fabpot · 2016-08-16T15:03:59Z

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

lyrixx · 2016-08-16T15:16:09Z

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.

The scream option could be interesting for people who want really high performance.

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

fabpot · 2016-08-16T15:24:58Z

For log, 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 be false?

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

nicolas-grekas · 2016-08-16T15:30:46Z

I think we can drop scope and scream as config options.
I'd be fine with setting log 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?

lyrixx · 2016-08-16T15:34:49Z

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.

fabpot · 2016-08-16T15:56:07Z

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

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


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

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.

I added a deprecation notice.

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

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

the value is now false.

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

nicolas-grekas · 2016-08-17T15:35:36Z

Thank you @lyrixx.

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#6870 1. Send the raw exception in the log context instead of custom formatting 2. Add config option to log/throw in Symfony all PHP errors 3. Always use an exception when a PHP error occurs 4. Expand exception in the log context in the web developer toolbar 5. Use the dumper to dump log context in the web developer toolbar --- I used the following code to produce screenshots: ```php public 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

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

… 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

lyrixx · 2023-06-14T15:11:47Z

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

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


I don't recall, why is it JsonSerializable? (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" ] } }

carsonbot added Status: Needs Review Debug labels Aug 8, 2016

lyrixx force-pushed the better-error-logging branch 3 times, most recently from c17f213 to 64b6a4e Compare August 8, 2016 15:13

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Aug 9, 2016

lyrixx force-pushed the better-error-logging branch 8 times, most recently from 816e296 to 29cb0a7 Compare August 10, 2016 16:38

fabpot reviewed Aug 11, 2016
View reviewed changes

lyrixx force-pushed the better-error-logging branch 10 times, most recently from 4d60d08 to 335f47b Compare August 11, 2016 16:00

carsonbot added Status: Needs Review and removed Status: Needs Work labels Aug 11, 2016

mickaelandrieu reviewed Aug 15, 2016
View reviewed changes

lyrixx force-pushed the better-error-logging branch from 335f47b to f19fe82 Compare August 15, 2016 10:55

lyrixx force-pushed the better-error-logging branch from f19fe82 to 89feb7f Compare August 16, 2016 15:38

fabpot reviewed Aug 16, 2016
View reviewed changes

lyrixx force-pushed the better-error-logging branch 3 times, most recently from 20585ae to 8829998 Compare August 17, 2016 13:24

[Debug] Better error handling

8f24549

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

lyrixx force-pushed the better-error-logging branch from 8829998 to 8f24549 Compare August 17, 2016 15:20

lyrixx mentioned this pull request Aug 17, 2016

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

Merged

nicolas-grekas merged commit 8f24549 into symfony:master Aug 17, 2016

lyrixx deleted the better-error-logging branch August 17, 2016 16:06

This was referenced Aug 18, 2016

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

Merged

[HttpKernel] Use VarDumper in the profiler #19614

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Debug] Better error handling #19568

[Debug] Better error handling #19568

[Debug] Better error handling #19568

[Debug] Better error handling #19568

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment