In the current state of the aggregator, an invalid token error results in the entire source feed going offline until the token error is removed from the source. The aggregator should have a softer response to these errors, such as replacing or dropping the bad token or dropping the item rather than dropping the entire source.

This comes from a forum discussion a little while ago: http://drupal.org/node/18788 where someone pointed out (rightly) that these errors result when a source feed contains a character that's not defined in the specified character set -- usually an accented character. Since the feed doesn't conform to the spec, it's not well formed and the aggregator drops it, refusing to load any new items from that feed.

Unfortunately this sucks big time for users who may depend on the news from that particular feed and aren't even notified that it has a problem. One stray é makes an entire news source totally invisible, perhaps for weeks at a time depending on the rate of news posts, and by the time that one character clears the feed the rest of the news might be completely out of date or at least pushed so far down the aggregator date queue that users miss it. The error is on the feed source end, but it's my site's users who suffer.

What would be a better situation is if the aggregator could take this error condition and deal with it in a smart way, perhaps escaping the bad character, or replacing it with a bullet, and loading the news item -- it's better for my users to see one bad character in an important piece of news rather than miss a dozen postings. Or, if perhaps this can't be done, the next best thing would be for the aggregator to skip the bad news item and load the rest of the items in the feed -- perhaps putting an error message into the news itself (rather than just the watchdog) so that users would know they've missed something and if they think it might be important they can check the source themselves.

In short, if my news feeds are constantly going offline and coming back without warning, for whatever reason, my users will decide that my news aggregator can't be depended on to bring them the news. They'll either use a different aggregator or go directly to the site, and either way I've lost the reader's trust. So, marked critical because if my users can't trust my news, they can't trust my site and thus my site's news is broken.

Comments

robin monks’s picture

robin monks commented
Assigned: Unassigned » robin monks

I've been doing some work in aggregator as of late, so I'll work on this. I doubt it's a very big problem.

Robin

Steven’s picture

Steven commented

The problem is not that it contains invalid /characters/ but that it is encoded incorrectly. This is a significant difference. Usually a feed is indicated as UTF-8 when it is not really UTF-8.

Here's a UTF-8 validator which satisfies the classic UTF-8 test page:
http://www.acko.net/dumpx/validate_utf8.phps

But I don't see why we should put something like this into core. XML says the parser should die when the feed is encoded incorrectly. Site owners should fix their feeds, end of story.

Steven’s picture

Steven commented
Priority: Critical » Minor

Oh and the Priority field should not be used as a personal urgency indicator. This issue affects a very small amount of sites, namely those that want to aggregate broken fields.

Greg Delisle’s picture

Greg Delisle commented

Steven, if you're volunteering to personally nag every site operator who puts out an incorrectly-encoded feed, then send me your contact info and I'll start sending you a list of sites to call. But seriously, we live in a world where things don't always conform to spec, and the Drupal should function in that one. Other aggregators handle improper character encoding without going paws-up, why can't ours be just as good? If you don't think this matters, then perhaps Drupal's aggregator doesn't interest you, which is fine, but don't act like it's not a problem for me just because it's not a problem for you. And I didn't mark it critical because of "personal urgency", I think you realize that. Perhaps it's not critical, but there's no need to be rude.

Thanks Robin for looking into it.

killes@www.drop.org’s picture

killes@www.drop.org commented

We've had that dicussion more than once. Drupal has no business in catering for other people's broken software. Get the clue bat out, buy some extra spikes, and clue those people in.

Steven’s picture

Steven commented

So, marked critical because if my users can't trust my news, they can't trust my site and thus my site's news is broken.

Sorry, but this sentence made it sound to me like you were marking it critical exactly because your site was broken: "my my my my". The priority indicator is important for keeping track of bugs, and a lot of people have a tendency to mark something as critical just because it is urgent for them. It is annoying for everyone else, which is why I react strongly to abuse.

Back to the issue at hand... in the words of Joel Spolski: "It's not that hard" (emphasis his, talking about Unicode and encodings). Experience shows that ignoring encodings and trying to cater to broken implementations of them only leads to more headaches and a decreased awareness of the problem in general. The encoding mess surrounding Trackbacks is a perfect example of that. It is something which I do not want to see repeated elsewhere.

If you want your aggregator to aggregate broken feeds, drop in my UTF-8 validator which I wrote for people just like you. I spent a significant amount of time on it to make sure it strips out invalid UTF-8 in the most correct way possible. However, one thing it will not do is guess at encodings and try to output text as intended rather than as encoded, as this encourages lazyness on the side of the implementors.

I have already spent way more time than anyone else fixing PHP's broken XML parser, so the last thing you can accuse me of is not being interested.

Greg Delisle’s picture

Greg Delisle commented

Steven, I tried integrating your UTF-8 validator code but it tied my aggregator in knots. In poking around there, though, I did find the drupal_convert_to_utf8() function in common.inc that appears to do this already. I stuck this code into aggregator.module as a workaround:
$encoding = 'utf-8';
if (ereg('^<\?xml[^>]+encoding="([^"]+)"', $data, $match)) {
$encoding = $match[1];
}
$data = drupal_convert_to_utf8($data, $encoding);

I put this into the aggregator_parse_feed() function just before the parser is created. When it runs, I get the lovely "cannot modify header information" error, so there must be something wrong with the way I'm doing it, but the good news is that it parses those broken feeds with no trouble.

If this function is already in core, then, what's the big deal about aggregator calling this function? I thought this whole discussion hinged on creating a new feature to do this, but it appears it's already there. Can somebody help me make a proper patch for this?

Steven’s picture

Steven commented

The behavior of drupal_convert_to_utf8() with $encoding set to "utf-8" depends on which conversion library is used.

- mbstring (mb_convert_encoding) simply removes illegal bytes. Though there is no indication if something went wrong, it will parse XML correctly.
- iconv (iconv) chops off the data at the first illegal byte. Using this for parsing XML is out of the question, as chopping off an XML file will break it.
- I don't have a working install with recode compiled in, but I recall something about it sharing traits with iconv. It is likely that it will have the same behaviour.

As iconv() is the default encoding mechanism in PHP5, your patch won't help in the majority of cases. As for PHP4, we can't depend on any of the conversion libraries being available: in that case, Drupal can only parse UTF-8 / US-ASCII / ISO-8859-1 (still better than nothing). Your patch breaks that fallback situation too.

Finally, if you convert the data, you need to change $encoding too, otherwise you will no longer be able to parse non-UTF-8 feeds.

Greg Delisle’s picture

Greg Delisle commented

Aha, I think I see why it's not on by default. I'm running php4 with mbstring built in, which is why it works for me, but Drupal core can't count on that. Shame that we can't turn it on in the interface somewhere, since it's sitting there in common.inc not being used for anything.

As for the encoding, that gets set to utf-8 by the drupal_xml_parser_create() function, so if you just change aggregator.module to call that one instead, and remove the condition on converting to utf-8, you get a feed that's always converted and always re-encoded. So, it should be possible to check for the presence of the mbstring module, and if it's present, always convert feeds to utf-8. Seems to be working for me.