Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The $message argument to watchdog is intended to be a text replaceable string. The help says "Keep $message translatable by not concatenating dynamic values into it! Variables in the message should be added by using placeholder strings alongside the variables argument to declare the value of the placeholders.".
Yet, there are only three places in core that use watchdog without replaceable arguments, which prevent an alternative watchdog implementation from aggregating results by query string. This is important because it prevents watchdog implementors from using the $message itself as a unique string representing the message. For example, every call to 'access denied' is logged as a separate message. What is a watchdog implementor wanted to group all these 'access denied' or 'page not found' messages together, without knowing that type is unique ... the type is often the module name, and you wouldn't want to necessarily say every watchdog in a module is unique.
I'm not sure that I've articulated this very well.
But these 3 watchdog's are preventing mongodb_watchdog from grouping log entries in a very efficient way.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 904994-rollback-D7.patch | 1.25 KB | Dave Reid |
| #13 | 904994-rollback-D7.patch | 1.57 KB | Dave Reid |
| #4 | watchdog-constant-strings.patch | 2.32 KB | chx |
| #1 | watchdog.patch | 1.64 KB | douggreen |
| watchdog.patch | 1.64 KB | douggreen | |
Comments
Comment #1
douggreen CreditAttribution: douggreen commentedupdated patch with correct replaceable arguments
Comment #2
chx CreditAttribution: chx commentedQuite good fixes thanks.
Comment #3
chx CreditAttribution: chx commentedThere is more than alternative implementations, it was supposed that watchdog messages are translatable strings.
Comment #4
chx CreditAttribution: chx commentedFound another.
Comment #6
chx CreditAttribution: chx commented#4: watchdog-constant-strings.patch queued for re-testing.
Comment #7
chx CreditAttribution: chx commentedThat was some testbot fluke...
Comment #8
ygerasimov CreditAttribution: ygerasimov commentedI was not able to find any other places where watchdog was used without replaceable arguments, so patch #4 looks complete.
Comment #9
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #10
Dave ReidUm... this completely broke watchdog's 'top' 404 pages since all 404 reports contain the same message 'page not found: %q'. Try viewing admin/reports/page-not-found after a couple 404 pages have been hit. And I'm not sure how we can fix it since it requires a GROUP BY to distinguish per page.
Comment #11
Dave ReidComment #12
Dave ReidThis also broke redirect.module which uses the same kind of thing.
Comment #13
Dave ReidRolling back what broke core dblog and other contrib modules only.
Comment #14
Dave ReidUm... type is unique and will be the same for these. I don't see what the problem with the pre-patched behavior is.
Comment #15
chx CreditAttribution: chx commentedhttp://api.drupal.org/api/function/watchdog/7
Comment #16
Dave ReidAgreed that's the 'proper' thing to do, but this breaks functionality and I don't see an easy fix that won't require API changes which are *way* too late for D7. We need to just accept the fact that these two watchdog calls have to break the standard. I would like to figure out a better API to track 404s and 403s in D8. Right now using the watchdog table is the only way to extract that information.
Comment #17
Dave ReidPatch re-rolled with the whitespace fix removed since it's already in.
Comment #18
chx CreditAttribution: chx commentedSure. We can make exceptions based on message type. Edit: by we i mean mongodb watchdog, translator and others who want the api to keep its contract :) core can have a more proper fix in D8.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #22
fgmFWIW, this is still present in D8 today. Resetting to "fixed" just so followers can be notified.