hook_watchdog() doc/sample missing information

Good 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.

Why 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.

This 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...

I'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.

Agreed, 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 .).

Thanks - 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?

doxygen 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:

+ *     - WATCHDOG_EMERGENCY    Emergency: system is unusable.

need to have colons (and one space after colon):

+ *     - WATCHDOG_EMERGENCY: Emergency: system is unusable.

Hmmm... Probably they shouldn't have two colons actually, so maybe:

+ *     - WATCHDOG_EMERGENCY: Emergency, system is unusable.

Check and see what watchdog() is doing (hopefully it is formatted correctly; if not it wouldn't hurt to patch both).

b)

+ *   - severity: One of the following values as defined in RFC 3164 http://www.faqs.org/rfcs/rfc3164.html.

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:

+ *     be added by using placeholder strings alongside the variables argument to declare 
+ *     the value of the placeholders. See t() for documentation on how $message and 
+ *     $variables interact.
+ *   - variables: An array of variables to be inserted into the message on display. Set to NULL 
+ *     if message is already translated or not possible to translate. Defaults to NULL

- Shouldn't say $message, $variables.
- last line should end in .

Cool thanks - revised patch incorporating comments attached.

Looks good! I'll get it committed soon. Thanks!

Uh 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

No problem - revised patch attached.

Since type hinting is recommended for D8 patches, shouldn't we include it in this example? I think the line should be modified to:

  * @param array $log_entry

Sounds sensible, thanks.

Thanks!

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:

 * This hook allows modules to route log events to custom destinations, such 

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):

+ *     display. Set to NULL if message is already translated or not possible to
+ *     translate. Defaults to NULL.

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:

This addresses #17. I'm not sure if the text for the WATCHDOG_* constants is acceptable, but in my opinion it reads better than before.

Looks good! I think the constants' wording is now fine.

I found one more typo this reading (from before):

+ *   - referer: The page that referred the use to the page where the event
+ *     occurred.

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):

+ *   - severity: One of the following values as defined in @link
+ *     http://www.faqs.org/rfcs/rfc3164.html RFC 3164 @endlink

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:

+ *     is normally the module name. Do not use 'debug'; use severity
+ *     WATCHDOG_DEBUG instead.  
[I would remove this last sentence]

+ *   - message: The text of the message to be logged. Variables in the message
+ *     should be added by using placeholder strings alongside the variables
[Say something like "Variables in the message are indicated using placeholder..."]

+ *   - variables: An array of variables to be inserted into the message on
+ *     display. Set this to NULL if a message is already translated or if the
+ *     message is not possible to translate. Defaults to NULL.
[Say something like "Will be missing or NULL if ..."]

And should we also put the better watchdog constant docs into the watchdog() docs? :)

Addresses #19 and #20.

@jhodgdon Thanks for the review! I really appreciate the feedback.

A couple of questions if I may:

1. - * This hook allows modules to route log events to custom destinations, such
   - * as SMS, Email, pager, syslog, ...etc.
   + * This hook allows modules to route log events to custom destinations, such as
   + * SMS, Email, pager, syslog, ...etc.
   

   This edit looks correct to me, as the 'as' fits within the 80 character limit, but perhaps I am wrong?




2. - *   - request_uri: The Request URI for the page the event happened in.
   - *   - referer: The page that referred the use to the page where the event
   + *   - request_uri: The request URI for the page the event happened in.
   + *   - referer: The page that referred the user to the page where the event

   I lowercased the word 'request', because I don't think it's a proper noun. Is this a correct edit?

Thanks! 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)

--- a/core/modules/system/system.api.php
+++ b/core/modules/system/system.api.php
@@ -1923,31 +1923,40 @@ function hook_xmlrpc_alter(&$methods) {
 }
 
 /**
- * Log an event message
+ * Logs an event message.

This change should not be made. See http://drupal.org/node/1354#hooks

b)

+ *   - severity: One of the following values as defined in @link http://www.faqs.org/rfcs/rfc3164.html RFC 3164 @endlink

Move the @link to the next line, so neither line goes over 80 characters.

c) In the last line:

+ *     the message is not possible to translate. Defaults to NULL.

I think we can remove the "Defaults to NULL" here. Again, that's for people calling watchdog(), not hook implementers. And here:

+ *   - type: The type of message for this entry. For contributed modules, this
+ *     is normally the module name.

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().

Thanks 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 :)

In 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??):

- *   The severity of the message, as per RFC 3164. Possible values are
- *   WATCHDOG_ERROR, WATCHDOG_WARNING, etc.
+ *   One of the following values as defined in
+ *   @link http://www.faqs.org/rfcs/rfc3164.html RFC 3164 @endlink
+ *     - WATCHDOG_EMERGENCY: Emergency, system is unusable.
+ *     - WATCHDOG_ALERT: Alert, action must be taken immediately.
+ *     - WATCHDOG_CRITICAL: Critical conditions.
+ *     - WATCHDOG_ERROR: Error conditions.
+ *     - WATCHDOG_WARNING: Warning conditions.
+ *     - WATCHDOG_NOTICE: Normal but significant conditions.
+ *     - WATCHDOG_INFO: Informational messages.
+ *     - WATCHDOG_DEBUG: Debug-level messages.

Other than that, looks great, thanks!

This addresses #26. I also added a couple of minor changes and I'm unsure if they're correct.

1. +++ b/core/includes/bootstrap.inc
   @@ -1568,16 +1568,16 @@
   + *   @link http://www.faqs.org/rfcs/rfc3164.html RFC 3164: @endlink
   

I added a colon after RFC 3164 since the next line is the beginning of a sub-list.

2. + * - WATCHDOG_NOTICE: (default) Normal but significant conditions.

I added (default) per list conventions.

Looks good, thanks!

Committed this to 8.x, thanks all! It didn't apply to 7.x, so it needs a backport.

It didn't cleanly apply because of const vs define.

I left out the data type in @param.

Looks good, ready for commit -- thanks for the reroll!

Committed to 7.x too! Thanks all!