Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#6083 closed enhancement (fixed)

Perhaps split up classes in existing component-class.php to separate files

Reported by: DJPaul Owned by: DJPaul
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Core Keywords:
Cc: tw2113@…

Description

Some of the -class.php files in the components have many classes in them. While historically okay, with all the new development, it feels to me that the class files are getting pretty long, and I think the larger a file is, the harder it is to understand the code.

WordPress core has a nice pattern of using class-whatever.php for separate classes. It keeps thing manageable in terms of removing distractions of other bits of code in the same file, and the class name in the file name makes some IDEs' features of opening a file by name possible (for example, cmd+p in Sublime Text).

I'm suggesting that we adopt a one class per file approach, and re-factor all the existing classes for this. We could either keep the files in the same directory as the rest of the component, or create a new sub-directory (e.g. bp-groups/classes/class-paul.php).

Change History (16)

#1 @boonebgorges
5 years ago

Yes please. I've been meaning to suggest the same thing for years now.

One class per file sounds perfect. Creating a subdirectory classes seems OK to me, but then I think maybe we should drop the class- prefix for the files. I don't feel strongly about this either way.

#2 @tw2113
5 years ago

  • Cc tw2113@… added

#3 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.3

Let's do this in 2.3. It'll give us time to decide how exactly we want to do this.

#4 @DJPaul
5 years ago

  • Owner set to DJPaul
  • Status changed from new to assigned

#5 @johnjamesjacoby
5 years ago

  • Component changed from Component - Any/All to Tools - Code Improvement

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


5 years ago

#7 @DJPaul
5 years ago

Per above comment, this was discussed at the last dev chat and derailed into a larger discussion about file naming conventions for the entire component. I'd like to limit this particular ticket to splitting up the bp-component-classes.php files. Using Activity as an example:

  • bp-activity/bp-activity-classes.php would be split by class into a new bp-activity/classes/ subfolder.
  • bp-activity/bp-activity-classes.php would require each of those new class files in the same order that they are defined today (a kind-of autoloader, if you like). This prevents breaking any custom code that is directly including bp-activity-classes.php (that particular concern is out-of-scope of this ticket, but I wanted to briefly mention it).
  • For example, we would add bp-activity/classes/class-bp-activity-activity.php, bp-activity/classes/class-bp-activity-query.php, bp-activity/classes/class-bp-activity-feed.php, etc.
  • I'm proposing that this change would ONLY affect classes explicitly within the bp-component-classes.php files. Any other classes (for example BP_Activity_Template, which is in bp-activity-template.php would remain unchanged for now. We could in future decide to move those classes out of those other files.

#8 @boonebgorges
5 years ago

DJPaul - The plan you've laid out here looks great to me. I like the idea of limiting the current changes to what you've suggested here, and leaving other filename/organization issues for future versions.

#9 @djpaul
5 years ago

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

In 9485:

Split each component's classes file, and move each individual class into its own file.

While historically manageable, the previous approach of having the majority of each component's classes in the same file is growing unwieldly with each new version of BuddyPress, and causing an avoidable increase in code complexity.

The existing -classes.php files are now a loading wrapper for the components' classes.

This change only affect classes that were explicitly declared within the -classes.php files. Any other classes, for example those in some components' template functions files, remain untouched for now.

Fixes #6083

#10 @r-a-y
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The __DIR__ constant is only available in PHP 5.3+.

Since we brought back PHP 5.2 to our unit test builds in r9504, this is causing an issue with our tests failing in PHP 5.2.

Perhaps switch out __DIR__ with BP_PLUGIN_DIR?

#11 @johnjamesjacoby
5 years ago

Might this be why our 5.2 tests occasionally fail? Because of use of __DIR__ to set WP_ROOT_DIR in our test suite?

#12 follow-up: @johnjamesjacoby
5 years ago

If we use BP_PLUGIN_DIR we'll need to hardcode the component directory paths. We can use dirname( __FILE__ ) in the meantime as a direct replacement for __DIR__.

#13 in reply to: ↑ 12 @boonebgorges
5 years ago

Replying to johnjamesjacoby:

If we use BP_PLUGIN_DIR we'll need to hardcode the component directory paths. We can use dirname( __FILE__ ) in the meantime as a direct replacement for __DIR__.

+1

#14 @DJPaul
5 years ago

I had absolutely no idea DIR was a PHP 5.3 thing. Sorry for the breakage.

#15 @djpaul
5 years ago

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

In 9514:

Replace use of __DIR__ as it is not supported on PHP 5.2.

Fixes #6083 again.

#16 @DJPaul
3 years ago

  • Component changed from Tools - Code Improvement to Core
Note: See TracTickets for help on using tickets.