• The RSS 2.0 specification defines several optional channel elements : see http://cyber.law.harvard.edu/rss/rss.html#optionalChannelElements
  • format_rss_channel() allows optional channel elements to be passed as an array of optional parameters
  • however, node_feed does not pass the extra elements it receives in $channel to format_rss_channel() when it invokes it to create the channel information

As a consequence, these optional fields are ignored, although everything is in place to include them. All it would take is:

  1. remove the already-used elements from the $channel array
  2. pass the thus-trimmed $channel to format_rss_channel()

The problem exists in D7 (today's HEAD), D6, D5, D4.7 and D4.6.

CommentFileSizeAuthor
#19 242991.patch2.99 KBCarl Johan
#9 node_feed-ignore-no-more.patch2.37 KBCarl Johan
#8 node_feed-ignore-no-more.patch2.5 KBCarl Johan
#6 node_feed-channel-elements-with-test-without-garbage.patch2.68 KBCarl Johan
#5 node_feed-channel-elements-with-test.patch2.69 KBCarl Johan
#3 node_feed-ignores-using-array_diff_key.patch1.34 KBCarl Johan
#1 node_feed-ignores.patch1.2 KBCarl Johan

Comments

Carl Johan’s picture

Carl Johan commented
StatusFileSize
new node_feed-ignores.patch1.2 KB
Carl Johan’s picture

Carl Johan commented
Status: Active » Needs review

Plox to review

Carl Johan’s picture

Carl Johan commented
StatusFileSize
new node_feed-ignores-using-array_diff_key.patch1.34 KB

Updated to use array_diff_key instead

Status: Needs review » Needs work

The last submitted patch failed testing.

Carl Johan’s picture

Carl Johan commented
Status: Needs work » Needs review
StatusFileSize
new node_feed-channel-elements-with-test.patch2.69 KB

Updated to include a test

Carl Johan’s picture

Carl Johan commented
StatusFileSize
new node_feed-channel-elements-with-test-without-garbage.patch2.68 KB

Garbage removed

Status: Needs review » Needs work

The last submitted patch failed testing.

Carl Johan’s picture

Carl Johan commented
Status: Needs work » Needs review
StatusFileSize
new node_feed-ignore-no-more.patch2.5 KB

Cleaned up the test description

Carl Johan’s picture

Carl Johan commented
StatusFileSize
new node_feed-ignore-no-more.patch2.37 KB

Changed tabs to spaces and array('') to array()

chx’s picture

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

Nice job and welcome to the testing team :)

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Reviewed & tested by the community » Needs work

Hi Carl,

Thanks for this patch, which is just as I imagined it when I opened this issue. However, I thought a bit longer about it, and think it should include additional spec-defined elements and some safeguards:

  • some extra elements are defined by the spec and should be included:
    • the 4 we already have: title, link, description, (optional) language
    • some optional elements could come from the site configuration:
      • managingEditor, webMaster from variable site_mail
      • image from variable "theme_settings[logo] ? theme_settings[logo_path] : NULL"
    • some optional elements could be generated:
      • pubDate and/or lastBuildDate from the latest changed date in the node set
      • categories from the union of taxonomy terms on the nodes in the node set
      • generator should be "Drupal"
      • docs could/should be http://cyber.law.harvard.edu/rss/rss.html
    • some optional elements should come from the call to node feed(), as allowed by the patch:
      • cloud, ttl
      • textInput
      • skipHours, skipDays
  • other extra elements from the call to node_feed() should only be included if they are namespaced, as per http://cyber.law.harvard.edu/rss/rss.html#extendingRss : the spec does not allow non-namespaced elements if does not define.
Carl Johan’s picture

Carl Johan commented

Ah, hi fgm :)

This patch is just part 1 out of 2 in making it possible for modules to extend the core rss feed.

Would you accept moving those additional changes into another issue so that we could close this one? I would really love to be able to focus on allowing modules to extend the rss, before starting to extend it as you described. (My main reason for picking this up is that I'm playing around with rsscloud.)

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community

The original issue is fixed. Ie now you can pass in extra channel tags to node_feed as the documentation/function signature always suggested you could.

I was looking for an RSS RFC because I felt uncomfortable about just adding a tag like that, however, the docs you linked lists copyright as an optional tag.

Adding other defaults inside node_feed is a separate issue IMO.

I am re-setting the issue to RTBC because I do not think it should be node_feed's task to enforce a standard like this... then we enter the quagmire of different RSS versions and so on. The patch is small and useful on its own. We can do more later.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Hm. This seems like quite a clever little workaround to this issue.

However, let's expand the documentation of node_feed() to hint at this new capability, since otherwise the only people who will know that it works this way are the 4 people in this issue. :P

Carl Johan’s picture

Carl Johan commented

so, something like?

 * @param $channel
 *   An associative array containing title, link, description and other keys,
 *   to be parsed by format_rss_channel() and format_xml_elements().
 *   A list of channel elements can be found at the @link http://cyber.law.harvard.edu/rss/rss.html RSS 2.0 Specification. @endlink
 *   The link should be an absolute URL.
fgm’s picture

fgm
Primary language French
Location Paris, France
commented

I tend to agree with you: let's close this issue once the comments have been added as per webchick's suggestion... and open a separate issue for the elements validation.

However, regarding the problem with the quagmire of RSS versions, node_feed() already states (by including this in the $channel_defaults array) that it generates RSS 2.0 if the caller does not specify otherwise, so it IMHO should abide by the specification it claims to support.

Carl Johan’s picture

Carl Johan commented

Looking at the spec you linked earlier the only required elements for RSS 2.0 are title, link and description. And since they're always included, the claim is correct?

Regarding the documentation, I don't know if my comment is too technical, just pointing at the two functions? There's a good example in the documentation of format_xml_elements() that I felt should be referenced, but simply making a copy seems a bit overkill.

Thoughts?

chx’s picture

chx commented

I think that will be just fine. api.drupal.org crosslinks functions like that if you use the () as you did.

Carl Johan’s picture

Carl Johan commented
StatusFileSize
new 242991.patch2.99 KB
Carl Johan’s picture

Carl Johan commented
Status: Needs work » Needs review
chx’s picture

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

nice bugfix.

dries’s picture

dries commented

The test is a bit odd -- suggests that we have an API problem. Probably OK for D7 though.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed this patch, but accidentically as part of another patch. Sorry for not having given credit.

Status: Fixed » Closed (fixed)

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