It's kind of nonsense that rendering image happens in, see this:, line 39.

$image = l(theme('image', array('path' => $image['url'], 'alt' => $image['title'])), $image['link'], array('html' => TRUE));

Only the path should be returned, and the link separately. Then, for example in template_preprocess_aggregator_feed_source(), instead of:
$variables['source_image'] = $feed->image;, the image can be assembled.

#7 move-out-rendering-image-from-aggregator-parser-1268234-7.patch2.17 KBderjochenmeyer
PASSED: [[SimpleTest]]: [MySQL] 33,302 pass(es). View
#5 move-out-rendering-image-from-aggregator-parser-1268234-5.patch2.18 KBderjochenmeyer
PASSED: [[SimpleTest]]: [MySQL] 32,862 pass(es). View
#3 move-out-rendering-image-from-aggregator-parser-1268234-3.patch2.08 KBderjochenmeyer
FAILED: [[SimpleTest]]: [MySQL] 32,867 pass(es), 0 fail(s), and 7 exception(es). View
#1 move-out-rendering-image-from-aggregator-parser-1268234-1.patch1.99 KBderjochenmeyer
PASSED: [[SimpleTest]]: [MySQL] 33,072 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


derjochenmeyer’s picture

Status: Needs review » Active
1.99 KB
PASSED: [[SimpleTest]]: [MySQL] 33,072 pass(es). View

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"
    <title>RSS Title</title>
    <description>This is an example of an RSS feed</description>
      <title>Image title</title>


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 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).
derjochenmeyer’s picture

Status: Active » Needs review
derjochenmeyer’s picture

Assigned: Unassigned » derjochenmeyer
Status: Active » Needs review
2.08 KB
FAILED: [[SimpleTest]]: [MySQL] 32,867 pass(es), 0 fail(s), and 7 exception(es). View

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).

Status: Needs review » Needs work

The last submitted patch, move-out-rendering-image-from-aggregator-parser-1268234-3.patch, failed testing.

derjochenmeyer’s picture

Status: Needs work » Needs review
2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 32,862 pass(es). View

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

twistor’s picture

Status: Needs review » Reviewed & tested by the community

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.

derjochenmeyer’s picture

2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 33,302 pass(es). View

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

webchick’s picture

Should this be backported to D7 as well?

Dries’s picture

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.

twistor’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

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.

derjochenmeyer’s picture

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

A regular expression that extracts the img path?

Jacine’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: +needs backport to D7

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. :)

twistor’s picture

#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;
twistor’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -needs backport to D7

Cross post?

Jacine’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, thanks for the walkthrough in #12.

Committed and pushed to 7.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.