Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 10 years ago

#3554 closed defect (bug) (wontfix)

Inverted component root page craziness

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Core Keywords: needs-patch 2nd-opinion
Cc:

Description

Example: Create pages with the slugs equal to any of the component ID's. When you set the members root page to be /groups/, and set groups to be /members/, the members directory page mistakingly shows the groups root directory.

Edge case, but broken when there's no reason for it to be, because we confidently know the page ID.

I believe this to be a flaw in the way array search for the current component, as it's seeing the slug match and returning a false positive match.

Attachments (1)

3554.01.patch (3.7 KB) - added by boonebgorges 13 years ago.

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
13 years ago

I haven't dug into it, but I'm pretty confident guessing that it has to do with bp_is_current_component(). In order to provide maximum flexibility and backward compatibility, that function looks for matches both to the canonical id ('groups') but also to the page slug, which could be 'members' in the case you've described.

If I'm right, we should just be able to change the order of the conditionals in bp_is_current_component(), so that the canonical id check comes first.

#2 @boonebgorges
13 years ago

I've done some investigation, and my theory about the root cause of the problem is essentially correct.

However, it's not as simple as just swapping the order. The function bp_is_current_component() is written with multiple fallbacks in mind, so that you can feed different values to the function. Let's say that you have the following setup:
$bp->members->root_slug = 'users';
$bp->members->slug = 'dudes';

The function is currently written so that the following three checks will all return true:
bp_is_current_component( 'members' );
bp_is_current_component( 'users' );
bp_is_current_component( 'dudes' );

We did it this way for backpat reasons. Previously, the standard throughout BP was (for the most part - we were inconsistent) to check $bp->current_component == $bp->members->slug. Some plugins followed suit. But sometimes we (and plugins) checked for $bp->current_component == $bp->members->id or simply $bp->current_component == 'members'. The introduction of root_slugs in 1.5 makes this even more complicated. So when we introduced bp_is_current_component() for 1.5, we wanted to provide maximum flexibility to plugin/theme authors.

This is fine and dandy, until we start having crazy setups like the one you suggest. When
$bp->members->root_slug = 'groups';
the current component checks become, essentially, a race to see which screen function gets fired first. That's because bp_is_members_component() and bp_is_groups_component() will *both* return true; bp_is_current_component( 'members' ) is true because of the check against canonical IDs, while bp_is_current_component( 'groups' ) is true because of the check against root_slugs/$bp->pages.

I think that there are three possible strategies for fixing the problem:
1) Ignore it as an extreme edge case.
2) Remove some of the fallback logic, thereby enforcing best practices for bp_is_current_component(). Presumably, the recommended practice will be to use $bp->{$component}->id, or the hardcoded canonical component name, when checking your current component (this is what we do in the bp_is_x_component() functions).
3) Add a special case to bp_is_current_component() that checks for these kinds of cases. In other words, we still allow different kinds of inputs for bp_is_current_component(), but when we detect a clash like the one you've outlined, we fall back on strict $bp->pages/root_slug matches.

IMO: (3) kinda stinks. Seems ad hoc and inelegant, plus it will result in somewhat unpredictable behavior for bp_is_current_component(), as your plugin won't be able to accurately predict what the user's setup will be. Between (1) and (2), I lean toward (2); bp_is_current_component() is new anyway, and we can recommend to plugin authors that they use the wrapper functions bp_is_groups_component() etc when available, and otherwise to use canonical names with custom components, eg bp_is_current_component( 'achievements' ) and not bp_is_current_component( $bp->achievements->slug ). 3554.01.patch is a proposal for the revised logic (along with a corresponding logic change in bp_get_name_from_root_slug()).

My only misgiving is that it's very late in the dev cycle to be making such substantial changes to a function like this, when there's little time for testing, and little time for reaction by plugin developers. So I would be OK with punting the issue for the moment if others think it would be appropriate.

#3 @boonebgorges
13 years ago

  • Milestone changed from 1.5 to 1.5.1

During yesterday's dev chat, I believe it was decided that this enough of an edge case to punt for the moment. After the 1.5 release, let's explore the possibility of doing something like what bbPress does, which is to check for conflicts between core slugs when they are saved (like my number (3) suggestion, but better because it's upstream and not part of bp_is_current_component())

#4 @boonebgorges
13 years ago

  • Milestone changed from 1.5.1 to 1.6

Probably too complex for a bugfix release. If we have time, and can easily use some of bbPress's logic, let's do this for 1.6.

#5 @rogercoathup
13 years ago

Maybe related - I've had the members directory showing incorrectly as well:

if I:

  1. create a standard WordPress page template with members in the filename e.g. my-members.php
  2. create a page based on the template defined in that file
  3. try to view the page - it incorrectly shows the standard members directory (members/index.php), and not my template

Had this on a number of occasions with a 1.5 site. Fixed it by removing members from the filename.

#6 @boonebgorges
13 years ago

  • Keywords 1.7-early added
  • Milestone changed from 1.6 to Future Release

I think it makes sense to take care of these issues once #2086 is taken care of, so that we can take advantage of whatever new saving routine we've got in place.

#7 @boonebgorges
12 years ago

  • Keywords 1.7-early removed

Given the theme compatibility stuff, I'm uneasy with committing to the slug changes for this dev cycle. Moving out of 1.7-early for the moment, though we can reconsider at some point if we have loads and loads of dev time on our hands.

#8 @foxly
12 years ago

I'm pretty sure the new router we wrote for BP fixes this.

F

#9 @boonebgorges
10 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Given:

  • the edge-case nature of this bug
  • the fact that it has not been reported in the wild
  • the fact that it would go away with a switch to rewrite rule

I'm closing as wontfix.

Note: See TracTickets for help on using tickets.