dblog expects debug icon and class [#188246]

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.

Comment	File	Size	Author
#14	dblog.patch	1.79 KB	mfb
#14
#12	drupal-diff-2008-02-01-09-04-24.patch	7.57 KB	deekayen
#12
#12	theme.inc-diff-2008-02-01-09-04-57.patch	1.22 KB	deekayen
#12
#10	watchdog-icons-refresh.png	6.76 KB	BrightLoudNoise
#1	drupal-diff-2007-10-31-14-29-59.patch	1.54 KB	deekayen
#1
#1	188246.png	50.22 KB	deekayen
#9	drupal-diff-2008-01-04-09-51-42.patch	7.55 KB	deekayen
#9
#9	theme.inc-diff-2008-01-04-10-08-25.patch	1017 bytes	deekayen
#9
#5	dblog-debug.png	168 bytes	deekayen
#5	dblog-info.png	168 bytes	deekayen
#5	dblog-notice.png	168 bytes	deekayen
#5	dblog-warning.png	339 bytes	deekayen
#5	dblog-error.png	799 bytes	deekayen
#5	dblog-critical.png	168 bytes	deekayen
#5	dblog-alert.png	168 bytes	deekayen
#5	dblog-emerg.png	168 bytes	deekayen
#5	1x1.png	161 bytes	deekayen
#5	drupal-diff-2007-12-20-13-15-00.patch	7.68 KB	deekayen
#5
#5	dblog-ok.png	3.14 KB	deekayen
#4	dblog.admin_.inc-188246-2.patch	1.94 KB	fp
#4
#2	dblog.admin_.inc-188246.patch	1.96 KB	fp
#2
	dblog.admin_.inc-diff-2007-10-31-14-08-51.patch	1.5 KB	deekayen

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

deekayen CreditAttribution: deekayen commented 4 January 2008 at 15:33

File	Size
188246.png	50.22 KB
drupal-diff-2007-10-31-14-29-59.patch	1.54 KB

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 $icons or $classes in dblog_overview().

Comment #2

fp CreditAttribution: fp commented 1 November 2007 at 01:00

File	Size
dblog.admin_.inc-188246.patch	1.96 KB

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

Comment #3

deekayen CreditAttribution: deekayen commented 1 November 2007 at 02:22

Status:

Needs review

» Needs work

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?

Comment #4

fp CreditAttribution: fp commented 1 November 2007 at 05:58

File	Size
dblog.admin_.inc-188246-2.patch	1.94 KB

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.

Comment #5

deekayen CreditAttribution: deekayen commented 20 December 2007 at 18:18

Status:

Needs work

» Needs review

File	Size
dblog-ok.png	3.14 KB
drupal-diff-2007-12-20-13-15-00.patch	7.68 KB

1x1.png	161 bytes
dblog-emerg.png	168 bytes
dblog-alert.png	168 bytes
dblog-critical.png	168 bytes
dblog-error.png	799 bytes
dblog-warning.png	339 bytes
dblog-notice.png	168 bytes
dblog-info.png	168 bytes
dblog-debug.png	168 bytes

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.

Comment #6

bdragon CreditAttribution: bdragon commented 4 January 2008 at 00:40

subscribing

Comment #7

Dries CreditAttribution: Dries commented 4 January 2008 at 08:22

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.

Comment #8

catch

he/him

English

CreditAttribution: catch commented 4 January 2008 at 11:14

Status:

Needs review

» Needs work

Comment #9

deekayen CreditAttribution: deekayen commented 4 January 2008 at 15:19

Status:

Needs work

» Needs review

File	Size
theme.inc-diff-2008-01-04-10-08-25.patch	1017 bytes

drupal-diff-2008-01-04-09-51-42.patch	7.55 KB

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 output img tags 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 a dblog- 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.

Comment #10

BrightLoudNoise CreditAttribution: BrightLoudNoise commented 23 January 2008 at 23:05

File	Size
watchdog-icons-refresh.png	6.76 KB

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.

Comment #11

BrightLoudNoise CreditAttribution: BrightLoudNoise commented 31 January 2008 at 18:44

Status:

Needs review

» Needs work

@Deekayen: Patch no longer applies cleanly to update.report.inc, could you re-roll?

patching file modules/dblog/dblog.admin.inc
patching 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

Comment #12

deekayen CreditAttribution: deekayen commented 1 February 2008 at 14:09

Status:

Needs work

» Needs review

File	Size
theme.inc-diff-2008-02-01-09-04-57.patch	1.22 KB

drupal-diff-2008-02-01-09-04-24.patch	7.57 KB

Same as #9, just fresh rolls for #11.

Comment #13

BrightLoudNoise CreditAttribution: BrightLoudNoise commented 5 February 2008 at 16:23

Status:

Needs review

» Needs work

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.patch
patching 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

Comment #14

mfb

they or he

CreditAttribution: mfb commented 30 March 2008 at 20:20

Status:

Needs work

» Needs review

File	Size
dblog.patch	1.79 KB

Here's a minimal patch to just fix the bug by setting an icon and class for each severity.

Comment #15

mfb

they or he

CreditAttribution: mfb commented 8 May 2008 at 07:11

Priority:

Normal

» Critical

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.