Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Enhance logging in compiler passes#21396
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJan 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It would be great to have a profiler panel that displays the contents of the var/cache/dev/appDevDebugProjectContainerCompiler.log file btw! |
a7f3d74 to9f0545eCompare| */ | ||
| publicfunctionaddLogMessage($string) | ||
| { | ||
| @trigger_error($string); |
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 will trigger them asE_USER_ERROR. IMO, it is too high. What aboutE_USER_NOTICE instead ? Lower than notice would be even better as this is a debugging info, but trigger_error does not have something lower (as it is not meant to be used as a debug logger)
btw, I'm not even convinced this is a good idea (at least not today as people will complain about silenced notices in the profiler)
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.
looking at the phdoc, the default isE_USER_NOTICE, so all good.
but I now made it explicit, and configurable (via a third "$level" arg)
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.
hum OK, I removed the trigger_error call for now - but kept the $level arg: even unused, it make the signature compatible with future changes requiring it.
stof commentedJan 25, 2017
The panel showing the container logs is a good idea though |
a0623c9 to03be6ddCompare| $this->getCompiler()->log($pass,sprintf('Removed service "%s"; reason: %s.',$id,$reason)); | ||
| } | ||
| publicfunctionlog(CompilerPassInterface$pass,$message,$level =E_USER_NOTICE) |
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.
I don't think passing a level here makes much sense, especially not as a PHP error level. It would couple it too much to the error triggering layer, while we talk about logging here.
If you want to provide a logging level, use PSR-3 levels instead.
Btw, a way to allow flexibility about adding a level only the day we need it is to make the method final: final method can add optional args as they want (adding an optional arg only breaks BC for inheritance)
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.
good idea, they are final now
70b9a6e to21b8b34Comparenicolas-grekas commentedJan 29, 2017
"logRemoveService" removed |
fabpot commentedJan 30, 2017
Thank you@nicolas-grekas. |
This PR was merged into the 3.3-dev branch.Discussion----------[DI] Enhance logging in compiler passes| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -We should log more in compiler passes - and this should be better integrated in usual log reports.For logging more, let's drop LoggingFormatter and add a simple "log" method on ContainerBuilder.For better integration, let's throw silenced notices - they can be caught by our Debug handler.Commits-------fb200a0 [DI] Enhance logging in compiler passes
We should log more in compiler passes - and this should be better integrated in usual log reports.
For logging more, let's drop LoggingFormatter and add a simple "log" method on ContainerBuilder.
For better integration, let's throw silenced notices - they can be caught by our Debug handler.