Theme compat is overaggressive in loading its files, especially JS

[boonebgorges]

(This bug was originally reported to me by imath - thanks!)

A number of popular BuddyPress standalone themes ("standalone" in the sense that they are *not* bp-default themes) load bp-default's global.js, either directly from bp-default or from a copy shipped with the theme.

On these sorts of themes, theme compat will kick in - it only bails when the theme is (a child of) bp-default: ​https://buddypress.trac.wordpress.org/browser/trunk/bp-templates/bp-legacy/buddypress-functions.php#L47 This is generally not a huge problem, because the theme's native templates will be loaded by bp_core_load_template() and the theme compat layer will not kick in. So, it's not ideal, but it's not harmful either.

However, bp-legacy's JS *is* a problem. When bp-legacy-js is loaded alongside dtheme-ajax-js, it means that AJAX actions are fired off twice, which causes lots of problems: activity items are double-posted, activity deletes fail because the item has already been deleted, etc.

There are a couple of strategies we might employ to fix this.

1. Load bp-legacy-js as late as possible, to give other themes plenty of time to enqueue. Then, before enqueuing, check in WP's JS enqueue stack to see if something called dtheme-ajax-js is already enqueued, and bail if found. This would probably cover most cases, because it looks like most themes (at least those I've checked) have used 'dtheme-ajax-js' to name their JS (they just copied it over from bp-default I guess).

2. Do a better job of automatically determining when theme compat should run. I'm not sure what this would be. We could, for instance, check to see if a file exists called _inc/global.js. This seems like a pretty lousy solution in a couple ways, but I'm just throwing it out there.

3. In addition to checking get_template() == 'bp-default' and get_stylesheet() == 'bp-default', we could also do something like: current_theme_supports( 'buddypress-ajax' ). Then, we do a big publicity push to get theme authors to register their theme as having 'buddypress-ajax' support if they plan to load their own JS for AJAX. The upside of this strategy is that it introduces pretty much zero overhead for us, and it will never result in false negatives (cases where theme compat should load its JS but doesn't). The downside, of course, is that it requires action by theme authors.

4. In bp-legacy, do the following:
   • First, add prefixes to all of the ajax hooks ​https://buddypress.trac.wordpress.org/browser/trunk/bp-templates/bp-legacy/buddypress-functions.php#L137. Eg, 'post_update' becomes 'bp_legacy_post_update'.
   • Then, after bp-legacy has hooked all of its AJAX handlers, loop through all of the non-prefixed handlers and *unhook* them. That way, the theme's JS will still load, and AJAX requests will still be sent, but they won't do anything because WP won't be expecting them.

This fourth strategy is good because it's something that's within our control, and is pretty much guaranteed to catch all cases of rogue themes. It's bad because it doesn't prevent the duplicate AJAX requests from firing, which results in additional overhead (as multiple instances of WP will spin up each time a request is sent). Of course, this latter issue would clear itself up as theme authors fixed their themes.

I personally lean toward a combination of 3 (best practice) and 4 (failsafe), but it's possible that I'm missing something. In any case, we really have to do *something*, because as it currently stands, these themes are largely broken on 1.7.

[r-a-y]

Couldn't we also check a theme's tags to see if buddypress is used as a tag?

Another solution similar to point no. 3 is using the existing current_theme_supports( 'buddypress' ).

We already add this in bp-default:
​https://buddypress.trac.wordpress.org/browser/tags/1.6.4/bp-themes/bp-default/functions.php#L70

But we don't use it for any checks at the moment.

[johnjamesjacoby]

Replying to r-a-y:

Another solution similar to point no. 3 is using the existing current_theme_supports( 'buddypress' ).

This. Though it's plausible many themes don't have this bit, it's exactly (and only, IMO) the correct use-case for using the theme support API.

[imath]

Hi,

thanks boone for the mention ;)

I think point 3 or similar is really interesting because it will help BuddyPress plugin developers to choose the best way to load their custom templates.

[boonebgorges]

4846.patch implements a current_theme_supports( 'buddypress' ) check before loading bp-legacy theme compat. I had to change the load order for this to work properly. The WP Codex recommends that themes register feature support at after_setup_theme: ​http://codex.wordpress.org/Function_Reference/add_theme_support That means that, in order for our current_theme_supports() check to work, it needs to load later than after_setup_theme:10. I've opted to move the bp_after_setup_theme hook to 100. I'm posting it here before committing, to see if there are objections to this specific method.

