Skip to:
Content

Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#4846 closed defect (bug) (fixed)

Theme compat is overaggressive in loading its files, especially JS

Reported by: boonebgorges Owned by:
Milestone: 1.7 Priority: high
Severity: critical Version: 1.7
Component: Theme Keywords: 2nd-opinion has-patch
Cc: hugoashmore@…

Description

(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).
  1. 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.
  1. 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.
  1. In bp-legacy, do the following:

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.

Attachments (2)

4846.patch (2.3 KB) - added by boonebgorges 14 months ago.
4846.01.patch (935 bytes) - added by boonebgorges 14 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: r-a-y14 months ago

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.

Another check is to try and find a BP template file like /members/members-loop.php, that way we can be certain that this is a BP theme.

Last edited 14 months ago by r-a-y (previous) (diff)

comment:2 in reply to: ↑ 1 johnjamesjacoby14 months ago

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.

comment:3 imath14 months ago

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.

comment:4 boonebgorges14 months ago

  • Keywords has-patch added

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?

boonebgorges14 months ago

comment:5 follow-up: boonebgorges14 months ago

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.

boonebgorges14 months ago

comment:6 in reply to: ↑ 5 imath14 months ago

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.

comment:7 imath14 months ago

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'.

comment:8 sbrajesh14 months ago

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.

comment:9 sbrajesh14 months ago

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.

comment:10 boonebgorges14 months ago

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

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

Fixes #4846

comment:11 boonebgorges14 months ago

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.

comment:12 hnla14 months ago

  • Cc hugoashmore@… added
Note: See TracTickets for help on using tickets.