Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#7140 closed defect (bug) (fixed)

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

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.6.1 Priority: normal
Severity: major Version: 2.6.0
Component: Core Keywords: has-patch
Cc:

Description (last modified by 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.

Attachments (1)

7140.01.patch (453 bytes) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (13)

@r-a-y
3 years ago

#1 @r-a-y
3 years ago

  • Description modified (diff)
  • Severity changed from normal to major

#2 @johnjamesjacoby
3 years ago

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' ) ) ) {

#3 @r-a-y
3 years ago

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.

#4 @boonebgorges
3 years ago

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.

#5 @dcavins
3 years ago

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?)

#6 @r-a-y
3 years ago

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:

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.

Last edited 3 years ago by r-a-y (previous) (diff)

#7 @DJPaul
3 years ago

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.

#8 @dcavins
3 years ago

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...)

#9 @r-a-y
3 years ago

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.

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


3 years ago

#11 @r-a-y
3 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

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).

#12 @r-a-y
3 years ago

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).

Note: See TracTickets for help on using tickets.