Currently you're not able to modify the body of an RSS item. The $item_text is built from the node's body/teaser and then hook_nodeapi('rss item') is called.

By switching the order of these two events, modules that would like to modify the body could take the node passed to hook_nodeapi and modify it before the the RSS item is built.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

re-rolled

drewish’s picture

Chasing head...

drewish’s picture

re-rolled for HEAD

drewish’s picture

re-rolled for HEAD

drewish’s picture

Since I'm having a bit of trouble getting this reviewed I'll talk a little bit about why it's needed.

I ran into this issue while working on an update of the audio module for 4.7. The problem comes when you try to build an RSS feed for podcasting. Currently the order is:

  • The modules builds a SQL query and pass that to node_feed(). node_feed() loops through the query and loads each node and calls hook_nodeapi($op = 'view').
  • The module does its normal themeing.
  • $item_text is generated from the, themed, body and teaser.
  • Finally, hook_nodeapi($op = 'rss item') is called allowing modules to return additional elements for the feed.

The problem from the audio module's perspective is that during the 'view' it doesn't know if the node will be used for HTML or RSS so it adds an <object> element for the flash player assuming it'll be for HTML. When the 'rss item' is finally called it's too late to change the body and remove the flash player.

The patch I've supplied was the simplest way to work around this. Now, during 'rss item' event, the audio module just reloads the body and teaser. It might be better to call 'rss item' before 'view' and then set a for_rss value on the $node object. That way the current "return additional elements" logic can stay in 'rss item' and the view can determine how the node will be view the first time.

Any thoughts?

drewish’s picture

Now that I think about it, it might make more sense to just call 'rss item' instead of 'view'. Modules that need to distinguish between the two could and those that don't could just treat the two operations the same way.

The event module's (see event_calendar_rss()) it doesn't use node_feed() because it uses additional filtering beyond what could easily be done in a SQL WHERE clause. It only calls 'rss item' not view.

markus_petrux’s picture

I have observed a similar problem with the AdSense module.
http://drupal.org/node/45237

Maybe the nodeapi('view') hook could have an extra argument to tell if the node is to be build for an RSS feed?

So you would know if you need to add the flash object (or the AdSense module would know when to insert "section targeting tags").

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman

this makes sense. without it, rss item is severely crippled. i will test this soon

moshe weitzman’s picture

if i read this right, drewish argues against his own patch in #6. makes sense to me. does that imply that many modules that implement view should also be implementing 'rss item'? if so, we need a patch :) because this one isn't really 'in review'?

drewish’s picture

So maybe call hook_nodeapi('rss item') and if nothing's returned/no module implements it, call hook_view/hook_nodeapi('view')? I guess I'll roll a patch ;)

markus_petrux’s picture

What about adding a new argument to hook_view/hook_nodeapi('view'), FALSE by default, and explicitly set to TRUE from node_feed?

This is how it could look the relevant part of node.module::node_feed():

    if ($item_length != 'title') {
      $teaser = ($item_length == 'teaser') ? TRUE : FALSE;

      // Filter and prepare node teaser
      if (node_hook($item, 'view')) {
        // The 5th argument here means this is to build a RSS feed
        node_invoke($item, 'view', $teaser, FALSE, TRUE);
      }
      else {
        $item = node_prepare($item, $teaser);
      }

      // Allow modules to change $node->teaser before viewing,
      // or add additional item fields.
      // The 5th argument here means this is to build a RSS feed
      $extra = node_invoke_nodeapi($item, 'view', $teaser, FALSE, TRUE);
      if (is_array($extra)) {
        $extra = array_merge($extra, array(array('key' => 'pubDate', 'value' =>  date('r', $item->created)), array('key' => 'dc:creator', 'value' => $item->name), array('key' => 'guid', 'value' => $item->nid . ' at ' . $base_url, 'attributes' => array('isPermaLink' => 'false'))));
        foreach ($extra as $element) {
          if ($element['namespace']) {
            $namespaces = array_merge($namespaces, $element['namespace']);
          }
        }
        $items .= format_rss_item($item->title, $link, $item_text, $extra);
      }
    }

Maybe that way, hook_nodeapi('rss item') is not needed.

I haven't tested this method. I believe though, that it's simpler than calling 2 nodeapi hooks.

drewish’s picture

personally, i think hook_nodeapi() has enough parameters, it seems silly to add another for just one case. i'll work on a patch that calls rss item and if nothing implements it, falls back to view.

drewish’s picture

Here's a patch that add a hook_rss_item.

<?php
/**
 * Format a node as an RSS item.
 * @param $item a node object.
 * @param $item_length Specifies how much of the node's content to display: 'fulltext', 'teaser' or 'title'.
 * @return array indexed by namespace, of arrays containing extra elements.
 */
function hook_rss_item($item, $item_length) {}
?>

The idea is, if hook_rss_item is implemented, that's used instead of hook_view. After the body and teaser are built, hook_nodeapi(op='rss item') is called as it is currently.

The current behavior is unchanged, but modules that need to differentiate between RSS and standard views can now do so.

drewish’s picture

managed to forget the patch...

walkah’s picture

patch looks good, and I'm +1 for the functionality - I can see the desire to differentiate the view based on web vs. rss. However this begs 2 questions in my mind:

* Should we generalize beyond RSS - i.e. is it time to look at stronger atom support?
* And if we're special casing for RSS - how long before we special case for other mediums - i.e. does there need to be a hook_print ? Not to take us off on a tangent, but hopefully you see my point..

The other big concern I have is - right now you've got this hook set up as a node hook, that doesn't actually send a full node object. We also have several feeds (and could have a few more) that aren't node feeds (i.e. aggregator, and I totally forget why we don't have comment feeds). I'd vote for making it a general module_invoke() so, say, aggregator and comment can get in on the game if need be. make sense?

walkah’s picture

i should clarify (and bring my tangent back to earth):

+1 - just make it a general module_invoke hook rather than special casing for nodes (would be my vote). :)

drewish’s picture

but right now feeds are only built from nodes right? so we'd have to rename the function to a more generic build_feed() or something?

drewish’s picture

FileSize
1.71 KB

humm, after seeing this support request i remembered that this issue was never resolved. i've given it some thought and the original change seems like the simplest and best solution. i hope we can just get it applied and then figure out some more elegant solution.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.71 KB

Currently you're not able to modify the body of an RSS item. The $item_text is built from the node's body/teaser and then hook_nodeapi('rss item') is called.

By switching the order of these two events, modules that would like to modify the body could take the node passed to hook_nodeapi and modify it before the the RSS item is built.

Good idea, the latest patch is indeed a simple change. This should be committed IMHO. Updated patch attached.

Gábor Hojtsy’s picture

Title: node_feed() should build RSS item after calling hook_nodeapi 'rss item' » Allow node altering for RSS generation

Simlified issue title

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)
drewish’s picture

Version: » 4.7.3
FileSize
1.73 KB

for reference, here's a patch for 4.7

drumm’s picture

Version: 4.7.3 » 4.7.x-dev
Status: Closed (fixed) » Reviewed & tested by the community

+1, works as advertised on 4.7. Tested with a custom module for a client. If this is not considered an API change, then it is ready to commit.

killes@www.drop.org’s picture

Status: Reviewed & tested by the community » Fixed

applied

drewish’s picture

man, that stokes me out. i never though i'd see that get committed to 4.7.

Anonymous’s picture

Status: Fixed » Closed (fixed)