[buddypress-trac] [BuddyPress] #4081: Failed activity pages due to bad logic in bp_is_current_component

[buddypress-trac at lists.automattic.com]

#4081: Failed activity pages due to bad logic in bp_is_current_component
--------------------------+-------------------------------------
 Reporter:  chrisbliss18  |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Core          |    Version:  1.5.4
 Severity:  normal        |   Keywords:  has-patch needs-testing
--------------------------+-------------------------------------
 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:

 {{{
 #!php
 } 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:

 {{{
 #!php
 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:

 {{{
 #!php
 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
 [http://php.net/manual/en/function.array-search.php 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:

 {{{
 #!php
 "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.
 1. 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.
 1. 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
 1. remove-faulty-conditional.patch
 1. 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.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/4081>
BuddyPress <http://buddypress.org/>
BuddyPress