Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#4879 closed defect (bug) (fixed)

add_theme_support( 'buddypress' ) should not load buddypress-functions.php

Reported by: modemlooper's profile modemlooper Owned by: boonebgorges's profile boonebgorges
Milestone: 1.9 Priority: normal
Severity: major Version: 1.7
Component: Appearance - Template Pack Keywords: needs-testing has-patch
Cc: mercijavier@…

Description

bp-default derived themes copied over functions.php but theme compat uses buddypress-functions.php. If you use add_theme_support( 'buddypress' ) it loads buddypress-function.php and your functions file which can cause duplicate buttons, duplicate secondary avatars and any other code copied from bp-default.

Attachments (3)

4879.01.patch (3.1 KB) - added by r-a-y 9 years ago.
4879.02.patch (4.7 KB) - added by boonebgorges 9 years ago.
4879.02b.patch (4.6 KB) - added by r-a-y 9 years ago.

Download all attachments as: .zip

Change History (22)

#1 @boonebgorges
10 years ago

  • Keywords reporter-feedback added

This shouldn't happen. It's true that we load buddypress-functions.php, but none of the items (avatars, buttons, etc) should load, because of the check that we do in the constructor: https://buddypress.trac.wordpress.org/browser/trunk/bp-templates/bp-legacy/buddypress-functions.php#L47

Can you please share your child theme, or the relevant parts of it, so that the devs can attempt to reproduce the problem?

#2 @modemlooper
10 years ago

Actually, it may be because I'm using a theme switch for mobile.

I have twenty twelve active on desktop so it's using theme compat but the switched theme is a derivative of bp-default. I can change functions.php into buddypress-functions.php for a fix but I think add_theme_support( 'buddypress' ) should not load buddypress-functions.php

#3 @boonebgorges
10 years ago

I think add_theme_support( 'buddypress' ) should not load buddypress-functions.php

To repeat: while it's true that we are loading the file, it shouldn't actually do anything as long if your current_theme_supports( 'buddypress' ). So I don't see a functional difference between what we currently do and what you are suggesting.

I'm guessing that you have copied buddypress-functions.php into your own theme, and in the process you are double-defining many function names. Didn't you have to change them to clear duplicate function name errors? Again, if you would be willing to share the code that you are using, it would be a lot easier to help to debug the issue.

#4 @modemlooper
10 years ago

I'm not using any code. It's a theme inside wp-content/themes that was a derivative of bp-default. Theme switcher on mobile switches theme but since the desktop is using theme compat BuddyPress is loading buddypress-functions.php but the theme that is the derivative includes functions.php so it's duplicating some functions that add buttons and secondary images. I placed add_theme_support( 'buddypress' ) in the bp-default derived theme and it should block buddypress-functions.php

I could remove the extra functions but this won't fix the issue of loading both files.

#5 @boonebgorges
10 years ago

Is "theme switcher" a plugin? Would you mind sending a link so that I can see how it accomplishes this on-the-fly switch?

#6 @modemlooper
10 years ago

no plugin. my code.

	function get_stylesheet( $stylesheet ) {

		if ( wp_is_mobile() ) {
			return 'child';
		} else {
			return $stylesheet;
		}
	}

	function get_template( $template ) {
		if ( wp_is_mobile() ) {
			return 'child';
		} else {
			return $template;
		}
	}

	add_filter( 'template', 'get_template' );
	add_filter( 'stylesheet',  'get_stylesheet' );

#7 @modemlooper
10 years ago

I should add, this is an issue when you are using a full theme and not a child of bp-default and you include a copied functions.php from bp-default

Last edited 10 years ago by modemlooper (previous) (diff)

#8 @johnjamesjacoby
9 years ago

  • Keywords 2nd-opinion needs-testing added; dev-feedback reporter-feedback removed
  • Milestone changed from Awaiting Review to 1.8

If it's happening, I'm okay with this for now. It shouldn't be happening though, since BP_Theme_Compat::start() bails if the current theme supports BuddyPress. It's possible there's a load-order issue here, but that said, if you need to, you can completely unhook bp_load_theme_functions() for now if it interferes with anything.

remove_action( 'bp_after_setup_theme', 'bp_load_theme_functions', 1 );

#9 @modemlooper
9 years ago

That works!

#10 @boonebgorges
9 years ago

  • Milestone changed from 1.8 to 1.9

I've seen this issue come up in other contexts, so I'm moving to 1.9 to try to sort it out.

#11 @mercime
9 years ago

  • Cc mercijavier@… added

#12 @hnla
9 years ago

I opened a ticket on the add_theme_support issue @ #5142. As JJJ surmises it looks essentially to be a load order issue, if add_theme_support('buddypress') is correctly hooked on 'after_setup_theme' then class BP_Theme_compat will not bail as it ? runs too early to read add_theme_support - move add_theme_support out of any hook and the BP class now gets the value and does bail.

