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.
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | node.module_rss_fix_for_4-7.patch | 1.73 KB | drewish |
#19 | Drupal.rss-item-modification.patch.txt | 1.71 KB | Gábor Hojtsy |
#18 | node.module_41703.patch | 1.71 KB | drewish |
#14 | node.module_hook_rss_item_0.patch | 2.87 KB | drewish |
#13 | node.module_hook_rss_item.patch | 2.87 KB | drewish |
Comments
Comment #1
drewish CreditAttribution: drewish commentedre-rolled
Comment #2
drewish CreditAttribution: drewish commentedChasing head...
Comment #3
drewish CreditAttribution: drewish commentedre-rolled for HEAD
Comment #4
drewish CreditAttribution: drewish commentedre-rolled for HEAD
Comment #5
drewish CreditAttribution: drewish commentedSince 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 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?
Comment #6
drewish CreditAttribution: drewish commentedNow 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.
Comment #7
markus_petrux CreditAttribution: markus_petrux commentedI 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").
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedthis makes sense. without it, rss item is severely crippled. i will test this soon
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedif 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'?
Comment #10
drewish CreditAttribution: drewish commentedSo 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 ;)
Comment #11
markus_petrux CreditAttribution: markus_petrux commentedWhat 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():
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.
Comment #12
drewish CreditAttribution: drewish commentedpersonally, 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.
Comment #13
drewish CreditAttribution: drewish commentedHere's a patch that add a hook_rss_item.
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.
Comment #14
drewish CreditAttribution: drewish commentedmanaged to forget the patch...
Comment #15
walkah CreditAttribution: walkah commentedpatch 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?
Comment #16
walkah CreditAttribution: walkah commentedi 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). :)
Comment #17
drewish CreditAttribution: drewish commentedbut 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?
Comment #18
drewish CreditAttribution: drewish commentedhumm, 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.
Comment #19
Gábor HojtsyGood idea, the latest patch is indeed a simple change. This should be committed IMHO. Updated patch attached.
Comment #20
Gábor HojtsySimlified issue title
Comment #21
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #22
(not verified) CreditAttribution: commentedComment #23
drewish CreditAttribution: drewish commentedfor reference, here's a patch for 4.7
Comment #24
drumm+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.
Comment #25
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedapplied
Comment #26
drewish CreditAttribution: drewish commentedman, that stokes me out. i never though i'd see that get committed to 4.7.
Comment #27
(not verified) CreditAttribution: commented