Regression with 'bp_template_content' action

[DJPaul]

There's a regression in 1.8 where the bp_template_content action seems to occur twice with the oldskool "plugins" template. Tested with SHA 136660bf75 of my Achievements plugin; ​https://github.com/paulgibbs/achievements

Screenshots:
​http://cl.ly/image/0v0C2442021r
​http://cl.ly/image/1W0j1R1o2J0O

Relevant part of Achievements' code: ​https://github.com/paulgibbs/achievements/blob/master/includes/buddypress/functions.php#L40

[DJPaul]

Git bisect suggests this changed in r7131

[boonebgorges]

Thanks for the report, DJPaul.

It's possible that there is a BP bug here, but it's not directly related to 'bp_template_content' - a bit of debugging shows that that hook is only being fired once (as is the wrapper function dpa_bp_members_my_achievements_content()).

The duplication begins with DPA_Shortcodes::display_user_achievements(). My backtrace shows that it's being called from two different spots. First, it's called (as expected) from the dpa_bp_members_my_achievements_content() function, hooked to bp_template_content - ie, the normal BP template loading technique. Second, it's being called from the Achievements theme compatibility layer, in particular dpa_replace_the_content().

Indeed, I'm not sure that this is a regression. The same duplication appears to be happening when I test Achievements against BP's 1.7 branch.

It seems to me that the "right" way to do this (insofar as there is a "right" way) is for Achievements to bail out of its theme compat layer if BP's theme compat is running. One notable change in BP 1.8 is that there is not really a reliable way to check for this, because BP unsets its internal "is theme compat active?" flag very early in its own loop, making it unavailable to nested content: ​http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-theme-compatibility.php#L541.

As a data point, other plugins are *not* having the same duplication problem. BuddyPress Docs, for instance, does not duplicate member template content, hooked to 'bp_template_content' in members/single/plugins.php

DJPaul or jjj, I know this is an incomplete picture, but maybe it's enough to spark an idea with one of you. Any thoughts? (a) Is this in fact a regression? (b) If so, is it going to affect any plugins other than Achievements, which is the only one (that I know of) that had such an advanced theme compat layer? (c) If it's only Achievements, can we just ask Paul to make the necessary mods, rather than playing around more with BP's theme compat load order?

[DJPaul]

If it's only Achievements, can we just ask Paul to make the necessary mods

I'm not against this, but if I did, I'd like to understand why this particular revision in BuddyPress changed the behaviour in Achievements. I have not backtraced the functions as you have before/after r7131, but in terms of looking at the actual HTML on the screen, I see the duplicated template.

[boonebgorges]

I'd like to understand why this particular revision in BuddyPress changed the behaviour in Achievements

I agree that it'd be good to understand before making a change, but in my tests, it's not a regression. The same duplication is happening on the 1.7 branch. Have you tested there?

[DJPaul]

Achievements is working as expected against BP 1.7 and 1.7.2 (I've just tested those two specific versions).

[boonebgorges]

With BP 1.7 and BP 1.7.2 I'm seeing the duplicate content, with similar stack traces as those described above. I guess I'll have to dig deeper to figure out what's going on, though I'm not totally sure how I'll do that given that my rig is acting differently from yours. Are you running MS? Is Achievements network activated? (might make a difference in load order, though I'm grasping at straws a little)

[boonebgorges]

Quick follow up - it does appear to be a load order thing. When BP (1.7 or 1.8) is network activated, and Achievements is activated on a single site, the double template stuff happens. In other words: when BP loads before Achievements, BP's theme compat layer has a chance to load (and then continues to let Achievements's layer load). You don't see this under "normal" circumstances because *A*chievements loads before *B*uddyPress.

The only regression for 1.8, then, is that we must have moved something in BP to load earlier than Achievements *in every case*, rather than just in these odd circumstances. I'll dig into it deeper to try to get a better understanding. But, given that the double-content stuff was already happening in previous versions of Achievements, I think it's probably a bug (if you want to call it that) in both plugins. More to come.

[boonebgorges]

I've located the specific change that's causing the problem to surface in 1.8. Related ticket: ​https://buddypress.trac.wordpress.org/ticket/5021

In 1.7, BP ran bp_remove_all_filters( 'the_content' ) just before running its own the_content manipulation: ​http://buddypress.trac.wordpress.org/browser/tags/1.7.2/bp-core/bp-core-theme-compatibility.php#L522 In the case of Achievements, that meant that BP was shutting off Achievements's version of theme compat (​https://github.com/paulgibbs/achievements/blob/master/includes/core/theme-compatibility.php#L447). However, when BP 1.7.x runs its initial theme compat setup *before* Achievements, such as when BP is network-activated and Achievements is not, bp_remove_all_filters( 'the_content' ) runs *before* Achievements has a chance to hook its theme compat layer to 'the_content', with the result that the filter is not removed and we get double content. This is a bug in Achievements that only surfaces under very specific circumstances.

In r7131, BP stopped running bp_remove_all_filters( 'the_content' ) before running theme compat. For discussion, see #5021. As a result, the Achievements bug that was only visible in edge cases in 1.7.x became visible in every circumstance in 1.8.

My take on the situation is two-fold:

a) It's a coincidence of load order (ie, the alphabet) that Achievements didn't exhibit this bug under BP 1.7.x. So it should be fixed there. How could it be fixed? Ideally, Achievements's theme compat layer would check to see if BP's is already running (or has already run) before running its own, and bail if so. That's tricky to do right now, because there's no reliable way to tell while inside of BP's output buffer that you are experiencing theme compat. BP could set a flag early - something like buddypress()->theme_compat->started. Then, Achievements would add a check to dpa_do_theme_compat() ​https://github.com/paulgibbs/achievements/blob/master/includes/core/theme-compatibility.php#L547 along the lines of if ( bp_is_theme_compat_started() ) return false;. (Side note: we can't use bp_is_theme_compat_active() for this purpose because in BP we've set it to false by the time the buffering begins, to prevent recursive the_content swaps in certain scenarios.)

b) Although I think that, at root, this is an Achievements bug, I also think that the changes introduced in r7131 are introducing enough of a change in behavior for plugins that we should revert at least part of the behavior. In particular, if we reinstate the bp_remove_all_filters( 'the_content' ) in bp_template_include_theme_compat(), Achievements goes back to the 1.7.x status quo. This means that #5021 will remain unsolved for now, but IMO that's not a huge deal for 1.8 - it was an enhancement anyway.

Would like a sanity check before proceeding with either or both of these solutions. Thanks.

[DJPaul]

Thanks for figuring this out Boone.

The way I see it, because I am using the bp_template_content hook, I need to disable Achievements' theme compat if I know I am going to use bp_template_content. This should be do-able; my plugin here doesn't really care if you are using BP-Default or BP's theme compat layer.

As far as BuddyPress goes; because we've only seen this bug with Achievements, blame it on Achievements ;)

I think that if this change was likely to cause problems, I would have expected to see if happen with bbPress as well. I haven't extensively tested for this with bbPress, but I haven't seen an issue in casual use, either.

[boonebgorges]

I need to disable Achievements' theme compat if I know I am going to use bp_template_content.

Ha, duh, this is the obvious solution. Sorry for not thinking of it myself :)

Thanks very much for having a look here, DJPaul. Going to close this ticket as invalid.