Aggregator files two similar error messages for not parseable feeds

Thanks M. I've committed this patch to CVS HEAD (Drupal 7). It failed to apply against DRUPAL-6 though so I'm changing the version number.

I'm hesitant to mark this patch RTBC because I'm a patchtest noob, but this one seems to be a no-brainer. I've tested it against D6. It eliminated the double reporting of feed errors in my logs and I've seen no further issues.

Hm. Before the patch, the code structure is as follows:

  Do drupal_http_request
  switch (on result code) {
    if 304: no new content. always break!
    if 301: record new url and fall over!
    if 200, 302 or 307: try to parse and if not possible, fall over; if possible, break!
    otherwise: report error;
  }

So this ensures that if there is a new URL, or there was some response, but the data was not parseable, there is an error reported. Also, it reports an error, if the status code is other then 304, 301, 200, 302 and 307. For example, if the code is a 404.

From the patch, I see, it moved the break around, so what it does is:

  Do drupal_http_request
  switch (on result code) {
    if 304: no new content. always break!
    if 301: record new url and fall over!
    if 200, 302 or 307: try to parse; <strong>always break!</strong>
    otherwise: report error;
  }

Looks, I've emphasized your change. So this breaks the cycle of possibly parsing data when the feed was a 301 redirect, which was the intention of the original code. Also, as far as I see, it breaks reporting errors when there was a 200, 302 or 307 but the feed was not parseable.

I assume I might understand something wrong, since you clearly state that the patch makes one of two error messages appear. You don't hovewer present examples of those two error messages, and the one which still appears. So what I can see from here, is that you stepped over error reporting in many cases with this patch. But where is that actual error reporting done now I don't see; drupal_http_request() does not do error reporting itself and this aggregator_refresh() function is called from aggregator_cron() which does not care about any return values or errors produced inside aggregator_refresh().

Right, I've reviewed the patch again, and it looked like doing what it is advertised to do :) Committed.