@r-a-y
9 years ago

#13 @r-a-y
9 years ago

  • Keywords has-patch added; 2nd-opinion removed

As hnla alluded to in #5142, this is definitely a load order issue.

BP_Theme_Compat::start() runs after the buddypress-functions.php file is loaded and that functions file contains the extended BP_Theme_Compat class. Chicken before egg!

01.patch moves the enable theme compat logic into bp_load_theme_functions() so we can determine whether we should load up buddypress-functions.php.

I've also introduced new functions - bp_enable_theme_compat() and bp_set_enable_theme_compat().

bp_enable_theme_compat() is extremely useful for plugin devs as it helps in determining whether a plugin should serve up templates depending if theme compat or bp-default is enabled.

The only thing I'm worried about in the patch is deregistering the default theme compat template stack. Could use some dev feedback on this.

Last edited 9 years ago by r-a-y (previous) (diff)

#14 @boonebgorges
9 years ago

Good find, r-a-y.

The only thing I'm worried about in the patch is deregistering the default theme compat template stack. Could use some dev feedback on this.

I'm not sure the point of doing this. If theme compat is disabled, what's the difference if the template stack is registered?

Function naming is non-ideal, IMO. bp_enable_theme_compat() sounds like a verb ("hey BP, enable theme compat!") but it's actually a check. Maybe better: bp_is_theme_compat_enabled()? The other function, bp_set_enable_theme_compat(), also makes it sound as if theme compat is being enabled, when really it's *detecting whether* it should be enabled. Maybe better: bp_set_theme_compat()?

#15 @r-a-y
9 years ago

If theme compat is disabled, what's the difference if the template stack is registered?

I believe I remember reading in another ticket that it might be advantageous for bp-legacy templates to "leak" through. If you remove the bp_deregister_template_stack() line, bp-legacy templates will indeed be used as a fallback if the templates don't exist in the parent / child theme. I think we should deregister it as it makes logistical sense. Just wanted a second opinion!

(As I was looking for this ticket, Boone actually mentioned the bp_load_theme_functions() approach in ticket:4846#comment:4.)

I'm fine with your function naming, Boone! Was kind of contemplating the same thing as I was writing the code.

#16 @boonebgorges
9 years ago

I played with this a bit more, and I think that even my proposed function names are not clear enough. The issue is that we already have functions called is_theme_compat_active(); the difference between that and _enabled() is far from clear. What the new functions do is to detect whether the *current theme needs theme compat*. So I've renamed them accordingly. Let me know what you think about this.

I believe I remember reading in another ticket that it might be advantageous for bp-legacy templates to "leak" through. If you remove the bp_deregister_template_stack() line, bp-legacy templates will indeed be used as a fallback if the templates don't exist in the parent / child theme. I think we should deregister it as it makes logistical sense. Just wanted a second opinion!

Deregistering the template pack seems like a bit of a roundabout way to achieve this end - it's not very transparent, and would be pretty tricky to debug if you didn't already know what was up. A simple procedural check seems better to me, so we need a single point where we can check to see whether theme compat is required for the given theme, and bail if necessary. I've chosen the beginning of bp_theme_compat_reset_post(), because it's a single point that is called throughout the BP_Theme_Compat children in the individual components; if the dummy post doesn't get reset in this function, theme compat will fail. This seems a bit more elegant to me, but I'm open to feedback.

#17 @r-a-y
9 years ago

Nice changes in the function naming, Boone!

I think hooking into bp_theme_compat_reset_post() might even be too late. Doing the check there assumes that this function is called and hooked on the 'bp_template_include_reset_dummy_post_data' action.

Might be better to add the bp_use_theme_compat_with_current_theme() check at the beginning of bp_template_include_theme_compat().

Last edited 9 years ago by r-a-y (previous) (diff)

@r-a-y
9 years ago

#18 @boonebgorges
9 years ago

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

In 7432:

Do a better job preventing theme compat from running when current_theme_supports( 'buddypress' )

This changeset ensures that when a theme declares add_theme_support( 'buddypress' ),
BuddyPress will not load any of its theme compatibility layer. Previously,
'buddypress' support would prevent templates from loading, but would still
allow the buddypress-functions.php file to load in certain cases.

To this end, two new functions are introduced:

  • bp_detect_theme_compat_with_current_theme(), which centralizes the logic used by BuddyPress to detect whether the current theme will require theme compatibility (themes do not need theme compat when current_theme_supports( 'buddypress' ), when it's related to bp-default, or when members/members-loop.php is found). The function sets a global flag which can be checked later.
  • bp_use_theme_compat_with_current_theme(), which is used to check the flag set in bp_detect_theme_compat_with_current_theme().

Fixes #4879

Props r-a-y

#19 @johnjamesjacoby
8 years ago

  • Component changed from Backwards Compatability to Appearance - Template Pack
Note: See TracTickets for help on using tickets.