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

CommentFileSizeAuthor
#29 feed_12.patch2.68 KBm3avrck
#26 feed_11.patch2.69 KBm3avrck
#21 feed_10.patch5.55 KBm3avrck
#17 feed_9.patch9.02 KBm3avrck
#16 feed_8.patch8.93 KBm3avrck
#15 feed_7.patch9.05 KBm3avrck
#14 feed_6.patch9.1 KBm3avrck
#13 feed_5.patch9.08 KBm3avrck
#8 feed_4.patch9.02 KBm3avrck
#4 feed_3.patch8.89 KBm3avrck
feed_2.patch9.29 KBm3avrck
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jjeff’s picture

jjeff CreditAttribution: jjeff commented

Very cool!

+1

Dries’s picture

Dries CreditAttribution: Dries commented

Looks like what other systems would use a template 'tag' for.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

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

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

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Needs work » Needs review
FileSize
feed_3.patch8.89 KB

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.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented

It's still ready ;-)

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

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)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

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

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Needs work » Needs review
FileSize
feed_4.patch9.02 KB

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?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Looking at this code fragment:

+ $feed_url = url('rss.xml', NULL, NULL, TRUE);
+ drupal_set_feed($feed_url);
drupal_add_link(array('rel' => 'alternate',
'type' => 'application/rss+xml',
'title' => t('RSS'),
- 'href' => url('rss.xml', NULL, NULL, TRUE)));
+ 'href' => $feed_url));

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.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented

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.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Reviewed & tested by the community

I would say this is RTBC.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Needs work

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.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Needs work » Needs review
FileSize
feed_5.patch9.08 KB

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!

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
FileSize
feed_6.patch9.1 KB

Updated patch for HEAD.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
FileSize
feed_7.patch9.05 KB

Updated for HEAD.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
FileSize
feed_8.patch8.93 KB

Oops a little conflict got through, here is a revised one.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
FileSize
feed_9.patch9.02 KB

Updated for HEAD. New $delimiter to be more conistent with core, thanks to Earl for that tip.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Reviewed & tested by the community

This looks good to go to me; it's gone through quite a lot of hammering to get to this point!

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Title: Create new feed icon variable » Add RSS autodiscovery automatically
Category: feature » task
Status: Reviewed & tested by the community » Active

Committed to HEAD.

The RSS autodiscovery link should be added by drupal_add_feed() as the icon is added.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented

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

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
FileSize
feed_10.patch5.55 KB

Consolidate the drupal_add_link() calls.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Active » Needs review
walkah’s picture

walkah CreditAttribution: walkah commented

+1 this looks good.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Fixed » Active

The titles are missing the two feeds in the aggregsator module.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Active » Reviewed & tested by the community
FileSize
feed_11.patch2.69 KB

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.

Dries’s picture

Dries CreditAttribution: Dries commented

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

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Needs work

And t('Drupal') should be simply 'drupal', which seems the most common default for this ('Drupal' is second)

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Needs work » Reviewed & tested by the community
FileSize
feed_12.patch2.68 KB

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.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Reviewed & tested by the community » Fixed

Committed by Dries. http://drupal.org/cvs?commit=39020

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)