-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Bridge\Monolog][FrameworkBundle] Add & wire a DebugProcessor #20416
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
Conversation
6f1e17e
to
194d36a
Compare
'context' => $record['context'], | ||
'channel' => isset($record['channel']) ? $record['channel'] : '', | ||
); | ||
switch ($record['level']) { |
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 switch looks unneeded. What about using a simple if
?
if (in_array($record['level'], array(Logger::ERROR, Logger::CRITICAL, Logger::ALERT, Logger::EMERGENCY))) {
++$this->errorCount;
}
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.
Longer line, one array allocation, one function call: not my preference...
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.
OK then ... but according to 3v4l.org, the if
code generates 7 opcodes and the switch
code generates 11 opcodes (although it's true that not all opcodes are equal!)
5271fb6
to
3f53985
Compare
3f53985
to
7572a53
Compare
Thank you @nicolas-grekas. |
…ocessor (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [Bridge\Monolog][FrameworkBundle] Add & wire a DebugProcessor | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20370 | License | MIT | Doc PR | - As identified in #20370, collecting the log records for the profiler should happen before any processor. The only way to do so is by registering a processor to do exactly that. Since the last added processor is called first, this one is wired in a compiler pass that is called after other processors are registered. The DebugHandler class being now useless, it is deprecated. If this approach is accepted, I'll send a PR on monolog bundle & silex to leverage it. Commits ------- 7572a53 [Bridge\Monolog][FrameworkBundle] Add & wire a DebugProcessor
@nicolas-grekas Can you make the PRs on the bundle and Silex? Thanks. |
…le in MonologServiceProvider (nicolas-grekas) This PR was merged into the 2.0.x-dev branch. Discussion ---------- Use DebugProcessor instead of DebugHandler when available in MonologServiceProvider Related to symfony/symfony#20416 Commits ------- 213741a Use DebugProcessor instead of DebugHandler when available in MonologServiceProvider
…sorPass in FrameworkBundle (nicolas-grekas) This PR was merged into the 2.x branch. Discussion ---------- Deprecate DebugHandlerPass in favor of AddDebugLogProcessorPass in FrameworkBundle Related to symfony/symfony#20416 Commits ------- a55b0a0 Deprecate DebugHandlerPass in favor of AddDebugLogProcessorPass in FrameworkBundle
@nicolas-grekas there is an issue here though: if no handler is handling the record, processors will not be called by Composer |
What about making DebugProcessor a null handler then? Or adding a
NullHandler if that's too hacky :)
|
Not very useful for integration tests of processors 🤔 |
As identified in #20370, collecting the log records for the profiler should happen before any processor.
The only way to do so is by registering a processor to do exactly that.
Since the last added processor is called first, this one is wired in a compiler pass that is called after other processors are registered.
The DebugHandler class being now useless, it is deprecated.
If this approach is accepted, I'll send a PR on monolog bundle & silex to leverage it.