Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Display the log context in the debug pages#24151
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
fabpot commentedSep 11, 2017
Removing the event channel should probably not be harcoded, but moved to the default configuration in Flex/Symfony SE. |
ro0NL commentedSep 11, 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.
@fabpot is there a (hacky) way in twig to check if profiler_dump exists? Can we think of one? :) edit: a wrapper in CodeExtension could do |
javiereguiluz commentedSep 22, 2017
Instead of complicating things with a new config option to make the list of excluded channels configurable, could we hardcode it? For our own log channels this is correct (event is annoying but the rest are useful). The only problem could be if users want to exclude some custom channels that are also annoying. I'd say: let's not implement that unless a significant number of people ask for it. |
jvasseur commentedSep 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.
Maybe we could detect if all the informations from the context are used in the message an hide the context in those cases ? |
This PR was squashed before being merged into the 3.3 branch (closes#24244).Discussion----------TwigBundle exception/deprecation tweaks| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->- 1st commit) if you view a exception in the profiler, there is no logger available. Making the tab useless, disabled state is now triggered at zero log messages. There's a specialized panel here.- 2nd commit) when an exception occurs this highlights deprecations in the log table outside the profiler with a warning status. This follows the same signal colors in the profiler.- 3rd commit) hide the default inactive tabs from CSS to avoid scrollbar flickering.- 4th commit) favors document.DOMContentLoaded over window.load, we dont want to wait for images to be loadedFurther out-of-scope improvements could be;- From#24191; i think the logs table should show a direct `View file` link for every error/deprecation/red or yellow line in here. Traversing with `Show context` is tedious. - links to file.php for your trigger_error() calls - links to config.yml for trigger_error() calls by SF- From#24151; having the same tooling on both sides is nice- Events/Translations logs is noise, we have specialized panels for those. To further reduce the overall page size container logs can be moved away too, linked from Configuration and/or Logs. Also see#23247Commits-------1c595fc [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded eventea4b096 [WebProfilerBundle] Hide inactive tabs from CSS0c10f97 [TwigBundle] Make deprecations scream in logs03cd9e5 [TwigBundle] Hide logs if unavailable, i.e. webprofiler
| <td>{{log.message|format_log_message(log.context) }}</td> | ||
| {# hide the context of the 'event' channel because it only displays duplicated information#} | ||
| {%iflog.context ??falseandlog.channel!='event' %} | ||
| <divclass="text-muted"> |
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.
invalid markup. A div cannot be a child of a<tr>
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've checked the HTML spec and a<tr> can have<td> children and<td> can have children of any type included in "flow content":https://www.w3.org/TR/html51/tabular-data.html#the-td-element. And<div> is one of those "flow content" types:https://www.w3.org/TR/html51/dom.html#flow-content. So, are we good?
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.
but this isnot inside the<td>. It isafter it
ro0NL commentedSep 27, 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.
Why do we care aboutevent channel, to be excluded here, actually? What we need arefilters right? Profiler doesnt do this AFAIK. |
fabpot commentedOct 1, 2017
@javiereguiluz Can you fix@stof comment here? |
nicolas-grekas commentedOct 8, 2017
ping@javiereguiluz should we move this to 4.1? |
nicolas-grekas commentedOct 12, 2017
Moving to 4.1. |
| {# hide the context of the 'event' channel because it only displays duplicated information#} | ||
| {%iflog.context ??falseandlog.channel!='event' %} | ||
| <divclass="text-muted"> | ||
| {{ profiler_dump(log.context) }} |
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 indeed create a hard dependency on WebProfilerBundle in TwigBundle (which will break things in environments where WebProfilerBundle is not available)
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.
How can we solve this? Maybe we can replaceprofiler_dump byyaml_dump?https://symfony.com/doc/current/reference/twig_reference.html#yaml-dump
This PR was squashed before being merged into the 3.3 branch (closessymfony#24244).Discussion----------TwigBundle exception/deprecation tweaks| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->- 1st commit) if you view a exception in the profiler, there is no logger available. Making the tab useless, disabled state is now triggered at zero log messages. There's a specialized panel here.- 2nd commit) when an exception occurs this highlights deprecations in the log table outside the profiler with a warning status. This follows the same signal colors in the profiler.- 3rd commit) hide the default inactive tabs from CSS to avoid scrollbar flickering.- 4th commit) favors document.DOMContentLoaded over window.load, we dont want to wait for images to be loadedFurther out-of-scope improvements could be;- Fromsymfony#24191; i think the logs table should show a direct `View file` link for every error/deprecation/red or yellow line in here. Traversing with `Show context` is tedious. - links to file.php for your trigger_error() calls - links to config.yml for trigger_error() calls by SF- Fromsymfony#24151; having the same tooling on both sides is nice- Events/Translations logs is noise, we have specialized panels for those. To further reduce the overall page size container logs can be moved away too, linked from Configuration and/or Logs. Also seesymfony#23247Commits-------1c595fc [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded eventea4b096 [WebProfilerBundle] Hide inactive tabs from CSS0c10f97 [TwigBundle] Make deprecations scream in logs03cd9e5 [TwigBundle] Hide logs if unavailable, i.e. webprofiler
javiereguiluz commentedMar 27, 2018
To not add a hard dependency on VarDumper, I propose a different implementation: the log context is dumped according to what you have installed. If your app has VarDumper, you see this:
If your app has Yaml, you see this:
Otherwise, you see this:
|
| { | ||
| $logs =array(); | ||
| foreach ($logger->getLogs()as$log) { | ||
| if (class_exists('Symfony\Component\VarDumper\Dumper\HtmlDumper')) { |
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.
You could use::class consts.
| $dumper =newHtmlDumper(); | ||
| $dumper->dump($cloner->cloneVar($log['context']),$output =fopen('php://memory','r+b')); | ||
| $contextAsString =stream_get_contents($output, -1,0); | ||
| }elseif (class_exists('Symfony\Component\Yaml\Yaml')) { |
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.
Same here
| if (class_exists('Symfony\Component\VarDumper\Dumper\HtmlDumper')) { | ||
| $cloner =newVarCloner(); | ||
| $dumper =newHtmlDumper(); | ||
| $dumper->dump($cloner->cloneVar($log['context']),$output =fopen('php://memory','r+b')); |
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.
You can just passtrue as the second argument since 3.2 (4be9776) to get the string as returned value.
javiereguiluz commentedMar 27, 2018
My question ... should we complicate this so much ... or should we just always use |
| {%endif %} | ||
| <td>{{log.message|format_log_message(log.context) }}</td> | ||
| {%iflog.context ??false %} | ||
| <divclass="text-muted prewrap log-context">{{log.context_as_string|raw }}</div> |
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.
currently, you have a XSS issue here: you apply theraw filter, but only the rendering using VarDumper is producing HTML properly. Other case are not escaping anything to produce HTML
| }elseif (class_exists('Symfony\Component\Yaml\Yaml')) { | ||
| $contextAsString = Yaml::dump((array)$log['context'],1024); | ||
| }else { | ||
| $contextAsString =json_encode($log['context'],JSON_PRETTY_PRINT); |
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 suggest also adding theJSON_UNESCAPED_UNICODE andJSON_UNESCAPED_SLASHES flags to make the output more human-readable (having\u... codes for non-ASCII chars makes things harder to read)
| <td> | ||
| {{log.message|format_log_message(log.context) }} | ||
| {%iflog.context ??false %} | ||
| <spanclass="block text-muted prewrap m-t-5">{{log.context|json_encode(constant('JSON_PRETTY_PRINT') b-orconstant('JSON_UNESCAPED_UNICODE') b-orconstant('JSON_UNESCAPED_SLASHES')) }}</span> |
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.
it is is a block display withprewrap, why not using<pre> ? It would be the dedicated markup rather than aspan
| {{log.message|format_log_message(log.context) }} | ||
| {%iflog.context ??false %} | ||
| <spanclass="blocktext-muted prewrap m-t-5">{{log.context|json_encode(constant('JSON_PRETTY_PRINT') b-orconstant('JSON_UNESCAPED_UNICODE') b-orconstant('JSON_UNESCAPED_SLASHES')) }}</span> | ||
| <preclass="text-muted prewrap m-t-5">{{log.context|json_encode(constant('JSON_PRETTY_PRINT') b-orconstant('JSON_UNESCAPED_UNICODE') b-orconstant('JSON_UNESCAPED_SLASHES')) }}</pre> |
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.
do you still needprewrap here ? If no, this could be removed (and removing the class from the cSS as your PR is adding it)
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.
Yes, I think it's needed because when outputting long lines (e.g. long class namespaces) the<pre> doesn't break lines and makes the debug page hugely wide.
fabpot commentedOct 10, 2018
Thank you@javiereguiluz. |
…luz)This PR was squashed before being merged into the 4.2-dev branch (closes#24151).Discussion----------Display the log context in the debug pages| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23589| License | MIT| Doc PR | -### Before### AfterI'd like to exclude the `event` channel context because it only adds noise:-----This change would require to add a hard dependency to the VarDumper component. Do we want to do that? Thanks!Commits-------c59fbde Display the log context in the debug pages
Before
After
I'd like to exclude the
eventchannel context because it only adds noise:This change would require to add a hard dependency to the VarDumper component. Do we want to do that? Thanks!