Skip to:
Content

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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)

5020.01.patch (39.0 KB) - added by r-a-y 4 years ago.
Updated patch to fix mentions feed
5020.02.patch (39.5 KB) - added by r-a-y 4 years ago.

Download all attachments as: .zip

Change History (15)

#1 @r-a-y
4 years ago

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.

@r-a-y
4 years ago

Updated patch to fix mentions feed

#2 @boonebgorges
4 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?

#3 @ubernaut
4 years ago

+1000000

;)

i'll get to that testing asap!

#4 @ubernaut
4 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 @johnjamesjacoby
4 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.

#6 @boonebgorges
4 years ago

Marked #4564 as duplicate

@r-a-y
4 years ago

#7 @r-a-y
4 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 use bp_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.

#8 @boonebgorges
4 years ago

  • Keywords commit added

Whoop!

#9 @ubernaut
4 years ago

awesome hopefully i helped a little still need to get the hang of these patches i think.

Last edited 4 years ago by ubernaut (previous) (diff)

#10 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 7207:

Introduces BP_Activity_Feed class, and migrates existing feeds

RSS feeds for BP activity have historically been generated on a one-by-one
basis, with separate template files used for each theme. This resulted in a
huge amount of unnecessarily duplicated code. Moreover, it made the process
of customizing feeds, or providing new feeds, unnecessarily cumbersome.

BP_Activity_Feed abstracts the central parts of RSS feed building, allowing for
easier customization, and fewer points of failure.

This changeset also fixes our RSS implementation to match best practices with
respect to encoding content inside of the <description> element.

Fixes #5020

Props r-a-y

#11 @boonebgorges
4 years ago

In 7335:

Implement BP_Activity_Feed for activity mentions feed

This particular feed was missed during the original implementation of
BP_Activity_Feed. See #5020, r7207. As a result, the mentions feed was
attempting to load from a separate feed template file that no longer
exists. This broke the feed and filled the error logs with warnings.

Fixes #5124

Props r-a-y

#12 @boonebgorges
4 years ago

In 7336:

Implement BP_Activity_Feed for activity mentions feed

This particular feed was missed during the original implementation of
BP_Activity_Feed. See #5020, r7207. As a result, the mentions feed was
attempting to load from a separate feed template file that no longer
exists. This broke the feed and filled the error logs with warnings.

Fixes #5124

Props r-a-y

#13 @ubernaut
4 years ago

i was going to mention that i was still seeing the original out-of-sync issue reported in #4564 under 1.8 but forgot at the last devchat not sure if this is the cause but thought i would add that info.

Note: See TracTickets for help on using tickets.