Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

#4212 closed defect (bug) (fixed)

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

Reported by: johnbillion Owned by:
Milestone: 1.6 Priority: normal
Severity: normal Version: 1.5
Component: Core Keywords: has-patch dev-feedback
Cc:

Description

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.

Attachments (2)

4212.01.branch.diff (450 bytes) - added by cnorris23 8 years ago.
4212.01.trunk.diff (452 bytes) - added by cnorris23 8 years ago.

Download all attachments as: .zip

Change History (14)

#1 @DJPaul
8 years ago

  • Milestone changed from Awaiting Review to 1.5.6

We should fix for a 1.5 point release.

#2 @cnorris23
8 years ago

  • Keywords has-patch added

Patches for branch and trunk

#3 @DJPaul
8 years ago

  • Milestone changed from 1.5.6 to 1.6

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.

#4 @DJPaul
8 years ago

  • Keywords dev-feedback added

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().

#5 @cnorris23
8 years ago

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.

#6 @boonebgorges
8 years ago

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.

#7 @DJPaul
8 years ago

Alright.

#8 @DJPaul
8 years ago

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

#9 follow-up: @boonebgorges
8 years ago

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

Pretending that this isn't PHP? :)

#10 in reply to: ↑ 9 @cnorris23
8 years ago

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 :)

#11 @cnorris23
8 years ago

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.

Last edited 8 years ago by cnorris23 (previous) (diff)

#12 @djpaul
8 years ago

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

(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.
Note: See TracTickets for help on using tickets.