Move out rendering image from aggregator parser

It's wrong what gets saved to the database in the first place, because {aggregator_feed}.link is not populated with the channel's <link> (or image<link>), but instead with the feed URL (that is already stored in {aggregator_feed}.url). {aggregator_feed}.description is not populated at all.

The RSS 2.0 Specification lists the following required elements for <channel>:

• title
• link
• description

Image is an optional sub-element of <channel>, which contains another three required sub-elements.

• url
• title
• link (Note, in practice the image <title> and <link> should have the same value as the channel's <title> and <link>)

<?xml version="1.0" encoding="utf-8"?>
<rss version="2.0"
  <channel>
    <title>RSS Title</title>
    <link>http://www.example.com/</link>
    <description>This is an example of an RSS feed</description>
    <image>
      <url>http://www.example.com/feed-logo.png</url>
      <title>Image title</title>
      <link>http://www.example.com/</link>
    </image>
 
    <item>
      ...
    </item>

    <item>
      ...
    </item>
 
  </channel>
</rss>

On the other hand there are the following fields in {aggregator_feed} table:

fid            Primary Key: Unique feed ID.
title          Title of the feed
url            URL to the feed.
refresh        How often to check for new feed items, in seconds.
checked        Last time feed was checked for new items, as Unix timestamp.
queued         Time when this feed was queued for refresh, 0 if not queued.
link           The parent website of the feed; comes from the <link> element in the feed.
description    The parent website’s description; comes from the <description> element in the feed.
image          An image representing the feed.
hash           Calculated hash of the feed data, used for validating cache.
etag           Entity tag HTTP response header, used for validating cache.
modified       When the feed was last modified, as a Unix timestamp.
block          Number of items to display in the feed’s block.

1. First we need to populate {aggregator_feed}.link with the <channel><link> (which should be identical to <image><link>) and NOT the feed URL.
2. The HTML link containing the image (stored in {aggregator_feed}.image) is correctly build using the sub-elements of <image>. If we want to move the rendering out from aggregator.parser.inc I think we can just ignore the sub-elements of <image> and use the corresponding channel elements, which should have the same value (see definition).

Restored the check that all necessarry parts aren't empty before rendering the image link.

More elaboration. This patch also fixes a bug that became obvious reviewing the code. I don't know if this should be a seperate issue:

The channel's link and description are not saved to the database because of uppercase keys:

-    $feed->link = !empty($channel['LINK']) ? $channel['LINK'] : '';
-    $feed->description = !empty($channel['DESCRIPTION']) ? $channel['DESCRIPTION'] : '';
+    $feed->link = !empty($channel['link']) ? $channel['link'] : '';
+    $feed->description = !empty($channel['description']) ? $channel['description'] : '';

The result is that the description is always empty and instead of the channel's link the feed url gets saved (aggregator_refresh, line 600, aggregator.module).

Fix #4 and add a CSS class .feed-image to the image link.

Bingo bango!

It is absurd to build the link in the parser. I would also agree with setting the link and title of the image to the feed's link and title. In fact, it would be rather strange behavior if the link on the image went to a different place.

I'm guessing this isn't a candidate for back porting, which is a shame.

Here is an updated patch that just removes a trailing space in #5.

Should this be backported to D7 as well?

Committed to 8.x.

Moving to 7.x but not 100% convinced it should be backported. Unless I'm mistaken, this could break existing templates.

The only markup affected in the patch is adding back the feed-image class to the link. This class existed in 6.x, but is missing in 7.x. However, the class is referenced in aggregator.css. It only affects the bottom margin, so I highly doubt that it would break themes, unless the theme was using that class elsewhere.

The real issue is that this patch changes what is stored in the database. Previous to the patch, <a href="http://path/to/site/"><img src="http://path/to/image" alt="Feed title" /></a> was stored in the image field. After the patch, http://path/to/imge is the only thing stored. This would break existing feed images until the feed was updated. Although, an update_N could be easily added that replaces the markup with the image source.

This patch also fixes the feed description and link being saved properly. Those are existing bugs in 7.x.

Bumping down to 7.x as per #9.

What's the bast practice for update_N to replace the markup with the image source?

A regular expression that extracts the img path?

This is safe for backport to D7 IMO. This patch just moved where the link was being created to the preprocess function, so the template itself is unaffected. The small difference in the markup is that a class has been added, but since it's a new class, it should be harmless.

I've added the backport tag, but it's obviously up to you guys, so feel free to remove if you disagree. :)

#12, The issue is that the storage of feed images changes.

#11, Eww, regular expressions. I have no idea what best-practices would be, but I would do the following to grab the image src.

$dom = new DOMDocument();
$dom->loadHTML('<a href="http://path/to/site/"><img src="http://path/to/image" alt="Feed title" /></a>');
$xpath = new DOMXPath($dom);
$node_list = $xpath->evaluate('//img/@src');
$image_src = $node_list->item(0)->nodeValue;

Cross post?

@twistor Ha, yes definitely a cross post. Whoops. I was only responding to #9.

Cool, thanks for the walkthrough in #12.

Committed and pushed to 7.x. Thanks!