Posted by Aron Novak on September 2, 2011 at 9:33pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | aggregator.module |
| Category: | task |
| Priority: | normal |
| Assigned: | derjochenmeyer |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
It's kind of nonsense that rendering image happens in aggregator.parser.inc, see this:
aggregator.parser.inc, 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.
Comments
#1
It's wrong what gets saved to the database in the first place, because
{aggregator_feed}.linkis 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}.descriptionis not populated at all.The RSS 2.0 Specification lists the following required elements for
<channel>:Image is an optional sub-element of
<channel>, which contains another three required sub-elements.<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.
{aggregator_feed}.linkwith the<channel><link>(which should be identical to<image><link>) and NOT the feed URL.{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).#2
#3
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).
#4
The last submitted patch, move-out-rendering-image-from-aggregator-parser-1268234-3.patch, failed testing.
#5
Fix #4 and add a CSS class
.feed-imageto the image link.#6
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.
#7
Here is an updated patch that just removes a trailing space in #5.
#8
Should this be backported to D7 as well?
#9
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.
#10
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/imgeis 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.
#11
What's the bast practice for update_N to replace the markup with the image source?
A regular expression that extracts the img path?
#12
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. :)
#13
#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;
#14
Cross post?
#15
@twistor Ha, yes definitely a cross post. Whoops. I was only responding to #9.
#16
Cool, thanks for the walkthrough in #12.
Committed and pushed to 7.x. Thanks!
#17
Automatically closed -- issue fixed for 2 weeks with no activity.