Anyone using the drupal.org/cvs with a splendid RSS feed reader like Akregator feels the pain: at first, the commit appears in the feed, and an hour later (probably on cron) it seems to be different enough to be considered a different item by the feed reader. Depending on the changes in the feed format, I've got mostly two, sometimes three or more RSS items for the same commit.

So while pimping Commit Log's RSS feed, I ported the required code from node.module which does it right by assigning a 'guid' property in addition to 'pubDate', and that makes the feed reader update the item instead of adding a new one. Here's the (untested) cvs.module version of it. I'd love to finally get awesome commit feeds :)

CommentFileSizeAuthor
#4 cvs-rss-guid-try2.patch1.01 KBjpetso
cvs-rss-guid.patch896 bytesjpetso

Comments

dww’s picture

dww
we/he/they
commented

Looks good to me. I only wonder if the 'CVS commit' and 'at' parts should be translatable? I.e., instead of this:

       'guid' => 'CVS commit '. $commit->cid .' at '. $base_url,

Something like this?

       'guid' => t('CVS commit !commit_id at !base_url', array('!commit_id' => $commit->cid, '!base_url' => $base_url)),

???

dww’s picture

dww
we/he/they
commented

To clarify, my assumptions are:

A) "guid" stands for "globally unique identifier", and the specific format doesn't matter, only that:
A.1) it's globally unique
A.2) it never changes for a given commit at a given site

B) That some feed readers might display this guid somewhere in the UI.

C) That RSS feeds for sites using locale.module always end up in a single, site-specific language.

I'm probably totally wrong about (C), which would make my solution violate (A.2). I'm probably wrong about (B), as well. Bottom line: I don't know much about RSS, and I especially don't know hardly anything about RSS + locale. ;) So, please clarify exactly in what way(s) I'm wrong, and why your solution is the best...

Thanks! ;)
-Derek

jpetso’s picture

jpetso commented
Status: Needs review » Needs work

I'm not much of an RSS expert as well, apart from that I'm an extensive user of a lot of RSS feeds. So here's my take:

(A) is correct.
(C), as you presume, is incorrect, as it violates (A.2). This is backed up by node.module having $item->nid .' at '. $base_url as guid string.
As for (B), I can't tell for sure, but out of three RSS feed readers that I have used or seen, none have displayed the GUID anywhere in the user interface.

Anyways, it seems like the patch has to be adapted because the isPermaLink attribute of the guid tag is not set false. For a short but to-the-point description (just looked it up myself) see this page.

jpetso’s picture

jpetso commented
Status: Needs work » Needs review
StatusFileSize
new cvs-rss-guid-try2.patch1.01 KB

Ok, better one, even more similar to the version in node.module, with the effect that it includes an additional isPermaLink="false" attribute for the guid item in the RSS output. That's what we want, as far as I can see. (The other possibility would be to make the guid an URL, but as URLs can change more easily, I think having it this way is the better solution.)

No idea about your linebreak preferences though, do adjust the formatting in case you don't like it.

jpetso’s picture

jpetso commented

Pretty please?

hunmonk’s picture

hunmonk commented
Status: Needs review » Fixed

tested out good. committed w/ some minor code style adjustments, deployed on d.o

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.