dblog expects debug icon and class
deekayen - October 31, 2007 - 18:11
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | dblog.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
While making a D6 update patch for auto_nodetitle, I added two watchdog debug entries to the module as follows:
watchdog('auto_nodetitle', 'Setting node title to %title', array('%title' => $node['title']), WATCHDOG_DEBUG);
watchdog('auto_nodetitle', 'Setting node changed time to %time', array('%time' => $node['changed']), WATCHDOG_DEBUG);They worked fine, but dblog spit out errors as shown at:
http://www.flickr.com/photos/deekayen/1809789552/
when I viewed the log page. dblog.admin.inc has
$icons[$dblog->severity],on line 68 and on line 76 has
'class' => "dblog-". preg_replace('/[^a-z]/i', '-', $dblog->type) .' '. $classes[$dblog->severity]This patch adds to $icons and $classes to make them as follows:
$icons = array(
WATCHDOG_DEBUG => '',
WATCHDOG_INFO => '',
WATCHDOG_NOTICE => '',
WATCHDOG_WARNING => theme('image', 'misc/watchdog-warning.png', t('warning'), t('warning')),
WATCHDOG_ERROR => theme('image', 'misc/watchdog-error.png', t('error'), t('error')),
WATCHDOG_CRITICAL => '',
WATCHDOG_ALERT => '',
WATCHDOG_EMERG => ''
);
$classes = array(
WATCHDOG_DEBUG => 'dblog-debug',
WATCHDOG_INFO => 'dblog-info',
WATCHDOG_NOTICE => 'dblog-notice',
WATCHDOG_WARNING => 'dblog-warning',
WATCHDOG_ERROR => 'dblog-error',
WATCHDOG_CRITICAL => 'dblog-critical',
WATCHDOG_ALERT => 'dblog-alert',
WATCHDOG_EMERG => 'dblog-emerg'
);I do realize the three high errors probably mean Drupal doesn't display anything, but if you were to fix Drupal, those errors would still display in the recent log until cron cycles them out, so they need settings just as much as the lower level errors.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| dblog.admin_.inc-diff-2007-10-31-14-08-51.patch | 1.5 KB | Ignored | None | None |
#1
Same patch attached, but diffed from drupal root instead of the dblog directory.
Screenshot of error attached instead of on flickr. The auto_nodetitle entries are WATCHDOG_DEBUG level entries. 7 is their numerical priority index, which does not have a corresponding array element in
$iconsor$classesindblog_overview().#2
I've added the missing links to to-come-images.
Note: I have renamed misc/watchdog-ok.png to miscwatchdog-notice.png. Issue posted here: http://drupal.org/node/188354
Cheers
fp
#3
In that case, 1) none of the patches reflect the new filename, and 2) shouldn't the other t() strings be something like debug, info, etc, instead of all warning except one?
#4
ah... the copy/paste mistake. Item 2 is taken care of.
As for item 1, good point. I have found 4 instances where watchdog-ok.png was used in core. It could be also used in some contrib modules.
Maybe it's not worth fussing about but it seems to me that it should be renamed for the sake of consistency.
#5
Here's at least one instance of this cropping up elsewhere: http://drupal.org/node/202705
What exactly are we waiting on for RTBC? rename watchdog-*.png to dblog-*.png? If that's it, see attached for the code. All the new icons I attached are simply transparent 18x18 to have some sort of solution to this bug before D6 instead of arguing over what the new icons should look like. 1x1 is attached as alternative since dimensions on the output aren't set. ok, warning, and error are the same, just renamed.
#6
subscribing
#7
First, I don't think we want to attach transparent images. The theme function should simply do a file_exists() IMO. No point to send transparent pixels down the wire.
Second, I noticed that other modules are also starting to use these icons. That means they are not necessarily watchdog/dblog specific.
#8
#9
Just to clarify, this bug isn't about whether or not
theme_image()can find the image file or not. In fact, the log summary page at admin/reports/dblog doesn't even outputimgtags because the CSS handles those image inserts. The fact that a debug.png doesn't exist doesn't really matter because dblog just lists a dblog-debug in the class specification for CSS and since there's no corresponding CSS reference to a debug.png, there's not even a 404 for Apache to log.The notice displays because dblog-specific references to
$icons[$dblog->severity]and$classes[$dblog->severity]are looking for array elements that don't exist to support the new error statuses. This patch adds those missing elements.In this case, I renamed the file names to remove the
watchdog-and not replace it with adblog-prefix so they're not watchdog/dblog specific, as #7 pointed out. That means whoever commits needs to rename these files in the CVS tree: watchdog-warning.png, watchdog-ok.png, and watchdog-error.png.I started to write an update to theme.inc after reading #7 to make more existing file checks by replacing line 1134. Whether or not
theme_image()does more checking for existing files I think is a separate issue, I wouldn't call it a bug, and I think this bug can be marked fixed without the theme.inc patch.#10
This is closely related to http://drupal.org/node/138079
Please see Steven Wittens comments about conveying the severity levels http://drupal.org/node/138079#comment-544630.
I agree with his opinion regarding the difficulties of creating 8 meaningful severity icons.
I've also attached some examples from an icon set I'm working on, that refreshes the look of the current watchdog icons. I can add any more that are required.
#11
@Deekayen: Patch no longer applies cleanly to update.report.inc, could you re-roll?
patching file modules/dblog/dblog.admin.incpatching file modules/system/admin.css
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 2046 (offset -14 lines).
Hunk #2 succeeded at 2176 (offset -14 lines).
patching file modules/update/update.report.inc
Hunk #1 FAILED at 45.
1 out of 1 hunk FAILED -- saving rejects to file modules/update/update.repoc.rej
patching file themes/garland/style-rtl.css
patching file themes/garland/style.css
#12
Same as #9, just fresh rolls for #11.
#13
Unfortunately drupal-diff-2008-02-01-09-04-24.patch still fails on multiple hunks, and seems to be mangling whitespace in a comment which follows.
Had problems applying theme.inc-diff-2008-02-01-09-04-57.patch as well.
$ patch -p0 <drupal-diff-2008-02-01-09-04-24.patchpatching file modules/dblog/dblog.admin.inc
patching file modules/system/admin.css
patching file modules/system/system.admin.inc
Hunk #1 succeeded at 2042 (offset -4 lines).
Hunk #2 succeeded at 2172 (offset -4 lines).
patching file modules/update/update.report.inc
Hunk #1 FAILED at 45.
Hunk #2 FAILED at 54.
2 out of 2 hunks FAILED -- saving rejects to file modules/update/update.report.inc.rej
patching file themes/garland/style-rtl.css
patching file themes/garland/style.css
#14
Here's a minimal patch to just fix the bug by setting an icon and class for each severity.
#15
This is a fairly minor bug but, I'm getting tired of debug logging messages themselves being buggy. So upping severity a notch in hopes of someone taking a look.
#16
This also applies cleanly on head.
#17
Committed to CVS HEAD. Thanks.
#18
I've also taken the patch at #14 and committed that to 6.x. That's simple enough to go to a stable branch. Thanks!
#19
Automatically closed -- issue fixed for two weeks with no activity.