Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment #1
derjochenmeyer CreditAttribution: derjochenmeyer commentedIt'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>
: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>
)On the other hand there are the following fields in
{aggregator_feed}
table:{aggregator_feed}.link
with 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).Comment #2
derjochenmeyer CreditAttribution: derjochenmeyer commentedComment #3
derjochenmeyer CreditAttribution: derjochenmeyer commentedRestored 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:
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).
Comment #5
derjochenmeyer CreditAttribution: derjochenmeyer commentedFix #4 and add a CSS class
.feed-image
to the image link.Comment #6
twistor CreditAttribution: twistor commentedBingo 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.
Comment #7
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere is an updated patch that just removes a trailing space in #5.
Comment #8
webchickShould this be backported to D7 as well?
Comment #9
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Moving to 7.x but not 100% convinced it should be backported. Unless I'm mistaken, this could break existing templates.
Comment #10
twistor CreditAttribution: twistor commentedThe 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.
Comment #11
derjochenmeyer CreditAttribution: derjochenmeyer commentedWhat's the bast practice for update_N to replace the markup with the image source?
A regular expression that extracts the img path?
Comment #12
JacineThis 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. :)
Comment #13
twistor CreditAttribution: twistor commented#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.
Comment #14
twistor CreditAttribution: twistor commentedCross post?
Comment #15
Jacine@twistor Ha, yes definitely a cross post. Whoops. I was only responding to #9.
Comment #16
webchickCool, thanks for the walkthrough in #12.
Committed and pushed to 7.x. Thanks!