Posted by bjaspan on October 7, 2009 at 10:02pm
14 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
watchdog() passes the global $user object in the log record. dblog.module assumes $user is defined when watchdog is called:
<?php
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.
Comments
#1
This adds an isset check to the uid. If not set, it assigns uid 0.
#2
The 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.
#3
#4
+ // The user object may exist in all conditions, so 0 is substituted if needed.
You're missing "NOT".
#5
good catch! thanks.
#6
Hrm. 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…)
#7
isset seems to tolerate a non-existant $log_entry['user']. I also have no idea how to reproduce the actual issue though.
#8
@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.
#9
#10
#11
Committed to 8.x and 7.x. Thanks.
#12
Untagging since it's been committed to 7.x
#13
This 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.
#14
I 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.
#15
The last submitted patch, 598586-watchdog-13-2.patch, failed testing.
#16
Things 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.
#17
The last submitted patch, 598586-watchdog-16.patch, failed testing.
#18
#16: 598586-watchdog-16.patch queued for re-testing.
#19
The last patch is for 8.x. I looked at it and to me it looks ready for commit.
#20
Looks good. Committed/pushed to 8.x, moving to 7.x for backport.
#21
I applied the patch in 7.x then did a git diff, it worked without a hitch so here is your backport ^^
#22
#23
It has the correct number of fingers and toes, and no D8-specific anythings => RTBC. Thanks @Zgear!
#24
Committed and pushed to 7.x. Thanks!
#25
I'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
#26
Shouldn't we just create a follow up issue for that?
#27
Regarding #26, @gpk is correct. Proper documentation is one of the core gates:
http://drupal.org/core-gates
#28
Here's a first attempt.
#29
This looks correct to me.
#30
Committed to 8.x. Thanks. Moving to 7.x.
#31
Rerolled to fix offset just in case.
#32
Committed to 7.x. Thanks!
http://drupalcode.org/project/drupal.git/commit/4c6faf1
#33
Automatically closed -- issue fixed for 2 weeks with no activity.