Skip to:

Opened 13 years ago

Closed 11 years ago

#3578 closed enhancement (fixed)

Determine $bp->component->root_slug automatically

Reported by: boonebgorges's profile boonebgorges Owned by: boonebgorges's profile boonebgorges
Milestone: 1.9 Priority: low
Severity: minor Version: 1.5
Component: Core Keywords: dev-feedback
Cc: vivek@…


One of the parameters for BP_Components::setup_globals() is root_slug. But there's no reason (that I can think of) why this should need to be provided manually. The root_slug should *always* match the path of the corresponding item in $bp->pages.

On that note, here are two different proposals:
1) Don't ask for a root_slug at all from component classes. Just take a moment in BP_Component::setup_globals() to get it out of $bp->pages.
2) Use the value out of $bp->pages as a default

I think that we can safely go with (1), but IMO the least we can do is (2). (FWIW, I think that (2) can - and should - be slipped in for BP 1.5, while there is not enough time for (1).)


Attachments (2)

3578-automatic.patch (1.1 KB) - added by boonebgorges 13 years ago.
3578-defaults.patch (761 bytes) - added by boonebgorges 13 years ago.

Download all attachments as: .zip

Change History (11)

#1 @boonebgorges
13 years ago

  • Keywords dev-feedback added

3578-automatic.patch implements my idea (1), where we take the override option away from BP_Component extension classes (if they really need to change it - for what reason, I can't imagine - they can use the filter).

3578-defaults.patch takes the gentler route of providing a default value that can be overridden.

#2 @johnjamesjacoby
13 years ago

There are components that don't have root_slugs that never have them set. Friends, Messages, and Settings are good examples. I'd like to keep the root_slug argument around if/when we move components into custom post types, or if the directory-to-page idea doesn't work in practice.

The other reason was to keep decision making up to the individual component loaders and leave the lower level processing to the BP_Component class. In this instance it wasn't about DRY as much as developer understanding, using the BP core components as examples of how to use it.

The goal is ease of use and clarity of understanding. If you think it's too confusing now, I think 3578-defaults.patch is the best approach. Keep in mind this opens the door into moving each of the assignments towards having defaults in BP_Component::setup_globals() which I'd also like to avoid.

Point being, if the class isn't used 'correctly' is it responsible for making assumptions or does it do exactly what it's told.

#3 @boonebgorges
13 years ago

My "automatic" idea would tasse care of rootslugless components by assigning them an empty string. This is what's already happening anyway, just at the individual component level.

As for the broader question, I'm curious as to why you don't want to provide defaults for these values. It seems to me that the point of BP_Component is to make it as ready as possible to write new components. If we can help plugin authors by making intelligent guesses about values that they will never (or hardly ever) change, why shouldn't we?

#4 @boonebgorges
13 years ago

(Which is to say that I don't really find it *confusing*. I just want to make it as seamless as possible for others. Either way I don't feel ask that strongly about it, I just want to understand the philosophy behind providing a base class that requires more manual work by plugins)

#5 @boonebgorges
13 years ago

  • Milestone changed from 1.5.1 to 1.6

#6 @boonebgorges
13 years ago

  • Milestone changed from 1.6 to Future Release

I still think we should be doing more to help plugin authors make good default decisions about this kind of stuff, but I'm willing to wait to continue the argument :)

#7 @sooskriszta
11 years ago

  • Cc vivek@… added

Have you gotten any feedback on this? Your option 1 seems quite reasonable to me, and I can't think of a reason why the patch (after testing) shouldn't be merged into 1.9

#8 @boonebgorges
11 years ago

  • Milestone changed from Future Release to 1.9
  • Priority changed from normal to low
  • Severity changed from normal to minor

At this point, I don't think we can do option 1 safely, because there may be plugins in the wild that are passing custom 'root_slug' parameters. Going with the 'automatic' patch would break them.

However, the 'defaults' patch would be a seamless addition. And, on further reflection, it's actually quite similar to what we already do with 'slug': we provide a default value ($this->id) that will make sense for most use cases. So I'm going to add it for 1.9 and call this ticket fixed.

#9 @boonebgorges
11 years ago

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

In 7465:

Default to the component directory root_slug in BP_Component::setup_globals()

This change will make BP_Component extensions a bit easier to build, as it will
no longer be necessary to fish the component root_slug out of bp_pages
manually (in the majority of standard cases).

Fixes #3578

Note: See TracTickets for help on using tickets.