Class autoloading needs to check if the component is active before including the class

[r-a-y]

For some people that have the groups component disabled, the BP_Group_Extension class is still loaded, causing a fatal error in some instances:
​https://buddypress.org/support/topic/fatal-error-on-a-buddypress-2-6-0-upgrade/#post-255171

The problem is class_exists() calls are no longer relevant with class autoloading. This causes the class to be loaded, but calls to component functions will result in a fatal error.

For example:

Fatal error: Call to undefined function bp_get_current_group_id() in /wp-content/plugins/buddypress/bp-groups/classes/class-bp-group-extension.php on line 484

To duplicate the problem, disable the groups component and add the following to wp-content/plugins/bp-custom.php:

add_action( 'bp_include', function() {
        if ( class_exists( 'BP_Group_Extension' ) ) {
                class My_Group_Extension extends BP_Group_Extension {
                }

                bp_register_group_extension( 'My_Group_Extension' );
        }
}, 99 );

Load a page on the frontend and you should get the same fatal error as referenced above.

Attached patch is an attempt to fix this issue. Note the ugliness to check if PHPUnit is in use.

[johnjamesjacoby]

Patch works for me.

Are you able to update it with parenthesis around each separate condition?

Basically:

if ( ( 'core' !== $component ) && ( false === bp_is_active( $component ) ) && ( false === function_exists( 'tests_add_filter' ) ) ) {

[r-a-y]

Using parenthesis would make sense if we were checking multiple conditions for one clause.

For example:

( true === bp_is_active( 'members' ) || true === bp_is_active( 'groups' ) )

But since the patch is only checking single conditions, I don't think it is necessary.

I'm kind of agnostic either way, so whatever everyone thinks is best is okay with me.

[boonebgorges]

I'm kind of agnostic either way, so whatever everyone thinks is best is okay with me.

Seems to me like we could embrace simplicity and use !.

if ( 'core' !== $component && ! bp_is_active( $component ) && ! function_exists( 'tests_add_filter' ) ) { 

Fix seems good to me. Nice catch.

[dcavins]

Thank for this note: "The problem is class_exists() calls are no longer relevant with class autoloading."

How widespread/severe of a problem is this? (How soon do we need to do 2.6.1?)

[r-a-y]

How widespread/severe of a problem is this? (How soon do we need to do 2.6.1?)

This issue is mostly only applicable for:

• Sites that do not have the Groups component active; and
• Has a BP plugin activated that also extends the BP_Group_Extension class; and
• That BP plugin loads their group extension by only checking if class_exists( 'BP_Group_Extension' ) instead of bp_is_active( 'groups' )

So that should narrow down the problem to a relatively small subset of sites.

I did a quick search on Github for wp.org plugins only doing class_exists( 'BP_Group_Extension' ) checks without bp_is_active( 'groups' ) and came across the following:

• BP Group Suggestions - ​https://github.com/wp-plugins/bp-groups-suggestions/blob/master/loader.php#L22
• BuddyPress Activity Graph - ​https://github.com/wp-plugins/buddypress-activity-graphs/blob/master/buddypress_activity_graph.php#L98
• BP Group Documents - ​https://github.com/wp-plugins/bp-group-documents/blob/master/buddypress-group-documents.php#L10
• BuddyForms Attach Post with Group - ​https://github.com/wp-plugins/buddyforms-attach-posts-to-groups-extension/blob/master/includes/group-extension.php#L2
• VideoWhisper - ​https://github.com/wp-plugins/videowhisper-video-presentation/blob/master/videowhisper_presentation.php#L1654

There were also many other instances on Github (including some by BP contributors!).

I think we should strive for a minor release next week during dev chat.

[DJPaul]

I do hope -- for the ones on Github -- part of our "fix" is to tell these plugins to use bp_is_active, otherwise we are hacking around other's poor code.

[dcavins]

Ha ha, I'm sure some of those on github are mine, based on the examples in Ye Olde Skeleton Plugin sample plugin (maybe we should update that to reflect the modern state of things?), and on examples in our own Codex:
​https://codex.buddypress.org/developer/group-extension-api/

I'm not sure that calling that "bad code" is fair. It was accurate and was a reasonable safety net, right up until we started autoloading, ha ha.

I'll fix those examples in the Codex. So do we want to recommend doing both checks:
if ( bp_is_active( 'groups' ) && class_exists( 'BP_Group_Extension' ) ) {}
or are we recommending that
if ( bp_is_active( 'groups' ) ) {}
is sufficient? (Group Extension class was apparently part of BP 1.1 (amazing!) and was massively updated for 1.8, so...)

[r-a-y]

I'm not sure that calling that "bad code" is fair. It was accurate and was a reasonable safety net, right up until we started autoloading, ha ha.

This. Definitely not anyone's fault. We were basically telling people to use class_exists() calls in the older Skeleton Component plugin.

Ha ha, I'm sure some of those on github are mine

It's not just you :)

based on the examples in Ye Olde Skeleton Plugin sample plugin (maybe we should update that to reflect the modern state of things?), and on examples in our own Codex:

​https://codex.buddypress.org/developer/group-extension-api/

Yes, let's definitely do this in both the codex and the Skeleton Component.

if ( bp_is_active( 'groups' ) ) {}

IMO, this should be sufficient.

[r-a-y]

In 10918:

Core: In our class autoloader, check if the component is active before attempting to include it.

In #6853, we introduced class autoloading to help reduce the amount of
memory used by BuddyPress on any given page request.

This caused a problem in some BuddyPress plugins that previously relied on
a class_exists() check to determine if a BuddyPress component is active
or not. For example, if ( class_exists( 'BP_Group_Extension' ) ) {}.

In BuddyPress 2.6.0, class_exists( 'BP_Group_Extension' ) will now return
true all the time by default. In some instances, this can cause a fatal
error if the Groups component is disabled.

This commit adds a component check before including the class, while
allowing core classes and PHPUnit to load as usual.

Plugin developers should read the following development post for more
details:
​https://bpdevel.wordpress.com/2016/06/28/class-autoloading-and-what-this-means-for-plugin-developers/

Fixes #7140 (2.6-branch).

[r-a-y]

In 10919:

Core: In our class autoloader, check if the component is active before attempting to include it.

In #6853, we introduced class autoloading to help reduce the amount of
memory used by BuddyPress on any given page request.

This caused a problem in some BuddyPress plugins that previously relied on
a class_exists() check to determine if a BuddyPress component is active
or not. For example, if ( class_exists( 'BP_Group_Extension' ) ) {}.

In BuddyPress 2.6.0, class_exists( 'BP_Group_Extension' ) will now return
true all the time by default. In some instances, this can cause a fatal
error if the Groups component is disabled.

This commit adds a component check before including the class, while
allowing core classes and PHPUnit to load as usual.

Plugin developers should read the following development post for more
details:
​https://bpdevel.wordpress.com/2016/06/28/class-autoloading-and-what-this-means-for-plugin-developers/

Fixes #7140 (trunk).