Add RSS autodiscovery automatically
m3avrck - May 31, 2006 - 19:17
| Project: | Drupal |
| Version: | x.y.z |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
One thing that I continually run into when theming sites is the fact that Drupal hardcodes RSS feed links to the bottom of the $content in phpTemplate. This makes it really hard to move around this icon on the page.
This patch creates 2 new functions: one for setting the feed url for a page and another for grabbing it. It then pipes this url into a $feed_icon for phpTemplate, so you can place it anywhere you want in your theme.
Simple and elegant :-)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| feed_2.patch | 9.29 KB | Ignored | None | None |
#1
Very cool!
+1
#2
Looks like what other systems would use a template 'tag' for.
#3
- What happens when there are multiple feeds on a page?
- This would be a good opportunity to centralize
<link rel=...rss stuff.I don't think either of those should hold up this patch since it already seems like a good improvement.
I do have an idea for making the syntax on the template end better (psuedocoe):
<?phpfunction drupal_feed_icon() {
that ternary operator bit and return what the themeable function returns or an empty string
}
?>
Then a simple call to that can be put in page.tpl.php and in the appropriate place in .theme themes.
#4
Neil, I thought about the multiple feeds per page as well but after looking through core, this doesn't seem to apply. Also, I couldn't really think of a situation where multiple feeds would be set on the same page so I opted for the simpler route. If this need does arise, it would be easy later on to alter drupal_set_feed_url() and drupal_get_feed_url() to work with arrays.
Further centralizing link rel stuff would be good too, but that is a bit outside the purpose of this patch, although this patch is the first step in the right direction.
Newly updated patch with the new drupal_feed_icon() to clean up the code a bit. Should be ready to go now.
#5
It's still ready ;-)
#6
If you modified it to: drupal_set_feed_url($url = NULL, $icon_theme = 'feed_icon')
You could make it store a list of feed_urls with the theme for the icon to use, so that it can print multiple icons. I think it should be trivial.
Views can already make use of something like this, as it can do both RSS and atom feeds on a page, and puts icons (tho at the moment atom.module doesn't actually have a theme_atom_icon and it could probably use one. But nobody's maintaining atom.module, really)
#7
In case you start adding modules like 'atom' or 'commentrss', you quickly get multiple feeds per page. A comment and a node RSS for example on a taxonomy page, or an atom and an RSS feed. Commentrss module would nicely plug into this feature, if it would allow for more than one feed per page. So it should :)
#8
Ok here's an updated patch that allows multiple feeds per page and different feed icons per feed.
I think that's it, ready to go?
#9
Looking at this code fragment:
I thought one of the goals of this patch is to eliminate fiddling with add_link for feed links. The rel=alternate is obvious, the type and title would need to get passed to drupal_set_feed() additionaly. Then if the type is passed on to theme_feed_icon, there would need to be no $icon parameter to drupal_set_feed.
So drupal_set_feed($url, $type, $title) would store an icon (themed through theme_feed_icon with $type passed on) and set a HTML link tag too.
Also $stored_urls does not look good in drupal_set_feed(), since it does not contain URLs, but themes icons (images) actually.
#10
Goba, this patch does not address the drupal_add_link() requests. It just consolidates all of the feed icons and direct links on the page (like on a taxonomy page the little feed icon at the bottom). Neil suggested that too above, but that is outside the scope of this patch.
I agree that should be addressed but in another patch.
Also, the $stored_urls is correct, since that variable is holding an array of URLs/icons. Or maybe "stored_feed_links" would be better.
#11
I would say this is RTBC.
#12
Since this allows multiple feeds, it should be drupal_add_feed() instead of set and drupal_get_feeds() instead of feed.
The theme_..._icon() construct isn't standard and I'm not sure how useful this will be in real life. I think we should either:
- Accept a whole a $function argument that is the themeable function name.
- Realize that theme_feed_icon() is themeable and let calling it be hard coded.
#13
New patch that addresses Neil's issues. Now you pass in the full theme function you want to use, this is great because a themer can add a list of theme_atom_feed() etc time functions and just alter each and every icon super easily.
Ready to go now!
#14
Updated patch for HEAD.
#15
Updated for HEAD.
#16
Oops a little conflict got through, here is a revised one.
#17
Updated for HEAD. New $delimiter to be more conistent with core, thanks to Earl for that tip.
#18
This looks good to go to me; it's gone through quite a lot of hammering to get to this point!
#19
Committed to HEAD.
The RSS autodiscovery link should be added by drupal_add_feed() as the icon is added.
#20
Talking with Neil a bit more, this entails:
drupal_add_link() inside drupal_add_feed() and remove all the separate places we call it now
you would have to add a $title parameter to drupal_add_feed()
#21
Consolidate the drupal_add_link() calls.
#22
#23
+1 this looks good.
#24
Committed to CVS HEAD.
#25
The titles are missing the two feeds in the aggregsator module.
#26
Neil, good catch.
This patch cleans up the aggregator titles, along with the main RSS feed. Additionally, it cleans up the hack in there to add the aggregator feeds; they have been moved to their appropriate functions now for added clarity.
#27
Please double check the use of arg(2) -- make sure it can't be used for XSS attacks. It goes off to various paths (drupal_add_feed, drupal_add_link, etc). They all need to escape arg(2) later on ...
#28
And t('Drupal') should be simply 'drupal', which seems the most common default for this ('Drupal' is second)
#29
Dries, the arg(2) is taken care of. Before that function is called, the menu callback is:
$category = aggregator_get_category(arg(2));if ($category) {
$items[] = array('path' => 'aggregator/categories/'. $category['cid'] .'/view',
'title' => t('view'),
'type' => MENU_DEFAULT_LOCAL_TASK,
'weight' => -10);
$items[] = array('path' => 'aggregator/categories/'. $category['cid'] .'/categorize',
'title' => t('categorize'),
'callback' => 'aggregator_page_category',
So arg(2) is checked through that function and proper passing to %d in the query. When it gets to that function, it's already a valid %d that was in the database. arg(2) was already being used on the line above and below, no problem.
@drumm - I was following suit with how that was already working in aggregator but you're right. New patch to fix that.
#30
Committed by Dries. http://drupal.org/cvs?commit=39020
#31