Adapt Group extensions registration to BP Rewrites

[imath]

I suggest to move the bp_register_group_extension() function from the BP_Group_Extension class file to the src/bp-groups/bp-groups-functions.php file.

Using an anonymous function inside the bp_register_group_extension() function to init Group Extensions at bp_init is problematic.

1. I suggest to only use this function to register Group Extension classnames into a specific global (for now! I plan to extend this from the BP Rewrites plugin to manage slug customizations).
2. And use a new function (bp_init_group_extensions()) to init the Group Extensions by looping into this specific global.

It seems to have no regression effects according to my manual tests, and PHPUnit tests are not failing.

Unless one of us feels strongly about this change, I'm pretty confident about it so I'll commit it soon to carry on progressing on BP Rewrites.

[boonebgorges]

Thanks for working on this, @imath ! The change described here seems fine to me at a glance.

You mentioned in the corresponding GitHub ticket ​https://github.com/buddypress/bp-rewrites/issues/9#issuecomment-907216220 that you were considering moving the setup of BP_Group_Extension classes to bp_parse_query. I would be very cautious when considering this change. I've written a number of plugins and customizations that are sensitive to the specific load-order of BP_Group_Extension at bp_init, and moving it all the way to parse_query would almost certainly result in widespread breakage.

[imath]

Hi @boonebgorges,

Thanks a lot for your feedback and the tests you did with some of your plugins with BP Rewrites.

About moving Group extensions registration down to bp_parse_query: I must admit I'm considering it because it seems the easiest way ☺️ for BP Rewrites.

But, I'm fine with leaving the Group Extension registration to happen at bp_init. I'll start checking each method of the BP_Group_Extension API first to see if they are trying to get something that can't be set with BP Rewrites on. For example the get_group_id() method shouldn't be used before bp_parse_query (the origin of the issue reported by @shanebp), and at the same time I can understand the need to adapt the Group Extension behavior according to the displayed Group, so I think we should provide guidances/new methods/functions to do that once the Group can be set.

Our goal is to make this change without breaking any plugins (there might be a few exceptions, e.g. when no tests were made about these few plugins with the BP Rewrites one).

BP Rewrites won't be merged in BP 10.0.0 or even in 11.0.0, I believe. The goal is to release a stable version of BP Rewrites on the WP.org plugins directory to ease testing. And take the needed time, from there, to test as much BuddyPress plugins as possible. We'll improve the BP Rewrites plugin often and until we reach the required level of confidence we need to do the big switch to the WP Rewrite API safely into BuddyPress core.

We still have a quite long road 😅.

[imath]

.2.patch improves first patch making it possible to override the BP Group Extension class from (the BP Rewrites) plugin.

[imath]

In 13101:

Improve the Group Extension API extensibility

• Use a new global to store the registered Group extensions of the Groups component.
• Edit the behavior of the bp_register_group_extension() function so that it only registers custom group extensions into the new global.
• Introduce a new function bp_init_group_extensions() to init the registered Group extensions.

These improvements will make it possible for the BP Rewrites feature as a plugin to allow slug customization of the registered Group extensions.

Props boonebgorges

Fixes #8558

[dcavins]

I've uncovered an issue with this that seems to only "keep" the last group extension registered. I will a test plugin that exposes the problem. If BP is working, when you visit a group, you should see three new tabs: "New Tab" "New Tab 2" and "New Tab 3." If BP is broken, you'll only see "New Tab 3." I've also attached a patch that appears to resolve the issue by not passing the new class by reference in bp_init_group_extensions(). I don't really understand why passing by reference seems to cause issues here when it didn't in the function's previous context. So I'll ask @imath and @boonebgorges to weigh in if they understand the issue.

I've attempted to write a test to check this behavior, but I can't quite figure out how to directly test bp_register_group_extension().

Thanks for your feedback!

[dcavins]

Fix registration of extensions.

[dcavins]

Plugin to test registration of multiple group extensions.

[imath]

Thanks a lot for reopening this ticket to share the details of this issue. I have no idea if passing the Group Extension by reference is needed, I preserved this from the initial function: ​https://buddypress.trac.wordpress.org/browser/branches/9.0/src/bp-groups/classes/class-bp-group-extension.php#L1717

I'll test the patch asap.

[dcavins]

Fix group extension registration. Make the test work. :)

[dcavins]

I've tested this new patch in several versions of php and it seems to be ok. I was surprised all over again at the efficiencies built in to the newer versions of php:

version     page gen time   RAM usage   Query time   Number of queries
PHP 5.6.39  0.61S           64,119kB    0.0083S      44Q
PHP 7.3.5   0.43S           50,963kB    0.0061S      44Q
PHP 7.4.1   0.23S            8,493kB    0.0062S      44Q
PHP 8.0.0   0.12S            8,504kB    0.0059S      44Q

[imath]

Hi @dcavins

1. I've reproduced the issue using your group-extension-hook-tester.php. Without the patch I could only see the "New Tab 3"
2. I applied 8558.4.patch and I confirm it fixed the issue ✅

Let's commit it! Thanks again for your tests, investigations and fix! Amazing work 💪.

PS: I'll start building beta2 around 20:30 UTC, no worries if you couldn't commit it yourself in the meantime, I'll take care of it.

[dcavins]

In 13195:

Improve behavior of bp_init_group_extensions().

In r13101, the behavior of bp_register_group_extension()
was changed to make it possible to modify the behavior via
plugins. However, the same code in a new place behaved
differently, only fully registering the last group extension that
was passed to it. This commit stops passing the group
extension classes by reference, which fixes the issue.

See #8558.