The current watchdog message in aggregator.fetch.inc, aggregator_aggregator_fetch(), informs the user of an error by referring to the feed's title.
watchdog('aggregator', 'The feed from %site seems to be broken, due to "%error".', array('%site' => $feed->title, '%error' => $result->code . ' ' . $result->error), WATCHDOG_WARNING);
I've found that $feed->title can be null, leading to a PHP error. In addition, $feed->title is not as useful as $feed->url. Given $feed->title, a user needs to dig into configuration to find the feed's url in order to investigate why the feed is returning an error.
I propose the following change:
watchdog('aggregator', 'The feed from %url seems to be broken, due to "%error".', array('%url' => $feed->url, '%error' => $result->code . ' ' . $result->error), WATCHDOG_WARNING);
$feed->url is likely to exist and given the URL in the error message, a developer can more rapidly track down the issue causing the feed access to fail.
Comment | File | Size | Author |
---|---|---|---|
#37 | 1337898-37.patch | 3.65 KB | dcam |
#14 | 1337898-12.patch | 1.16 KB | naxoc |
#11 | 1337898-11.patch | 2.18 KB | pwolanin |
#7 | aggregator_watchdog_feed_error-1337898-1-D7.patch | 1.61 KB | jessebeach |
#6 | aggregator_watchdog_feed_error-1337898-1-D7.patch | 0 bytes | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedThe attached patch replaces $feed->title with $feed-url in the watchdog and dsm messages. This makes the message less likely to themselves create errors and it makes the error messages more useful to developers.
Just to elucidate the issue further, here's an example of the combo of log messages that are currently produced by the existing code:
The feed from seems to be broken, due to "400 Bad Request".; http://www.somesite.org; 1320864166; aggregator; ###.##.###.###; http://www.somesite.org/cron.php?cron_key=#######################-#####################; ; 0;
and this leads to
Notice: Undefined property: stdClass::$title in aggregator_aggregator_fetch() (line 57 of /mnt/www/html/tangle004/docroot/modules/aggregator/aggregator.fetcher.inc).; http://www.somesite.org; 1320864166; php; ###.##.###.###; http://www.somesite.org/cron.php?cron_key=#######################-#####################; ; 0;
Comment #2
jessebeach CreditAttribution: jessebeach commentedsetting to needs review.
Comment #3
mstef CreditAttribution: mstef commentedRerolled #1 to apply to drupal root - 7.x. Needed for drush make.
Comment #5
timhilliard CreditAttribution: timhilliard commented@mikestefff in future please add -D7 to the end or your patch name so that it is ignored by the test bot when the patch does not match the issue version number. i.e. aggregator_watchdog_feed_error-1337898-3-D7.patch
Changing to needs review
Comment #6
jessebeach CreditAttribution: jessebeach commented@mikesteff, I rolled the patch with -D7 at the end.
Comment #7
jessebeach CreditAttribution: jessebeach commentedThe old zero-byte patch! This one has content.
Comment #8
cweagansUpdating tags per http://drupal.org/node/1517250
Comment #10
pwolanin CreditAttribution: pwolanin commentedComment #11
pwolanin CreditAttribution: pwolanin commentedHere's a D8 re-roll.
Comment #12
pwolanin CreditAttribution: pwolanin commentedComment #14
naxoc CreditAttribution: naxoc commentedUsing the url for the error message makes much more sense.
Here is a reroll.
Comment #16
naxoc CreditAttribution: naxoc commentedFair enough testbot. Here is a patch with the test updated too.
Comment #17
jhedstromComment #18
kerby70 CreditAttribution: kerby70 commentedReroll attached.
Comment #19
mgiffordRe-uploading for the bots.
Comment #30
quietone CreditAttribution: quietone at PreviousNext commentedThe
aggregator
module has been removed from Core in10.0.x-dev
and now lives on as a contrib module.Issues in the Core queue about the
aggregator
module, like this one, have been moved to the contrib module queue.Comment #31
larowlanPatch needs update for contrib and uses long gone drupal_set_message
Comment #32
dcam CreditAttribution: dcam as a volunteer commentedRerolled #19
Comment #33
dcam CreditAttribution: dcam as a volunteer commentedThe test above failed for two reasons:
* The error message also comes from the DefaultParser, so that had to be updated too. I didn't find any additional instances of that error message.
* I'm assuming the drupal.org webserver redirects http:// traffic to https://. That included the bad URL in the test too. So the URL we gave it didn't match the updated URL after redirection. Changing it to https:/ corrected the problem.
Comment #34
dcam CreditAttribution: dcam as a volunteer commentedArray formatting
Comment #35
larowlanLooks good to me
Comment #37
dcam CreditAttribution: dcam as a volunteer commentedJust a quick 1.x port
Comment #39
dcam CreditAttribution: dcam as a volunteer commentedThe patch in #37 was committed to contrib Aggregator. I'm sending this back to the Drupal core queue to be backported and applied to 9.5.x, if anyone would like to do that. Otherwise, this is still tagged as needing a backport to D7.
Comment #40
catch9.5 is security only now.