Autoload BP classes

[boonebgorges]

A few versions ago, we moved most of our classes into their own files. Let's take advantage of this by autoloading.

[boonebgorges]

6853.diff​ is a proof of concept. In my very light testing, BP's memory footprint decreases by between 1MB and 1.5MB by switching to autoload, since the only classes loaded into memory are those that are actually needed to display the page.

The POC is a bit ugly because we don't have real namespaces, and our "namespaces" are inconsistently applied. (Though, to be honest, the $irregular_map lookup in the autoload() method is probably *faster* than the path concatenation. It's just harder to maintain in the long run.)

SPL can be disabled in PHP 5.2, so I've left a fallback in place, which uses the existing bp-{$component}-classes.php file.

Any gotchas here? One concern is that the autoload logic - which looks for classes named BP_{$component}_{$foo} against a whitelist of BP components - might hit a false positive in a plugin that has a class name like BP_Groups_Whatever. This *shouldn't* happen because the plugin should be loading its own classes manually before they call them, but there could be a conflict if both BP and the plugin register their own autoload functions.

Is it crazy to do this without PHP 5.3, with mandatory SPL and namespaces? Seems to me it's worth considering, given the memory improvements.

[r-a-y]

Looks good!

02.patch replaces a few older instances of autoload with do_autoload.

Is it crazy to do this without PHP 5.3, with mandatory SPL and namespaces? Seems to me it's worth considering, given the memory improvements.

Since the BP codebase is getting larger and larger, we should try to optimize wherever we can, so big +1!

If we could ignore PHP 5.2, it would probably be better to follow a PSR standard. But, hey, we're tied to WordPress -- for better or worse.

One concern is that the autoload logic - which looks for classes named BP_{$component}_{$foo} against a whitelist of BP components - might hit a false positive in a plugin that has a class name like BP_Groups_Whatever.

Good call. It might be good to write a sample plugin using the BP_Component class to see if we can trip it up.

[slaFFik]

Having BP_{$component}_{$foo} in 3rd party plugins is actually a bad practice. You know, prefix all the things and such. So in my opinion it should not be a blocking concern, as bad practices are a point of potential failures anyway (regularly, and developers should know that).

@r-a-y I see var_dump()s in your patch: src/bp-loader.php::541,590

[DJPaul]

There are admin classes we ought to support if we're going to do this, as well as template classes (that need splitting out into individual files -- there's another ticket for that).

Other than those two BP_Group classes, the others are all in the Core component, right? I am tempted to suggest we should rename those but that idea probably needs to wait until we discover what odd cases exist in the admin and template classes.

[r-a-y]

@slaFFik - Oops! :) Patch is updated to remove the debug lines.

[boonebgorges]

Replying to slaFFik:

Having BP_{$component}_{$foo} in 3rd party plugins is actually a bad practice. You know, prefix all the things and such. So in my opinion it should not be a blocking concern, as bad practices are a point of potential failures anyway (regularly, and developers should know that).

I agree with this, and I don't think it'll be a major issue.

Replying to DJPaul:

There are admin classes we ought to support if we're going to do this, as well as template classes (that need splitting out into individual files -- there's another ticket for that).
Other than those two BP_Group classes, the others are all in the Core component, right? I am tempted to suggest we should rename those but that idea probably needs to wait until we discover what odd cases exist in the admin and template classes.

See #6870, where I've outlined the task. IMO, we should do no renaming at this point - it's better to have a hash of inconsistently named classes than to break backward compatibility for the sake of aesthetics. When the day comes for full PSR-4 compliance, maybe we can talk about changing class names, and building a compatibility layer for it :)

I think it makes sense to wait for #6870 before implementing autoload.

[dcavins]

Thanks for taking this on. Sounds like a pretty huge efficiency improvement.

[boonebgorges]

In 10530:

Rename BP_Walker_Category_Checklist file.

The filename should match the class- standard, for easier loading.

See #6870, #6853.

[boonebgorges]

6853.2.diff​ is an updated patch, after the moves in #6870. I made the necessary additions to $irregular_map, and wrapped all new require statements in a do_autoload check.

Rough benchmarks are showing a pretty consistent memory savings of between 1.5 and 2MB, on a local dev installation that has about a zillion plugins running. On a front-end load, memory usage goes from just under 50MB to just over 48MB. If we assume that, say, half of that 50MB is directly attributable to BuddyPress, then we're reducing BP's memory footprint by 7 or 8 percent with autoloading. And with some further rearrangement in the future (so that components are loaded in a sleeker way) we could get that number down even further.

Comments and feedback are welcome on the technique used. And if anyone sees any gotchas that I'm missing, please speak up. It'd be nifty to put sneak this into 2.5.

[r-a-y]

03.patch fixes a fatal error with BP_Component being not found when SPL is disabled. I'm requiring the class in bp-core-loader.php as this is the earliest the class is required.

I'm also doing a class_exists() check in bp-core-component.php as some plugins might be manually requiring /bp-core/bp-core-component.php in their code to extend BP_Component (unlikely, but better to be safe than sorry!).

[boonebgorges]

Thanks, @r-a-y ! I was already feeling pretty shaky about sneaking this in at the last minute, and your patch is going to seal the deal. Let's do it first thing for 2.6, so we have plenty of time to shake things out.

[boonebgorges]

In 10652:

Autoload BuddyPress classes.

Backward compatibility with SPL-disabled installations is included.

See #6853.

[needle]

Since [10652], I get a "PHP Fatal error" when checking for the existence of one of my plugin classes called "BP_Groups_CiviCRM_Sync" - i.e. the with the following code:

if ( ! class_exists('BP_Groups_CiviCRM_Sync') ) {
  // do something
}

Does this change now mean that plugin authors cannot (or at minimum should not) name their classes "BP_Componentname_Etc"?

[boonebgorges]

Thanks for testing against trunk! That's why I put it in early :)

I had a feeling that this would probably happen. In general I'd suggest using a unique prefix for your class/function names. But it's not acceptable for us to fatal like this. The simple answer is to do a file_exists() check before we attempt to autoload. I'd originally avoided this to minimize I/O, but I just did some testing and the added overhead is negligible.

[boonebgorges]

In 10653:

When autoloading, check for the existence of a file before including.

This avoids stepping on the toes of plugins that are using BP-ish prefixes for
their class names.

See #6853.

[needle]

Replying to boonebgorges:

I had a feeling that this would probably happen. In general I'd suggest using a unique prefix for your class/function names. But it's not acceptable for us to fatal like this. The simple answer is to do a file_exists() check before we attempt to autoload. I'd originally avoided this to minimize I/O, but I just did some testing and the added overhead is negligible.

Thanks, the latest commit fixes the issue for me. I could rename my classes, but I have no idea what the knock-on effects might be. Will follow the naming advice in future.