#5020 closed enhancement (fixed)
BP_Feed class
Reported by: | r-a-y | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 1.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Activity | Keywords: | has-patch commit |
Cc: |
Description
In the current codebase (BP 1.7), creating an activity RSS feed requires loading a separate file and hardcoding a bunch of parameters that get reused over and over again.
What would be swell is if we had a dedicated class where we can pass some parameters into the class constructor and it will generate the RSS feed for us.
Attachments (2)
Change History (15)
#2
@
12 years ago
+10000
I've only had a few minutes to read over the patch, but my preliminary judgment is that it totally kicks ass, and we should've done it years ago.
r-a-y - I'll test some more in the upcoming days, but r-a-y, what kind of testing have you done for backward compatibility? I see that you're supporting some legacy hooks with some wrapper methods. It looks like some of your changes may alter feed output somewhat (like the CDATA stuff), but my impression is that this will increase compliance with the spec, correct? Have you tested that the feeds validate, etc?
#4
@
12 years ago
hmm just realized this patch is rather more extensive then the one from the other ticket i'm maybe a little more concerned about running this on a live site. the glitch i was reporting pretty much requires a fairly active live site in order to test. if anyone else can confirm this patch doesn't do anything terrible i can try running it on our live site after that.
#5
@
12 years ago
This is really great. A few things:
- Rename BP_Feed to BP_Activity_Feed (other components might have other kinds of feeds)
- Change namespace from 'bp_feed' to 'bp_activity_feed' per above, in actions/filters/etc...
- Rather than hook to 'bp_actions' does it make sense to have a new hook for 'bp_feeds' -- Haven't thought this through, but wondering if all feeds should be hooked to the same action, to make them easy to unhook later?
- Do members have activity feeds that need doing?
Let's fix up these bits, and get this in for 1.8.
#7
@
12 years ago
02.patch
addresses some of JJJ's notes and:
- Fixes the
bp_activity_enable_feeds
filter. It wasn't working properly when returning false for the filter. - The item guid was not generating a unique value (an old BP RSS bug)
- Add a new hook -
bp_activity_feed_postfetch
- Changed the logic in
groups_action_group_feed()
to usebp_is_group_visible()
Regarding Boone's notes:
It looks like some of your changes may alter feed output somewhat (like the CDATA stuff), but my impression is that this will increase compliance with the spec, correct?
I followed Mozilla's recommendation about using only <content:encoded>
instead of abusing the <description>
element with CDATA:
https://developer.mozilla.org/en-US/docs/RSS/Article/Why_RSS_Content_Module_is_Popular_-_Including_HTML_Contents
However, the RSS spec is kind of lenient about this:
http://www.rssboard.org/rss-profile#element-channel-item-description
Have you tested that the feeds validate, etc?
Just tested. Fixed an old BP RSS bug where the item guid wasn't unique. Now it validates against W3C's feed validator.
Regarding JJJ's notes:
Rename BP_Feed to BP_Activity_Feed (other components might have other kinds of feeds)
Change namespace from 'bp_feed' to 'bp_activity_feed' per above, in actions/filters/etc...
Done
Rather than hook to 'bp_actions' does it make sense to have a new hook for 'bp_feeds' -- Haven't thought this through, but wondering if all feeds should be hooked to the same action, to make them easy to unhook later?
I've already added a filter called 'bp_activity_enable_feeds'
, which allows plugin devs to disable all feeds at once. I'm kind of agnostic about the 'bp_feeds' hook.
Do members have activity feeds that need doing?
I think we've covered all instances.
#9
@
12 years ago
awesome hopefully i helped a little still need to get the hang of these patches i think.
Attached patch (5020.01.patch) is a first pass at what is described above.
This builds upon enhancements that were originally included in ticket:4564#comment:18 (adding slash and syndication module support) as well as streamlining the RSS spec in a few areas (implement
<ttl>
, do not use<description>
and CDATA together, use<content:encoded>
instead).Feedback welcome.