An issue I filed last year (http://drupal.org/node/29812) didn't get any attention. Maybe because it's filed on the ping module when the real problem is with cron.php.
The summary of the problem I discussed above is: I had an issue where my pings weren't being sent out to aggregators. There was some error in the underlying xmlrpc code. That error made the ping's crap out and not happen. The error was caused by faulty (by me) updating of my sites code.
HOWEVER ... the problem is that cron.php doesn't capture errors on this line of code:
module_invoke_all('cron');
Instead the error that had been occuring went to the stdout of the wget that was executing from cron. That stdout went to nowhere land.
If I had been notified of the error appropriately ... e.g. if the error had been captured and put into the watchdog log ... then I would have been able to immediately see the problem and fix it. Instead I had to spend several weeks scratching my head over what could be going wrong when there's no error indication that I could find.
The request is to rewrite that line something like:
try {
module_invoke_all('cron');
} catch (Exception $e) {
watchdog('cron', t('cron failed because ' . $e->getMessage()), WATCHDOG_ERROR);
}
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 77706_try_cron_w_test.patch | 2.99 KB | deekayen |
| #10 | 77706_try_cron_w_test.patch | 2.99 KB | deekayen |
| #8 | 77706_try_cron.patch | 753 bytes | deekayen |
Comments
Comment #1
magico commentedAfter solving this issue and (if needed) backport to 4.6.x according to this http://drupal.org/node/29812
Comment #2
magico commentedI don't think that having a catch exception is the best solution. Modules that implement the cron should evaluate their errors and then return a status or a message.
I know this is not critical, but there is no "high" priority... :(
Comment #3
eaton commentedDowngrading to non-critical. This is not a show-stopper for 5.0; it's a good thing to fix, but not critical.
Comment #4
Tresler commentedfeature instead of bug - this isn't throwing an error, just not catching it properly.
Also, please supply a patch if you have a proposed solution, if possible. http://drupal.org/patch
Comment #5
reikiman commentedIt may be too early to fix this. At the time I made the report my knowledge of PHP was sketchy, and since then I learned the try/catch paradigm is new in PHP5. But of course there's this adoption problem where PHP4 is still prevalent. I, for example, have not transitioned from PHP4. I know, I know, bad webmaster... I'm planning the transition but it seems to require moving my server to a newer OS as well.
I see 'magico' replied earlier suggesting a try/catch wasn't so good of an idea. I think while it's a good idea for modules to catch and record/evaluate their own errors, that at the same time some errors will slip through. This isn't Java where the exceptions are clearly delineated and the compiler helps you make sure you've caught all possible exceptions. This is PHP where everything is loosey goosey and there could very easily be exceptions you can't know about which will wind back to cron.php and then disappear into the ethers.
I can't offer a patch today because I have no way to test it (no PHP5). Sorry, but my proposed rewrite in the initial report will have to do for now. At least it's very straightforward.
Comment #6
reikiman commentedIt may be too early to fix this. what I meant is, since the Drupal community is not yet ready to draw a line in the sand and require PHP5 it's not appropriate to require PHP5isms in cron.php. Or else we could publish a cron.php5 which people can use instead.
Comment #7
Tresler commentedAh... well, as I understand it, drupal 7 will be php5 only - and drupal 6 just went beta. So... seems like it will work out just fine. Thanks.
Comment #8
deekayen commentedHere's the original request in patch form, updated for the newer watchdog() args.
Comment #9
alexanderpas commentedneeds tests (if possible), otherwise RTBC
Comment #10
deekayen commentedSince the test throws an error and had to run a cron, I created another test module. Through the test, I discovered that the watchdog call during the exception doesn't translate !, @, % replacements into the second parameter of watchdog for the built-in t(). Instead, what I did is wrap the error string in check_plain(). I know, it's probably never been done before, but I'm not sure how else to do it. I figure at least this way it's scrubbed and recorded as an error.
Comment #11
alexanderpas commentedwhy did you choose to use
assertTrue($count > 0,instead ofassertTrue($count == 1,?Comment #12
deekayen commentedBecause I just wanted to make sure the error is there. If someone makes the same error message in a test elsewhere, it'd break the test with having more than one entry. Moreover, this patch catches errors on a hook with another hookable call, so we can't necessarily predict that someone won't invoke hook_watchdog as part of logging an exception to the cron which invokes a cron of its own that could in turn fail and have another exception logged. Yes, complicated, unlikely scenarios, but possible nonetheless I think.
Instead, I could just do a TRUE/FALSE instead of COUNT() by figuring out the DBTNG equivalent to
SELECT 1 FROM {watchdog} WHERE type='cron'..., but I'd rather not because that isn't a common query and would then probably be classified something hard to read.Comment #13
alexanderpas commentedIf someone makes the same error message in a test elsewhere, it should not matter, as this is handled in a child installation.
also, if there is more than 1 message, while we only put this one in, there is something wrong... which might be a valid reason for the test to fail.
Comment #14
deekayen commentedThis version looks for 1 and only one instance of the expected exception message.
Comment #15
alexanderpas commentedComment #16
dries commentedI'm not sure -- it sounds like we need to move the error handling closer to where the problem happened. From the original description, it sounds like there is a problem in aggregator module that is not properly handled.
Comment #17
deekayen commentedMy understanding is that reikiman hacked up the module on his own, introduced a bug, and then wasn't able to see what the problem was because cron didn't report an error. I don't think there's any additional review of the aggregator module's error handling that can prevent people from introducing bugs with their own custom hacks.
Comment #18
reikiman commentedRight the issue here isn't the original bug I made. The issue is that errors are being dropped on the floor and are going unseen. cron.php is usually called by wget and the stdout output thrown away. In my case there was a FATAL ERROR which went unseen, but if FATAL ERROR had been entered in a log file somewhere I would have taken more notice.
Dries suggested moving error checking closer to the problem. Yes error checking close to a problem error can result in better error messages. The patch I suggested can only give a generic message because of where it is.
The flip side of that is requiring error trapping and reporting everywhere that an error could occur.
With the move to requiring PHP5+ maybe it's time to scatter more try{}catch{}'s around Drupal for this very reason, to give better error handling and reporting. But adding them "everywhere" would lead to code bloat, right? And you're unlikely to put catch{}'s everywhere, leaving a few uncaught exceptions remaining uncaught. Hence the catch{} in cron.php is still a good idea to report uncaught exceptions.
EDIT: yahoogling "uncaught exception" leads to lots of results indicating catching and reporting them is a Best Practice
Comment #19
deekayen commentedMaybe just wrap core's instances of module_invoke_all in try/catch and stop there?
Comment #20
alexanderpas commentedI still like to see this in, as cron does not have any other way of reporting nasty errors.
Comment #21
dave reidMoving to the new cron system component.
Comment #22
MichaelCole commented#14: 77706_try_cron_w_test.patch queued for re-testing.
Comment #24
eric115 commentedLooks like this has since been resolved in Drupal 7 core and also given the age of this issue, I am going to close it.
https://github.com/drupal/drupal/blob/fada06694312023535909764524ab43c99...