Failed activity pages due to bad logic in bp_is_current_component

[chrisbliss18]

I set up a site with BuddyPress and found that I could not get the activity page to work. No other plugins were active, I tried changing the slug of the used page, I tried regenerating rewrite rules, and nothing else seemed to be a potential cause. So I dug into the code.

What I found is that bp_core_action_search_site function would always redirect any activity page back to the home page. This was caused by a call to the bp_is_current_component function returning true when passed the output from the bp_get_search_slug function (in other words, on this site, it would always identify an activity page as a search page).

Digging into the bp_is_current_component function, I found the following section of code to be the source of the issue:

} else if ( $key = array_search( $component, $bp->active_components ) ) {
    if ( strstr( $bp->current_component, $key ) ) {
        $is_current_component = true;
    }
}

There is a logical error in this section of code as the $bp->active_components variable stores the components in keys not in values. Thus, the array_search call will never properly work.

To make this even more complex, the format of the $bp->active_components data structure can take two forms, depending on the logic path taken in BP_Core::bootstrap. It is for this reason that this code doesn't cause the results I found on every site.

The following data structure format is quite common and causes the conditional code above to always result in false:

array(
    'activity' => 1,
    'blogs'    => 1,
    'forums'   => 1,
    'friends'  => 1,
    'groups'   => 1,
    'members'  => 1,
    'messages' => 1,
    'settings' => 1,
    'xprofile' => 1,
);

This next data structure format is created when either of the fallback methods are used in BP_Core::bootstrap. This is the one that creates the issue. I'll explain about the issue after the data structure below:

array(
    'activity' => true,
    'blogs'    => true,
    'forums'   => true,
    'friends'  => true,
    'groups'   => true,
    'members'  => true,
    'messages' => true,
    'settings' => true,
    'xprofile' => true,
);

Note that in this data structure, a boolean true value is used instead of an integer value of "1" as in the other data structure.

Why does this difference matter? The difference matters because of how the ​array_search function operates. Note the third, optional argument: the $strict argument. By default, the array_search function will do a typeless comparison between the needle and haystack.

Knowing this, things started to make sense:

"search" == 1    // false
"search" == true // true

This accounts for the different behavior on this new site as compared to the other sites I've worked with. On this new site, the true values were in the data structure and caused the array_search function to always return "activity", the first key in the array, as a typeless comparison is used. Thus, it always thinks that an activity page is a search page.

The bug has other implications, but this is the main one as it results in the page being redirected to home, preventing further defects from being seen.

There are a few possible solutions:

1. The $bp->active_components data structure can be normalized to always use an integer 1 for the array values. This is a good idea as keeping the data structure consistent can help avoid future problems. While doing this would avoid my specific issue, it would leave the true source of the issue (the bad conditional) in place.
2. The problematic "else if" section can be removed. Since the logic is such that the section of code will either never run or only run in a manner that prevents it from properly testing for what it should test for, it seems that removing it should eliminate this issue while also avoiding other issues.
3. The problematic "else if" section can have its logic improved to properly work with either data structure. I think that this is the safest "do no harm" solution. I assume that the conditional was originally added for some purpose. My assumption is that the purpose is no longer required and that the code is merely vestigial at this point, but it is also possible that it is critical for some functionality. If this is the case, it clearly isn't functioning in these cases as it cannot due to the logic problems.

I have a patch for each option:

1. active_components-normalization.patch
2. remove-faulty-conditional.patch
3. improve-faulty-conditional.patch

I have tested each one against the current trunk (as well as against 1.5.4), and each patch corrects the issue. I think that implementing a patch of type #1 along with either patch #2 or #3 would work well to avoid potential other bugs.

[chrisbliss18]

Normalize the data structure of the $bp->active_components variable

[chrisbliss18]

Remove the faulty conditional logic

[chrisbliss18]

Improve the faulty conditional logic code

[boonebgorges]

Thanks for the in-depth report and patches, Chris.

I will need to do more research to be sure, but I seem to remember that the purpose of the array_search() clause was for backward compatibility with the pre-1.5 method of storing components in $bp->active_components, in which component names sometimes *were* the array values instead of the keys. (IIRC, 'canonical' component names were the keys, and component slugs were the values.) So the clause was meant to cover a specific (albeit edge) case: where someone was passing a component *slug* to bp_is_current_component(), and the component being checked was registered in the array by a plugin that had not been updated properly for the new BP 1.5+ active_components array format.

Again, this is off the top of my head - I will have to go back to BP 1.2.10 and the changelogs to find the answer. But if I'm right, then probably the check should stay there, but we should be doing strict array_search() checking, to avoid the problem you originally found, and we should be internally consistent about how we build $bp->active_components.

Putting in the 1.5.5 milestone for the moment, though we may have to punt it past, as there is an unrelated reason why 1.5.5 has to come out in the next couple days and I don't want this ticket to hold it up.

[cnorris23]

Although I can't replicate the failed activity pages, I can replicate the array_search failure.

I seem to remember that the purpose of the array_search() clause was for backward compatibility with the pre-1.5 method of storing components in $bp->active_components, in which component names sometimes *were* the array values instead of the keys. (IIRC, 'canonical' component names were the keys, and component slugs were the values.)

From what I can tell, this is correct. The recommended, pre-bp-core-component.php, way of registering a component was

$bp->active_components[$component_slug] = $component_id;

New patch changes the active_components array from

array(
    'activity' => true,
    ...
);

to

array(
    'activity' => 1,
    ...
);

Running array_search with STRICT set to true fixes the issue, but, in my opinion, we're just asking for more edge cases going that route. Patch was written on branch, but applied cleanly to trunk as of [6074].

[DJPaul]

How risky is this change? If it's not 110% safe, it should go into 1.6 incase we have to release a BP 1.5.6 for compat issues with WP 3.4.

[boonebgorges]

I vote for 4081.01.diff, but putting it off until 1.6.

[cnorris23]

@DJPaul it shouldn't be risky at all, but given the the fact that no one else (or at least very few) seems to have been effected by this, I agree with boone that we should just move it to 1.6.

[boonebgorges]

(In [6096]) Ensures consistent typing when core components register themselves in active array

When core components register themselves in the active_components array using
a boolean, it causes problems with the array_search() logic in
bp_is_current_component(). By using the int 1 instead, we avoid false positives
from array_search(), and thus from bp_is_current_component().

Fixes #4081.

Props chrisbliss18, cnorris23