Also, it feels like checking for theme support in the Theme_Compat constructor is wrong somehow. My gut feeling is that we ought to be checking one step upstream, at bp_load_theme_functions(), so that the theme compat layer doesn't get loaded at all (saving a bit of overhead). Thoughts?

[boonebgorges]

Quick follow-up. In addition to 4846.patch, we might consider a sanity check like 4846.01.patch (basically, my suggestion 2 above). In short: before enqueuing bp-legacy JS, check to see if bp-default's js has already been enqueued. If so, throw an error message and bail. (We can't safely dequeue dtheme-ajax-js because we will break dependencies.) Note that this patch will only work if you hook the enqueue_scripts() method later than after_setup_theme:10; see my comment 4 for explanation.

[imath]

Replying to boonebgorges:

we might consider a sanity check like 4846.01.patch

Hi Boone,
I think it's a good idea, i've ran some test with a premium theme with the 2 patches. It's working for the first one if in the functions.php of the theme the "add_theme_support( 'buddypress' );" is added.

But for 4846.01.patch it looks like dtheme-ajax-js is not enqueued yet. So when debug is set to true in my wp-config.php, the warning message is not displayed.

If i edit the functions.php in order to enqueue the script a bit earlier, for example :
add_action( 'wp_enqueue_scripts', 'bp_dtheme_enqueue_scripts', 9 );
then the message is showing

If i change the priority of the bp_enqueue_script hook to 11 in bp-core-actions.php, like this
add_action( 'wp_enqueue_scripts', 'bp_enqueue_scripts', 11 );
then the message is showing

Problem is : if the priority is changed in bp-core-actions, does it cause any regressions ? I will keep on my tests with the 2 other themes we've discussed about and keep you updated.

[imath]

I ran some tests with 3 "standalone BuddyPress" themes (1 free and 2 premiums) :

• Only one had the add_theme_support( 'buddypress' ) in his functions.php file
• 2 were using the identifier 'dtheme-ajax-js' to enqueue _inc/global.js, the other was using 'bp-js' to enqueue _inc/global.js

I confirm my last comment : adding "add_theme_support( 'buddypress' );" solves the trouble for each theme
I confirm the warning message (4846.01.patch) is only fired if the priority of the bp_enqueue_script hook is later than the one used by the theme to enqueue his 'dtheme-ajax-js'.

[sbrajesh]

Hi,
just wanted to let you know that I did some tests on it too.
Adding add_theme_support( 'buddypress' ) on after_setup_theme action did not solve the issue. buddypress.js and buddypress.css was getting loaded(I am on rev r6821 with WordPress Multisite Installation).

I had to remove the 'bp_load_theme_functions' from 'bp_after_setup_theme' action and that stopped loading.

I may be wrong, but if a theme registers support for BuddyPress, should not we completely avoid using theme compat files in that case?

There are other options too like woocommerce does(gives the user options to load the included css/js and the user can decide) but I personally believe, if a theme registers support for BuddyPress(if after_setup_theme hook is too late, we can check using theme tag), we should completely avoid loading the theme compat, otherwise It may create a mess.

As a theme Author, I feel, we need more flexibility and control. I am not sure If I want BuddyPress to automatically inject some elements in my presentation of the theme if I am creating a theme specially for BuddyPress.

For the themes, which does not register support for BuddyPress, I believe it is fine to inject the content.

[sbrajesh]

Ok, Just did some cleanup, installed a fresh copy from svn and applied the patches and It does stops buddypress.js/buddypress.css from loading if we register support for BuddyPress using add_theme_support.

[boonebgorges]

(In [6822]) Don't load bp-legacy theme compat if current_theme_supports( 'buddypress' )

Fixes #4846

[boonebgorges]

Thanks for the feedback, all. It sounds like current_theme_supports( 'buddypress' ) is generally agreed to be the best way forward. I'm going to put a post up on bpdevel explaining what theme authors have to do.

For the moment, I'm not planning on doing any of the automated cleanup items I suggest above (1, 2, and 4). Basically, each one would introduce overhead in BuddyPress, and we should be encouraging theme authors to do it the "right" way.

For the moment, I am *not* going to add a check for the 'buddypress' theme tag. (1) This also adds some overhead; and (2) I don't trust those tags - at least in the case of plugins, authors use the tag very promiscuously.

Feel free to reopen for further discussion if you think that more needs to be done than r6822.