watchdog() passes the global $user object in the log record. dblog.module assumes $user is defined when watchdog is called:
function dblog_watchdog(array $log_entry) {
Database::getConnection('default', 'default')->insert('watchdog')
->fields(array(
'uid' => $log_entry['user']->uid,
The problem is that watchdog() can be called after global objects like $user is destructed, for example when a global output buffer handler is invoked (boost.module does this). The result is a PHP Warning because $user->uid is undefined.
If watchdog() may not be called under these conditions, it should be documented as such. Otherwise, it should have a fallback for when $user is not defined.
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-598586-31.patch | 1.08 KB | tim.plunkett |
#28 | drupal-598586-28.patch | 1.1 KB | tim.plunkett |
#21 | 598586-watchdog-21.patch | 3.99 KB | Zgear |
#16 | 598586-watchdog-16.patch | 4.09 KB | dawehner |
#14 | 598586-watchdog-13.patch | 1.21 KB | matglas86 |
Comments
Comment #1
deekayen CreditAttribution: deekayen commentedThis adds an isset check to the uid. If not set, it assigns uid 0.
Comment #2
grendzy CreditAttribution: grendzy commentedThe code makes sense to me. It would probably benefit from a comment explaining that $user might not be there. Also it could be condensed as a ternary conditional.
Comment #3
grendzy CreditAttribution: grendzy commentedComment #4
Morbus Iff+ // The user object may exist in all conditions, so 0 is substituted if needed.
You're missing "NOT".
Comment #5
grendzy CreditAttribution: grendzy commentedgood catch! thanks.
Comment #6
Morbus IffHrm. Will this cause a NOTICE when we check for isset() on a non-existent object? If $user is being destroyed before watchdog gets to it, then $log_entry['user'] won't exist at all (right?). In which case, we might get a NOTICE for checking on ->uid when $log_entry['user'] isn't there either? (I don't have a reliable way to test/create the fail case this is fixing…)
Comment #7
grendzy CreditAttribution: grendzy commentedisset seems to tolerate a non-existant $log_entry['user']. I also have no idea how to reproduce the actual issue though.
Comment #8
deekayen CreditAttribution: deekayen commented@morbus since isset is a language construct, not a function and is designed to check for the the possible destruction caused by unset(), it and empty() I think are the only ways to safely check a non-existent variable without a notice.
In other words, it shouldn't cause a NOTICE if the variable isn't there.
Comment #9
Morbus IffComment #10
dawehnerComment #11
Dries CreditAttribution: Dries commentedCommitted to 8.x and 7.x. Thanks.
Comment #12
bfroehle CreditAttribution: bfroehle commentedUntagging since it's been committed to 7.x
Comment #13
gpk CreditAttribution: gpk commentedThis fixes the problem for dblog_watchdog(), but not syslog_watchdog(). In any case, is fixing the problem this way in hook_watchdog() the best fix for this? There seems not much point in including $user in $log_entry if $user might sometimes be empty, requiring workarounds. Also there is a difference between user anonymous and $user undefined, but maybe that's a nicety that's not worth capturing.
Comment #14
matglas86 CreditAttribution: matglas86 commentedI fixed it the same way as the previous patch does but now for
syslog_watchdog
.I looked at
hook_watchdog
. The only way I can think of doing this is adding a extra element uid to the array and use that in other methods and have user as a seperate object there.There is a example of that in the second patch.
Comment #16
dawehnerThings should first be fixed in drupal8 and then be backported to 7.x
Here is a patch which fixes the tests in the second patch, as i think fixing this in watchdog() is the right place to do it.
Though there were some problems with the test, as they uses raw db_watchdog which changes.
In general i think db_watchdog doesn't have to check that, because people should just use watchdog() though i'm not sure about that.
Comment #18
dawehner#16: 598586-watchdog-16.patch queued for re-testing.
Comment #19
matglas86 CreditAttribution: matglas86 commentedThe last patch is for 8.x. I looked at it and to me it looks ready for commit.
Comment #20
catchLooks good. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #21
Zgear CreditAttribution: Zgear commentedI applied the patch in 7.x then did a git diff, it worked without a hitch so here is your backport ^^
Comment #22
Zgear CreditAttribution: Zgear commentedComment #23
xjmIt has the correct number of fingers and toes, and no D8-specific anythings => RTBC. Thanks @Zgear!
Comment #24
webchickCommitted and pushed to 7.x. Thanks!
Comment #25
gpk CreditAttribution: gpk commentedI'm a bit out of the loop these days but would I be right in thinking that the new key 'uid' in $log_entry needs some documentation? No mention of it in http://api.drupal.org/api/drupal/core!modules!system!system.api.php/function/hook_watchdog/8
Comment #26
Zgear CreditAttribution: Zgear commentedShouldn't we just create a follow up issue for that?
Comment #27
xjmRegarding #26, @gpk is correct. Proper documentation is one of the core gates:
http://drupal.org/core-gates
Comment #28
tim.plunkettHere's a first attempt.
Comment #29
xjmThis looks correct to me.
Comment #30
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks. Moving to 7.x.
Comment #31
tim.plunkettRerolled to fix offset just in case.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x. Thanks!
http://drupalcode.org/project/drupal.git/commit/4c6faf1