bp_get_button() does not allow a core component button (regression)

[johnbillion]

The bp_get_button() function (and corresponding BP_Button class constructor) accepts a 'component' item in its array of arguments.

Before the button is built, bp_is_active() is called to check if the required component is active. If you don't specify a component, it defaults to 'core', but bp_is_active( 'core' ) always returns false, so a core component button will never be shown.

Note that this is a regression. At BuddyPress 1.2.9, bp_is_active( 'core' ) would return true and core buttons would work fine. At some point between 1.2.9 and 1.5, this changed and broke.

Marking as 1.5, may affect earlier versions.

[DJPaul]

We should fix for a 1.5 point release.

[cnorris23]

Patches for branch and trunk

[DJPaul]

Changing milestone to 1.6 because if we've only just noticed this issue just before 1.6 is ready, then it can wait until 1.6.

[DJPaul]

Patch works, but I am wondering about where/how this function was used without a component being specified. I am also thinking it might be better to change the !bp_is_active check in bp_button, rather than updating bp_is_active().

[cnorris23]

I am wondering about where/how this function was used without a component being specified.

I wondered the same thing. The reason I chose to patch bp_is_active() directly is two-fold

1) It is technically a regression, since prior to 1.5 a check for bp_is_active( 'core' ) would return true, since it was never in the $bp_deactivate components global

if ( !isset( $bp_deactivated[ 'bp-' . $component . '/bp-' . $component . '-loader.php' ] ) )

Obviously, the 'core' check returning true was a matter of happenstance rather than being intentional, but a regression nonetheless.

2) BP_Button defaults to 'component' => 'core', so technically a function could pass an array to bp_button/bp_get_button() without 'component' defined (lazy, but allowed), and still pass the conditional checks. Changing this behavior would also be a regression.

Changing bp_is_active() to return true when 'core' is passed was the simplest method with the least amount of impact. Personally, I like the idea of setting 'component' to an empty string in the BP_Button defaults, on top of adding the extra check in bp_is_active(), but it will likely break a few plugins.

[boonebgorges]

I'm with cnorris23 on this one. If this is a regression in bp_is_active(), we should restore its old behavior. Having a special case for 'core' is not elegant or beautiful to look at it, but it's harmless.

[DJPaul]

Alright.

[DJPaul]

Is there a specific reason for the typecast to a string?

[boonebgorges]

Is there a specific reason for the typecast to a string?

Pretending that this isn't PHP? :)

[cnorris23]

Replying to boonebgorges:

Is there a specific reason for the typecast to a string?

Pretending that this isn't PHP? :)

Haha! It was just a better-safe-than-sorry addition. I don't really have a preference either way, it's just habit. You never know what someone might try :)

[cnorris23]

Also, I just remembered it was because I wanted to do a strict check so we're only returning true on the string 'core' which what BP_Button defaults to, and what brought this to light.

After thinking about what I just said, and in light of boone's joke, it's a moot point. BP_Button already defaults to 'core' as a string, and this will likely be the case everytime, so a string cast isn't really necessary. Just for get this whole comment even happened, and I'll stick with what I said in the previous comment.

[djpaul]

(In [6104]) Fix regression where bp_get_button() doesn't allow a 'core' component button.

• This changes the behaviour to match BP 1.2, increasing backpat with old themes/plugins.
• Fixes #4212, props cnorris23.