Thank you for an example of using hook_watchdog(), but there is one issue which seems to be missed. Sometimes the $log_entry['message'] is a string with placeholders like '%file' and in the $log_entry array there is an array of substitutes for these placeholders, $log_entry['variables'].
So if doing just
'@message' => strip_tags($log_entry['message']),
the output string in the email message will be the
Parsed JavaScript file %file.
So I'm doing
...
if (is_array($log_entry['variables'])) {
$log_entry['message'] = t($log_entry['message'], $log_entry['variables']);
}
$params['message'] = t($params['message'], array(
...
in my hook_watchdog to avoid this, so the message ends up ok,
Parsed JavaScript file misc/jquery.js.
I've filed this issue here and posted it as a comment to the http://api.drupal.org/api/function/hook_watchdog/6 as seems the problem may be spotted by many people and some of them may not correctly identify its source as the hook_watchdog is a relatively new one. Please feel free to remove my comment there if needed.
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-720630-31.patch | 5.45 KB | tim.plunkett |
#27 | system-hook_watchdog_docs-720630-27.patch | 5.51 KB | kid_icarus |
#27 | interdiff.txt | 2.2 KB | kid_icarus |
#24 | system-hook_watchdog_docs-720630-23.patch | 5.45 KB | kid_icarus |
#24 | interdiff.txt | 2.8 KB | kid_icarus |
Comments
Comment #1
jhodgdonGood catch! This should be fixed in the Drupal 7 version of http://api.drupal.org/api/function/hook_watchdog/7 and then backported to D6. At that point, this user's comment http://api.drupal.org/api/function/hook_watchdog#comment-743 should be removed.
Comment #2
vito_a CreditAttribution: vito_a commentedThank you.
Also I've created a Textlog module which is at http://vito.ho.ua/en/content/textlog-module for logging system events to file, as a demonstration of possibilities of the hook_watchdog() usage and also the tiny and lightweight replacement option for watchdog (dblog for Drupal 6) module. I'm attending to make a drupal.org release, they are reviewing my query now.
Also seems I can't delete a http://api.drupal.org/api/function/hook_watchdog#comment-743 comment myself as seems I'm only allowed to edit it. Can someone having the proper permissions for doing that help me?
Comment #3
jhodgdonWhy do you need textlog -- can't you just use the core syslog module?
Anyway, let's wait on deleting the comment until this issue has been fixed.
Comment #4
vito_a CreditAttribution: vito_a commented1. No, both core syslog and http://drupal.org/project/logging_alerts are only logging to the system log which typically resides at the /var/log/... . Users may not have access to it at different hostings and I need the small and quick module which may write to both the system log (if file exists, it can be pointed to and module will just append messages to it) and to the custom log file. Textlog does.
2. For core syslog there is no possibility to select which message levels(severity) to log. For example, I don't want to log all Notice's, I've selected to log only events with Warning severity and higher. I've added a configuration option for that to Textlog. However the http://drupal.org/project/logging_alerts has this option too, but it can't log to custom file what I need first of all.
3. I'm planning to do some email and IM integration and tagging to choose which messages to send to email and IM no matter that severity for these is lower then the border level selected at module configuration, what Dries mentioned in the http://drupal.org/node/63881#comment-407234 . I've faced the need for it and come to this idea separately, however I'm not suprised that Dries come to that idea more than 3 years ago as it is Dries :)
4. I'm also considering to offer a patch to the http://drupal.org/project/logging_alerts or core syslog.module doing the above for coming Drupal 7 release if that would be more effective. However applying a patch and especially patching core seems to be a long process, and I don't see why users who want to have the custom logging module today shouldn't have it, as I do.
It is the http://drupal.org/node/720720 where trial goes at :)
Comment #5
jhodgdonThis issue still needs a patch. Looking at watchdog(), you can see that there are a few components such as 'variables' that are passed to hook_watchdog() that are not listed in the hook_watchdog() documentation.
Seems like a good Novice project to add missing components...
Comment #6
tsphethean CreditAttribution: tsphethean commentedI've attached a patch to add in the missing variables definition from the hook_watchdog documentation. All other variables passed to hook_watchdog appear to be already included.
Comment #7
jhodgdonAgreed, that was the only missing piece, thanks!
So... I think the added sentence could use a bit of grammatical/punctuation cleanup to make it read a little easier, but it has the information to get the point across.
It would also be great if this patch could fix up the list formatting in the documentation block. See standards at:
http://drupal.org/node/1354#lists
(for instance, the constants in severity should be formatted as a list, and there are a few list items that currently do not end in .).
Comment #8
tsphethean CreditAttribution: tsphethean commentedThanks - I've modified the formatting to meet the standards (I hope).
I decided that the description of the variables array was made easier by providing more detail on the "message" variable, so I've used a copy of the same variable from the watchdog() function documentation which I think now makes more sense.
As an aside - is there an easy way to test the doxygen generation locally?
Comment #9
jhodgdondoxygen locally - yes, it's http://drupal.org/project/api and there are links to docs from there.
patch - looks pretty close!
A couple of things to bring it into compliance with the standards:
a) These lines:
need to have colons (and one space after colon):
Hmmm... Probably they shouldn't have two colons actually, so maybe:
Check and see what watchdog() is doing (hopefully it is formatted correctly; if not it wouldn't hurt to patch both).
b)
This line needs to end in : because the next line starts a sub-list.
c) I pretty much like what you did with messages/variables, but I dont' think it can be just copied directly from watchdog(). The reason is that on watchdog() they are $messages/$variables, and here they are array elements on one input parameter. So this doesn't quite work:
- Shouldn't say $message, $variables.
- last line should end in .
Comment #10
tsphethean CreditAttribution: tsphethean commentedCool thanks - revised patch incorporating comments attached.
Comment #11
jhodgdonLooks good! I'll get it committed soon. Thanks!
Comment #12
jhodgdonUh oh. I just noticed that the severity: line goes past 80 characters... maybe a couple of the other lines too. No lines should wrap past 80 characters (including initial whitespace and *), see
http://drupal.org/node/1354#general
Comment #13
tsphethean CreditAttribution: tsphethean commentedNo problem - revised patch attached.
Comment #14
tsphethean CreditAttribution: tsphethean commentedComment #15
Lars Toomre CreditAttribution: Lars Toomre commentedSince type hinting is recommended for D8 patches, shouldn't we include it in this example? I think the line should be modified to:
Comment #16
tsphethean CreditAttribution: tsphethean commentedSounds sensible, thanks.
Comment #17
jhodgdonThanks!
This patch is pretty close. I did notice some trailing whitespace that needs to be fixed though, such as in this line near the top:
Actually, it looks like just about all the lines in the patch have trailing whitespace. Check your editor settings -- most good code editors have a setting that highlights and/or removes trailing whitespace. :)
There are also a couple of minor grammar/punctuation things that could be fixed (and were likely in the original text):
- comma splice (change comma to semicolon):
Do not use 'debug', use severity WATCHDOG_DEBUG instead.
- The text for the WATCHDOG_* constants could be more consistent. Sometimes it says "conditions", and sometimes "condition", and there is a lot of repetition like "Error, error conditions" that seems silly.
- This final sentence "Set to NULL..." needs a rewrite (needs some "the" and maybe another "is" added, to make it into decent English):
And this could be made into an @link:
+ * - severity: One of the following values as defined in RFC 3164
+ * http://www.faqs.org/rfcs/rfc3164.html:
Comment #18
kid_icarus CreditAttribution: kid_icarus commentedThis addresses #17. I'm not sure if the text for the WATCHDOG_* constants is acceptable, but in my opinion it reads better than before.
Comment #19
jhodgdonLooks good! I think the constants' wording is now fine.
I found one more typo this reading (from before):
I think that should be "that referred the user to..." right?
Also, @link ... @endlink needs to be all on one line (it is OK to violate the usual wrapping conventions in order to ensure this):
Oh.... and one more thing I just noticed.... This is the documentation for hook_watchdog(). So it shouldn't be telling people what to *set* values to (that documentation belongs in the watchdog() docs, not here). It's really telling people who are writing modules to do logging what they'll be getting passed to them. So we need to change wording like these lines to that orientation, or just remove them:
Comment #20
jhodgdonAnd should we also put the better watchdog constant docs into the watchdog() docs? :)
Comment #21
kid_icarus CreditAttribution: kid_icarus commentedAddresses #19 and #20.
@jhodgdon Thanks for the review! I really appreciate the feedback.
A couple of questions if I may:
This edit looks correct to me, as the 'as' fits within the 80 character limit, but perhaps I am wrong?
I lowercased the word 'request', because I don't think it's a proper noun. Is this a correct edit?
Comment #22
kid_icarus CreditAttribution: kid_icarus commentedComment #23
jhodgdonThanks! This patch mostly looks good, and the answers to your questions are that you have the right ideas.
A few things remain to be fixed:
a)
This change should not be made. See http://drupal.org/node/1354#hooks
b)
Move the @link to the next line, so neither line goes over 80 characters.
c) In the last line:
I think we can remove the "Defaults to NULL" here. Again, that's for people calling watchdog(), not hook implementers. And here:
I think we can remove that second sentence.
d) I think we need to make similar improvements to the WATCHDOG_* area in the function doc for watchdog().
Comment #24
kid_icarus CreditAttribution: kid_icarus commentedThanks for the latest review @jhodgdon :)
This patch addreses #23. Thanks for the tip on the @link. My initial reasoning for @link to go over 80 characters lied in visualising how the link would look when output as HTML :)
Comment #25
kid_icarus CreditAttribution: kid_icarus commentedComment #26
jhodgdonIn the watchdog() part of the patch, the list indentation is not correct, and maybe it should start with "The severity of the message; one of the following..." (should the hook_watchdog() one start with that too??):
Other than that, looks great, thanks!
Comment #27
kid_icarus CreditAttribution: kid_icarus commentedThis addresses #26. I also added a couple of minor changes and I'm unsure if they're correct.
I added a colon after
RFC 3164
since the next line is the beginning of a sub-list.+ * - WATCHDOG_NOTICE: (default) Normal but significant conditions.
I added
(default)
per list conventions.Comment #28
kid_icarus CreditAttribution: kid_icarus commentedComment #29
jhodgdonLooks good, thanks!
Comment #30
jhodgdonCommitted this to 8.x, thanks all! It didn't apply to 7.x, so it needs a backport.
Comment #31
tim.plunkettIt didn't cleanly apply because of const vs define.
I left out the data type in @param.
Comment #32
jhodgdonLooks good, ready for commit -- thanks for the reroll!
Comment #33
jhodgdonCommitted to 7.x too! Thanks all!