Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GES should provide full topic text for bbPress 2.x notifications #46

Closed
boonebgorges opened this issue Dec 4, 2012 · 8 comments
Closed
Assignees
Milestone

Comments

@boonebgorges
Copy link
Member

In part because of the bug described here http://bbpress.trac.wordpress.org/ticket/2082, and in part because it's more helpful for user engagement in Commons In A Box-type communities, we should intervene in the GES process and provide full reply/topic content. See http://redmine.gc.cuny.edu/issues/2297

@r-a-y
Copy link
Member

r-a-y commented Dec 4, 2012

For the first bug, wptexturize() on BBP content, BP Reply By Email works around this by adding a filter to GES:
https://github.com/r-a-y/bp-reply-by-email/blob/master/includes/bp-rbe-extend-bbpress.php#L256

These filters should probably be added to GES instead of RBE. Also, wp_specialchars_decode() should be run on all BP email content, see:
https://buddypress.trac.wordpress.org/ticket/4684#comment:3


Secondly, the code you added into frontend-ges.php will not run unless a GES option is checked from the CBOX Settings admin page.

We can fix this by either adding an admin option under the CBOX Settings admin page, or if we want to autoload a plugin's code, I can add some logic to frontend.php so we can always load our plugin's frontend-XX.php code if that plugin is setup.

It might make more sense if the full text code was in GES.

boonebgorges added a commit that referenced this issue Dec 4, 2012


Also refactors the settings API a bit so that multiple settings can be
registered for each plugin. See #30
@boonebgorges
Copy link
Member Author

Secondly, the code you added into frontend-ges.php will not run unless a GES option is checked from the CBOX Settings admin page.

Yeah, oops. I was fixing it as you were writing that :)

These filters should probably be added to GES instead of RBE. Also, wp_specialchars_decode() should be run on all BP email content, see:
https://buddypress.trac.wordpress.org/ticket/4684#comment:3

I'm not sure that wp_specialchars_decode() will work for all of these characters. It won't convert curly-quotes, right? In any case, the real fix is to store raw text in the db, and to allow easy access to that raw text, at least in the case of this bbPress stuff.

It might make more sense if the full text code was in GES.

Yeah, it might. Then you have to worry about people who are already using the plugin and are expecting it to continue to behave the way it currently does, etc. So for now I think I'll continue with it in Commons In A Box, and we can revisit for GES at some point.

@r-a-y
Copy link
Member

r-a-y commented Dec 4, 2012

bbPress actually does remove all its filters before sending its own subscription emails:
https://bbpress.trac.wordpress.org/browser/tags/2.2.2/includes/common/functions.php#L1005

Perhaps, we should copy what bbP does instead?

Here's what it might look like:
http://pastebin.com/pQXmuESx

@boonebgorges
Copy link
Member Author

Yeah, that's a thought. The only thing is that you might legitimately
want at least some filters to be run on this content - you just don't
want presentation filters to be run on it.

On 12/04/12 15:15, r-a-y wrote:

bbPress actually does remove all its filters before sending its own
subscription emails:
https://bbpress.trac.wordpress.org/browser/tags/2.2.2/includes/common/functions.php#L1005

Perhaps, we should copy what bbP does instead?

Here's what it might look like:
http://pastebin.com/pQXmuESx


Reply to this email directly or view it on GitHub
#46 (comment).

@r-a-y
Copy link
Member

r-a-y commented Dec 4, 2012

bbP runs filters before saving (balanceTags, wp_filter_kses and wp_rel_nofollow), so it is safe to assume that is enough?

For reference, here are the filters currently being run on reply content output:

add_filter( 'bbp_get_reply_content', 'capital_P_dangit'         );
add_filter( 'bbp_get_reply_content', 'wptexturize',        3    );
add_filter( 'bbp_get_reply_content', 'convert_chars',      5    );
add_filter( 'bbp_get_reply_content', 'make_clickable',     9    );
add_filter( 'bbp_get_reply_content', 'force_balance_tags', 25   );
add_filter( 'bbp_get_reply_content', 'convert_smilies',    20   );
add_filter( 'bbp_get_reply_content', 'wpautop',            30   );
add_filter( 'bbp_get_reply_content', 'bbp_mention_filter', 40   );

@boonebgorges
Copy link
Member Author

Yeah, those are fine as they stand, but still a third party plugin might
want to legitimately run a filter on this value, and
remove_all_filters() prevents this. So I guess I feel like bbPress is
being a little heavy-handed in the way it handles this issue natively.
That said, I can see the value of acting in the same way that bbPress
acts in this case, so if you feel strongly that it makes more sense to
do remove_all_filters() in the cbox filter I've added, please do so.

On 12/04/12 15:30, r-a-y wrote:

bbP runs filters before saving (balanceTags, wp_filter_kses and
wp_rel_nofollow), so it is safe to assume that is enough?

For reference, here are the filters currently being run on reply content
output:

|add_filter( 'bbp_get_reply_content', 'capital_P_dangit' );
add_filter( 'bbp_get_reply_content', 'wptexturize', 3 );
add_filter( 'bbp_get_reply_content', 'convert_chars', 5 );
add_filter( 'bbp_get_reply_content', 'make_clickable', 9 );
add_filter( 'bbp_get_reply_content', 'force_balance_tags', 25 );
add_filter( 'bbp_get_reply_content', 'convert_smilies', 20 );
add_filter( 'bbp_get_reply_content', 'wpautop', 30 );
add_filter( 'bbp_get_reply_content', 'bbp_mention_filter', 40 );
|


Reply to this email directly or view it on GitHub
#46 (comment).

@r-a-y
Copy link
Member

r-a-y commented Dec 4, 2012

Nah, I'm fine with the way it is! Just looking at the code again, using
get_post_field() also bypasses the 'bbp_get_reply_content' filter as
well, so both approaches are the same.

Boone Gorges wrote:

Yeah, those are fine as they stand, but still a third party plugin might
want to legitimately run a filter on this value, and
remove_all_filters() prevents this. So I guess I feel like bbPress is
being a little heavy-handed in the way it handles this issue natively.
That said, I can see the value of acting in the same way that bbPress
acts in this case, so if you feel strongly that it makes more sense to
do remove_all_filters() in the cbox filter I've added, please do so.

On 12/04/12 15:30, r-a-y wrote:

bbP runs filters before saving (balanceTags, wp_filter_kses and
wp_rel_nofollow), so it is safe to assume that is enough?

For reference, here are the filters currently being run on reply content
output:

|add_filter( 'bbp_get_reply_content', 'capital_P_dangit' );
add_filter( 'bbp_get_reply_content', 'wptexturize', 3 );
add_filter( 'bbp_get_reply_content', 'convert_chars', 5 );
add_filter( 'bbp_get_reply_content', 'make_clickable', 9 );
add_filter( 'bbp_get_reply_content', 'force_balance_tags', 25 );
add_filter( 'bbp_get_reply_content', 'convert_smilies', 20 );
add_filter( 'bbp_get_reply_content', 'wpautop', 30 );
add_filter( 'bbp_get_reply_content', 'bbp_mention_filter', 40 );
|


Reply to this email directly or view it on GitHub

#46 (comment).


Reply to this email directly or view it on GitHub
#46 (comment).

@boonebgorges
Copy link
Member Author

Cool. I'm going to close this for now. Maybe we can revisit this for GES down the road, especially if GES is rewritten to be group-independent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants