Posted by agentrickard on April 18, 2007 at 5:26pm
In working on the MySite project, I just wrote the following piece of code. It transfers aggregator feed images and writes them to a local directory.
My thought is this has security and performance benefits. But I may have missed something....
Feedback?
<?php
/**
* This function takes a Feed image and saves it locally.
* We do this for added security and speed.
* @param $fid == the feed id, taken from {aggregator_feed}
* @param $image == the image string taken from {aggregator_feed}
* @return $newfile == the filepath string pointing to the local copy of the file
*/
function mysite_type_feed_image($fid, $image = NULL) {
$path = file_directory_path() . '/mysite';
$temp = file_directory_temp();
$newfile = '';
if (!empty($image)) {
if ($src = preg_match('/src="(.+?)"/', $image)) {
$src = preg_replace('/.+src="(.+?)".+/', '\1', $image);
$src = check_url($src);
$ext = explode('.', $src);
$ext = '.' . array_pop($ext);
}
}
$filename = 'aggregator-' . $fid . $ext;
$file = file_check_location($path . '/' . $filename, $path);
if (file_exists($file)) {
$newfile = $path . '/' . $filename;
}
if (empty($newfile) && !empty($image)) {
watchdog('MySite', t('Copying icon file for Feed ID %fid.', array('%fid' => $fid)), WATCHDOG_NOTICE);
$file = drupal_http_request($src);
$newfile = file_save_data($file->data, $temp .'/' . $filename, FILE_EXISTS_REPLACE);
$info = image_get_info($newfile);
if ($info['extension']) {
if (image_get_toolkit()) {
image_scale($newfile, $newfile, 120, 60);
}
$move = file_move(&$newfile, $path, FILE_EXISTS_REPLACE);
}
else {
$newfile = '';
watchdog('MySite', t('The transfer of a MySite feed icon failed -- bad file extension -- for Feed ID %fid.', array('%fid' => $fid)), WATCHDOG_ERROR);
}
if ($move) {
$newfile = $path . '/' . $filename;
}
else {
$newfile = '';
watchdog('MySite', t('The transfer of a MySite feed icon failed -- could not copy file -- for Feed ID %fid.', array('%fid' => $fid)), WATCHDOG_ERROR);
}
}
return $newfile;
}
?>Of course, this code is designed to work with the data already stored in the {aggregator_feed} table. If approved, we would move the code into the parsing routine.
Comments
This is a great feature. It
This is a great feature.
It should be optional, though: Think of massively downloading feeds: you wouldn't want to worry about the bandwidth and storage space for downloading and saving pictures.
On a similar note: another detail to think of is deleting pictures when the feed item gets deleted...
Probably interesting: Just recently I integrated favicon download in leech:
http://cvs.drupal.org/viewcvs/drupal/contributions/modules/leech/leech.m...
http://www.twitter.com/lxbarth
Clarification
Let me clarify. This function does not download images embedded in feeds.
It downloads the IMAGE element of the feed (the logo icon of the site).
An extra module may be the way to go, but then you'd have the issue of two ways to handle feed images....
And shouldn't you use
check_url()instead ofcheck_plain()is statements like:$data = drupal_http_request(check_plain($url_str));--
http://ken.therickards.com/
http://savannahnow.com/user/2
http://blufftontoday.com/user/3
--
http://ken.therickards.com/
Sorry, misunderstood. I got
Sorry, misunderstood. I got thrown off by the term "icon" - should we stick to "Image" instead of "Icon"?
Thanks for the clue on check_plain/check_url - the code is mainly from urlicon module - I'll roll a patch for both leech and urlicon then.
http://www.twitter.com/lxbarth
I think we should probably
I think we should probably start standardizing on an "Aggregation & RSS" category in .info files. I agree, this is probably best as a separate module. Storing and caching the favicons once IS a good thing, but some folks won't care at all.
The other bit that is important, of course, is the centralized feed listing/store. That's a task for someone: to look at the DB structures of all the modules and see what the minimal set of columns for the "feeds" store is.