This module currently provides a lot of different types of feeds and not all of them are likely to be useful to everyone. It would be nice to disable some feeds if they are not desired.

I've attached a patch which adds this as well as a few other things.
1. Adds "Comment RSS" settings page which lets you enable only the feeds you want.
2. Removes the "nodetype" feed because there is no obvious use for that
3. Advertises feeds with "drupal_add_feed" instead of "drupal_add_link" so that comment feeds will show a feed icon on the page so they are not discoverable only if the browser supports the meta tag.
4. Changes feed titles slightly (I wasn't happy with the "Site - Article - Comments" formatting)
5. Fixes the content type on feed headers
6. Adds the comment module as a dependency
7. Adds install and uninstall functions to initialize and clean up new variables (will be in separate file)

Sorry for overloading this patch with so many changes but I have already spent a lot of time on this module and preparing this patch and it would take a lot longer to break it up. If there are concerns with any particular part preventing a commit please discuss.

CommentFileSizeAuthor
#9 commentrss.module.HEAD_.r0_0.patch6.9 KBgnassar
#8 commentrss_settings.patch6.03 KBtangent
#3 commentrss.module.HEAD_.r0.patch6.9 KBgnassar
#1 commentrss.install.txt626 bytestangent
commentrss.diff14.36 KBtangent

Comments

tangent’s picture

tangent commented
StatusFileSize
new commentrss.install.txt626 bytes

This is the new commentrss.install file.

tangent’s picture

tangent commented

I neglected to mention that this patch also removes the node from the comment feed for a node. Including it seems like very non-standard practice. A feed is a queue of content and including a static item in a queue is illogical.

gnassar’s picture

gnassar commented
StatusFileSize
new commentrss.module.HEAD_.r0.patch6.9 KB

Wow. This was surely a lot of work. And digging through the patch, I was also able to find yet another bug I wouldn't have found otherwise. (Will submit a separate issue for that, if it's not in there already.)

That being said, I disagree with a few of those changes, and would've loved to see a patch solely for "making comments optional," as the title states. As I know that's a lot of work, I went ahead and modified the patch myself to only add that functionality, with nothing else. Tested not DOA (on my production site -- bad habit of mine) :-).

BTW, one of the major reasons I think this feature patch should be separated is that you address a few good points that should be treated as bugs, and thus prioritized higher.

To discuss the specific changes:

1) This change is the point of this issue, so of course I agree :-)
2) There's probably as much need for a nodetype comment feed as there is for a sitewide comment feed, in the mind of those who have, say a separate blog and forum. Either way, the patch to make all feeds optional clearly should allow for making this optional as well to be a proper, isolated patch. (For that matter, that also means that all feeds should by default be enabled, so that installing this patch by default changes nothing on the user experience side. The enclosed patch follows that philosophy. So it can be said the patch resolves #1 and #2.)
3) Agreed. This is an easy enough change to isolate and package in its own patch.
4) I like the original ones better. This can be a separate patch, but if it's a big enough deal to make a separate patch for, then it should probably be "done right" and a title configuration option should be included in the admin menu.
5), 6), 7) Good call. Each one is an easy one-line bug patch. Should be separate issues, so it gets patched quickly. (6 and 7 can probably be dealt with together.)

And I'll make a #8 out of the later comment: "I neglected to mention that this patch also removes the node from the comment feed for a node. Including it seems like very non-standard practice. A feed is a queue of content and including a static item in a queue is illogical."

8) I absolutely agree, but as this is a change that will affect the output of the module itself, and clearly the module creator likes it enough to have put it in in the first place, this should a) definitely be a separate patch, and b) if you really want it committed, I imagine you have a better shot if you make it an optional item configurable in the admin menu. (Speaking of which, this is where the bug I found lies -- it doesn't actually include teasers as comments; it'll include whatever the RSS settings are configured to publish, which could be the full text or nothing at all but the title. Not the behavior the module says it will exhibit. Fortunately, I think that'll be an easy fix.)

gnassar’s picture

gnassar commented

I went ahead and created a standalone patch for #8 here: http://drupal.org/node/157255.

Speaking of separate patches for bugs, I noticed that in your uber-patch, you also changed the way each of the feed constructors looked up their id value (for $vid, $tid, and so on). I haven't looked too carefully at that, but if there's a good reason for that, that's another (bug?) patch that could be submitted.

Just wanted to point that out; thought it might help since I'm already digging through your patch and I'm sure you want to dig through it as little as possible. :-)

tangent’s picture

tangent commented

I expected that this patch would have to be broken up. Thanks for your detailed review.

Regarding:
#2 The reason I concluded that a nodetype feed for comments was not useful is that there is no nodetype feed for nodes (in core at any rate). It may be useful to support the Views module RSS view in offering comment feeds for any set of nodes rather than adding arbitrary feed types directly. However, the feature must have been added because someone found it useful so this is all just my opinion I guess.

#4 Title formatting may or may not be a preferential topic. I would prefer more control over feed formatting in general but that's another topic. I'll defer to the will of the masses (and just use a customized version of the module until feeds are themeable).

#8 I'm torn about offering an option to show a source item (node) in comment feeds. If you examine comment feeds generated by any other system I think you will not find such a feature. The feed will already refer to the source and it is up to the feed parser (reader) to provide links to the source but showing a static item in a feed is likely to just confuse the consumer and the feed reader. For example, I only view unread feed items in Google Reader so I'm not sure (haven't tested) but I suspect the node item would never appear again anyway.

#9 I changed the variable validation sequence so that it ensured the value used would always come from the object instead of from the page args. I thought that was more sensible.

I'll try to submit a few of these as individual patches. Feel free to discuss further. I'm always willing to be proven wrong.

Christefano-oldaccount’s picture

Christefano-oldaccount commented

+1 for splitting this into separate patches. Thanks for your great work so far! Will be following with interest.

tangent’s picture

tangent commented

Ok, change #5 is moved to issue 157594 and change #6 is moved to issue 157571. Change #7 is a new file and it's really only necessary with the settings changes so I'll leave it here.

Change #3 will conflict with the current patch I think so I'm going to wait to match that patch until some of these others get committed.

tangent’s picture

tangent commented
StatusFileSize
new commentrss_settings.patch6.03 KB

The patch in comment #3 doesn't apply for me.

Here's my updated patch for this issue. It's as stripped down as I can make it and only adds what's necessary to add options to disable specific feeds. This along with the commentrss.install in comment #1 should suit this issue.

gnassar’s picture

gnassar commented
StatusFileSize
new commentrss.module.HEAD_.r0_0.patch6.9 KB

I just redownloaded my patch, and it was missing a linefeed at the end of the file. Added that linefeed, and the patch now applies to a clean copy of the DRUPAL-5 dev branch, as indicated in the version spec, just fine. Reupping it again, just for thoroughness' sake.

On #2: well, not really on #2, but tangential to it -- I think that would make a lot of sense. Tying this all to the Views RSS module would obviate the need for things like this.

Then again, I'm one of those "Views for everything" type of guys; I like the flexibility of being able to define how content is collated at my discretion. Much more than having hard-coded per-content, per-taxonomy, etc. collections of nodes that I may or may not use, and am likely to have to find a way to disable at least one of. I think "views for everything" is the most flexible and logical way to deal with these things -- but I think I'm in the minority of Drupal users, as Views are a comparatively recent way to deal with these things in Drupal. So until Views is in core (I can't wait!), I think we're going to need things like #2. Not the ideal way to do things, I think, but the most practical seeing where we currently stand.

#8 - I went ahead and commented on the thread specific to this issue.

#9 - yeah, that made sense when I saw it, but I didn't know if there was a problem with the original implementation. IIRC, Drupal core tends to err toward using the passed parameters.

tangent’s picture

tangent commented
Status: Needs review » Fixed

I committed this feature to HEAD.

gnassar’s picture

gnassar commented

It seems that all the patches you've made so far have not gone into HEAD. They are all in, but not in the HEAD branch. It has to be specified explicitly when you do the commit.

Putting these into HEAD, and backporting into DRUPAL-5, would be appreciated. Let me know if you want me to do that for you; I'd be happy to.

Anonymous’s picture

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