Only load component action and screen code when we're on the component's page

[r-a-y]

We should only load a component's action and screen code when we're on the component's page.

This can save us some memory.

Attached patch does this for the activity component. I've introduced a new method called late_includes() that runs on the 'bp_setup_canonical_stack' hook because that is the place where we've ensured that $bp->current_component and $bp->current_action is fully set up.

In the patch, I've moved some code that is not specific to the activity component's action or screen code to bp-activity-functions.php temporarily until I can figure out a better place to put them. Perhaps, create a new file called bp-activity-hooks.php?

[DJPaul]

Interesting idea. Might break code that does a function_exist for something in the affected files, but might not. I don't think this'd affect a huge number of sites.

There's also AJAX handlers or form submission handlers that may be in these locations. For example, bp_ajax_get_suggestions needs to work from anywhere.

If the idea is to be a memory optimisation, we should benchmark this a bit with the current patch to see what impact it has (or doesn't). But I don't know how to reliably measure memory usage for PHP.

[DJPaul]

I am excited about the potential of this ticket, albeit it with the concerns I stated previous that probably just need time to test and work through, and figure out what to do.

@r-a-y If we want to progress this, perhaps we could start by relocating functions that we feel are in the wrong files?

[r-a-y]

If we want to progress this, perhaps we could start by relocating functions that we feel are in the wrong files?

Sounds like a good plan. For things like Akismet, AJAX and the like, do we stuff them in bp-activity-functions.php or create a new file for hooks such as bp-activity-hooks.php?

Akismet could also be loaded conditionally with bp_is_active( 'activity', 'akismet' ). It's already in another file. Maybe we can move the Akismet-related code in bp-activity-actions.php to bp-activity-akismet.php. I'll experiment and report back.

[r-a-y]

In 11102:

Activity: Move Akismet loader to bp-activity-akismet.php.

Previously, bp_activity_setup_akismet() was placed in
bp-activity-actions.php. The actions file isn't a proper place to put
this code.

This commit moves this function to bp-activity-akismet.php and also
removes some duplicate conditional code. BP_Activity_Component already
checks if Akismet is loaded, so no need to re-duplicate this logic.

This is part of some prep work to conditionally load a component's action
and screen code when necessary.

See #7218.

[r-a-y]

r11102 moved the Akismet loader to bp-activity-akismet.php. akismet.patch moves some additional hardcoded Akismet code from bp-activity-functions.php over. There are still a few hardcoded instances left in bp-activity-functions.php, but I left them for now.

---

02.patch is basically the same as 01.patch, but fixes an issue with the activity email options not showing up on the "Settings > Email" screen. Had to move the activity email <table> code to bp-activity-notifications.php. Patch also moves the suggestions AJAX hook out of bp-activity-actions.php to bp-activity-functions.php as mentioned in comment:1.

[DJPaul]

@r-a-y Let's look at this in 2.8, and decide if and how we want to do it. I know you are keen on it, but I think we should have a dev-chat discussion at some point because it's somewhat of a structural change.

[r-a-y]

I'm still keen on this.

Let me know what I can do to make this happen! :)

[boonebgorges]

I like this idea, and I think that limiting it to action/screen handlers is a good way to limit the possibility of compatibility breaks.

If we wanted something a bit more idiomatic than the "late includes" strategy, we could move the guts of the screen functions into classes and turn the procedural functions into wrappers. The classes would then be autoloaded. Not quite the memory savings as what you're proposing, but also a bit less subject to breakage (and no special cases needed for unit tests).

[DJPaul]

We never really gave much feedback on this, let's try to pick it up for 2.9.

[r-a-y]

I've refreshed patches for the Activity, Friends, Groups and Members components.

For the Activity component, I moved the RSS feed code to a new file called bp-activity-feeds.php and loading that only on activity RSS pages.

For the Members component, I moved the registration and activation screen code to a new file called bp-members-register.php and loading that only on those pages.

For the Groups component, I moved the legacy forum screen code to bp-groups-forums.php and am loading this if the Legacy Forums component is active.

For the Friends component, the activity code is only loaded if the Activity component is active.

For all components, the action and screen code is only loaded if necessary. Also, I am conditionally loading the <table> markup used on the "Settings > Email" page as chances are this code isn't going to be used outside of that page.

---

I haven't done this for the Messages, Settings and Notifications components yet, but I will once we've decided we like this approach.

[r-a-y]

@boone or @jjj - Can one (or both) of you take a look at this and let me know if this is viable?

I'm already doing this approach in a few plugins with success. This would most benefit multisite users since we wouldn't be loading a bunch of unnecessary code on every sub-site.

[boonebgorges]

General approach looks good to me. bp_is_active() checks for intercomponent dependencies is definitely good, regardless of the screen function stuff. For screen functions, I still think that autoloading would be more elegant, but that's something we can approach in the future, and shouldn't block these improvements ​https://buddypress.trac.wordpress.org/ticket/7218#comment:16

Let's be sure to mention the changes in the beta announcement, and/or urge plugin devs via bpdevel to test for compatibility issues.

Also, I am conditionally loading the <table> markup used on the "Settings > Email" page as chances are this code isn't going to be used outside of that page.

This code is loaded during the save routine in addition to the screen function, for validation reasons. I haven't checked myself, but you should be sure that this validation continues to work. ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-settings/bp-settings-functions.php?marks=98#L83

[r-a-y]

A few dev chats ago, we talked about moving the screen and action code to classes so we can utilize class autoloading.

I've done this for the Activity and Groups components in the groups-autoload.patch and activity-autoload.patch patches.

The result is the filesize for bp-activity-actions.php and bp-activity-screens.php is now only 6KB individually, instead of 28KB and 16KB respectively.

The filesize for bp-groups-actions.php and bp-groups-screens.php is now only 7KB and 10KB respectively, instead of 22KB and 53KB.

This cuts down the Activity and Groups screen code by 75%!

activity-autoload.patch also requires late-includes.patch due to offshoring the activity RSS feed code until we are on an actual feed page.

Existing unit tests pass. Let me know what you think.

[boonebgorges]

Thanks for working on this, @r-a-y! I think it's moving in the right direction.

You opted for a separate class for each router. Any specific reason why? I'd imagined maybe a single BP_Activity_Actions class, with (static?) methods for each router. This would keep our number of files lower. It would also prevent us from doing lots of logic inside of a __construct() method, which makes it easier to imagine writing tests for these at some point in the future.

What do you think? I don't feel strongly about it.

[r-a-y]

You opted for a separate class for each router. Any specific reason why?

If we added a singular BP_Activity_Actions class, we would run into the same problems as before (albeit less so).

Individual classes ensures that we only load the code we need to run when we hit a specific action.

It would also prevent us from doing lots of logic inside of a construct() method, which makes it easier to imagine writing tests for these at some point in the future.

For writing tests, we could pass parameters to the constructor. I did these for a few of the activity classes because some of the functions like bp_activity_action_delete_activity() required a return value.

[r-a-y]

Bumping to 3.0 and marking as early.

[r-a-y]

I'm committed to getting this in 3.0.

However, I need a gutcheck on the new class proposal. Read comment:28 and on.

There has been some talk about using PSR-4 in the BP WP-CLI project. This got me thinking about PSR-4 for BP, but such a change would affect our current classloader and the entire BuddyPress codebase. Definitely out-of-scope for this ticket, but thought I'd mention it.

[boonebgorges]

Thanks for continuing to pursue this, @r-a-y.

I've reviewed the patches and the end of the conversation above. I think the approach you've put in the most recent patches feels fine. My questions in comment:30 were really just questions, and I'm happy with your judgment here.

The talk about PSR-4 in wp-cli-buddypress is pretty early. It probably makes sense for the whole team to have a conversation about this, but this conversation will be complex (what about WP coding standards, for example?) and may not happen soon. Also, we may decide only to enforce new naming conventions for new features, like CLI commands or API-related stuff. So, I'd suggest going with what you've got, which matches our current conventions.

[DJPaul]

I am very excited by what this change represents.

I've been trying to find a way to advance the state of the BuddyPress codebase for quite some time, while keeping most of our current (decisions? limitations? goals?) intact. What you've done here @r-a-y is better than the few ideas I've come up with (and mostly kept to myself), but also inspires me by setting a vision for the what the future of the codebase can look like.

Modernising the codebase needs to be approached with care. It starts with what you've done here, and for the current patches, I'd like to offer just a couple of comments for review:

• In each new class, we shouldn't not put the implementation inside the constructor. This can cause very subtle bugs regarding null-value (future?) class properties in certain load-order situations. That sounds vague, I can't remember the precise details, but I got schooled by this way back in 2009, just as I started using BuddPress, and I remember the incident vividly (hack day at my first WordCamp!).
• In one of Boone's comments, he's suggested this and said maybe use static methods. I'd like to suggest we use regular instance methods.

The flexibility this gives us, in terms of being to re-implement and re-name these classes in the future, is very liberating compared to what we'd have to do today.

After these changes go in, we can then plan a modernisation strategy, the first steps of which involves adopting, more fully, dependency injection. In the long run, we'll be able to get to the point where we can retire the $bp global, because it'll be passed directly to classes that need it.

In all, this change will set us on a road to making BuddyPress more testable, more clear, and more decoupled. Bravo, @r-a-y!

[DJPaul]

@r-a-y I'm happy to help contribute some time to get this ready, once you've had time to digest my above comments -- do you have this on a Git branch somewhere?

[r-a-y]

No Git branch I'm afraid. I've been waiting for things to die down on the bp-forums retirement front before refreshing my patches and looking into this some more.

About the proposed new class changes, perhaps you can provide a code sample on how you see things structured.

Are you thinking something like:

<?php
// sample new action class
class BP_Activity_Action_Delete {
        // intentional, empty constructor
        protected function __construct() {}

        // static init
        public static function do( $activity_id ) {
                return $whatever;
        }
}

// In bp-activity-actions.php, we'd call on the action class here
function bp_activity_action_delete_activity( $activity_id = 0 ) {

        // Not viewing activity or action is not delete.
        if ( ! bp_is_activity_component() || ! bp_is_current_action( 'delete' ) ) {
                return false;
        }

        if ( empty( $activity_id ) && bp_action_variable( 0 ) ) {
                $activity_id = (int) bp_action_variable( 0 );
        }

        // Not viewing a specific activity item.
        if ( empty( $activity_id ) ) {
                return false;
        }

        return BP_Activity_Action_Delete::do( $activity_id );
}
add_action( 'bp_actions', 'bp_activity_action_delete_activity' );

[DJPaul]

Let's check my understanding first:

My understanding the intent of this ticket is to avoid loading components' action and screen code (files) when we're not on a page route that requires it. Because of how the codebase uses the bp_actions hooks etc to check if it needs to handle the current request, those callbacks always need to be loaded, but the implementation handlers could be moved elsewhere.

And that you've decided that moving the implementation into classes, and relying on a class autoloader, is the best approach.

Broadly, I agree with you, but the devil's in the details. This is what @boonebgorges commented previously:

I'd imagined maybe a single BP_Activity_Actions class, with (static?) methods for each router. This would keep our number of files lower. It would also prevent us from doing lots of logic inside of a construct() method, which makes it easier to imagine writing tests for these at some point in the future.

As I mentioned previously, I think using static methods for this is bad, but I do agree with Boone's suggestion we have a single class, rather than one class per function, which is what your patches currently do (and surely must be some sort of class loader anti-pattern).

Here's my take on your code sample:

<?php
class BP_Activity_Action {
  public function action_delete( $activity_id ) {
    return $whatever;
  }

  public function action_foobar( $activity_id ) {
    return $whatever;
  }
}

function bp_activity_action_delete_activity( $activity_id = 0 ) {
  // ...
  $thing = new BP_Activity_Action();
  return $thing->action_delete( $activity_id );
}
add_action( 'bp_actions', 'bp_activity_action_delete_activity' );
}}

[r-a-y]

Your understanding of the ticket is correct.

About your code example, I don't really mind if we use either a static or non-static method here.

The only thing I care about is using separate classes instead of having an action or screen God class. We're used to using one class to cover a lot of things, but we should preferably move away from this type of class going forward. I touch on this a bit in comment:31.

I understand having everything consolidated is easier to manage though. If another lead dev agrees about using a God class, I'll move everything into one action / screen class for this ticket.

[boonebgorges]

I have no strong opinions about class organization. I will say that the term "God class" feels like an exaggeration here, as does "anti-pattern". The methods/classes in question are closely linked in functionality, and I don't see an architectural principle that very clearly dictates the "right" answer. We're trying to balance performance, organization, ease-of-maintenance, and this will require trade-offs.

Given that we autoload classes, it's likely that more-but-smaller classes will provide a slight memory benefit, as @r-a-y suggests. I also lean toward not doing so much inside of a __construct() method, but for these kinds of specialty classes, I don't think it makes a difference.

Unless @DJPaul feels strongly otherwise, I'm fine with the approach in the most recent patches.

[DJPaul]

Let me think about the god object approach a bit, now I know what it's called, I can go learn about it.

[DJPaul]

I’ve thought about this and @r-a-y I don’t object to your proposal to have lots of small classes instead of one large one. I think it’s more important to use non-static and implementation outside the constructor, please.

[DJPaul]

Just a thought - as we are introducing new classes for this ticket, after the work here is done, i’m going to start a discussion to talk about introducing proper namespacing for these new classes.

[r-a-y]

Patches have been refreshed for v3.0.

activity-v3.patch needs to be applied first, then groups-v3.patch can be applied afterwards.

Please let me know if any changes are needed to the class names or to the way the classes are initialized.

One thing I found is the bp_ajax_get_suggestions() function currently resides in the Activity component. Perhaps this should be moved to Core or Members instead?

[DJPaul]

bp_ajax_get_suggestions() - yes. Probably Members? Don't feel strongly.

@r-a-y Patch looks good, it's ok to commit. We can tweak class names later if we need to; patch is too large for anyone to give meaningful feedback to that particular aspect at the moment, IMO. init() thing is fine - thanks for humouring me on that.

I'm going to benchmark the affect of this patch tomorrow for some basic numbers.

[DJPaul]

I have no idea what patches are required due to the inconsistent file naming. I assume only 7218.groups-v3.patch and 7218.activity-v3.patch are required.

I'm also annoyed at trying to look at such a large patch without a Git branch to play with, so I've made a *temporary* Github repo with these two patches and pushed them to: ​https://github.com/paulgibbs/bptemp/tree/ray-7218

[r-a-y]

I have no idea what patches are required due to the inconsistent file naming. I assume only 7218.groups-v3.patch and 7218.activity-v3.patch are required.

I mention what is needed in comment:45. I've also removed all other previous patches from this ticket so it is clear.

I'm also annoyed at trying to look at such a large patch without a Git branch to play with, so I've made a *temporary* Github repo with these two patches and pushed them to: ​https://github.com/paulgibbs/bptemp/tree/ray-7218

Thanks for pushing it to a Git repo for those of you needing to test. Let me know if anything needs to be altered.

[boonebgorges]

Let's move forward with this ticket. The patches are huge and keep getting stale. Let's get it in.

Benchmarks

I ran some primitive benchmarks to gauge memory + time changes. (I set up a logger that captures memory usage and elapsed wall time in a log file at register_shutdown_function(), and then I used Apache Bench to hit a URL - in this case, example.com/activity/ - 100 times, to gather averages.)

A: Trunk
B: Trunk with activity-v3.patch
C: Trunk with activity-v3.patch + groups-v3.patch

   Avg Mem  | % Diff Mem | Avg Time | % Diff Time
A  60,436KB |     -      | .476668s |     -
B  60,336KB |   -0.17%   | .476679s |   +0.002% 
C  60,136KB |   -0.50%   | .468618s |   -1.69%

Worth noting that these percentages are against the entire memory/time footprint of the WP installation (with many plugins active, etc). The percentage of BP-specific resources that are saved by this change would be larger. And as we add components, the numbers will get larger.

Obviously these are rough numbers, but I think it shows that the improvements are meaningful enough to make the project worthwhile.

Questions

I'll be combing through the patches one component at a time (there's a lot there!) and I'll have a couple of questions before giving a final signoff on each.

1. The checks that remain in the -screens.php and -actions.php functions are mostly of the form bp_is_current_component(), etc. In other words, mostly "routing" logic - only load if we're in the right place. I think this makes sense. But in a number of cases, you're also checking is_user_logged_in(). To my mind, this is a permissions check, and permissions checks should be kept together, to make future security reviews easier to reason about. @r-a-y Do you have thoughts about that, or am I overthinking?

2. The only potential compatibility breaks I see in 7218.activity-v3.patch​ are related to the new -feeds file. That is: Screen/action functions that are converted to wrappers for classes will continue to be available as they always were. But functions moved to a new file bp-activity-feeds.php and then not loaded until later are potential vectors for breakage. I assume that the reasoning behind bp-activity-feeds.php and the late_includes strategy is to pare a bit more stuff out of the initial pageload. This is sensible, but in my opinion, it's too much engineering for a very modest gain. Perhaps new functionality could be introduced in a late-loading way, but the edge-case potential for breaks leads me to think that maybe we can just keep the feed functions in -actions.php. @r-a-y Have I understood this correctly? What do you think about rolling this change out, at least to simplify the specific task at hand?

3. Similar to 1 - 7218.groups-v3.patch​ has some permissions checks bp_user_can_create_groups(), some nonce checks, and even does some bp_core_redirect() logic. For consistency, I'd be in favor of moving this logic into the new classes.

4. Not a question, but good catch on bp_groups_update_orphaned_groups_on_group_delete() :)

5. groups_screen_notification_settings() is not really a screen function, as it doesn't load a template. I'd favor moving it elsewhere, and not treating it with a Screen class. Same with bp_ajax_get_suggestions().

@r-a-y I know that a few of these suggestions require a non-trivial amount of work (especially 1 and 3) so please let me know your thoughts before rushing in. I can help to update the patches if that eases the pain at all :-D

[johnjamesjacoby]

I still don't like the changes proposed here, and 1% doesn't seem significant enough to me.

It also seems like a "classes are better" approach that gets negated when PHP eventually improves the way files and/or functions are loaded. We'll have introduced classes we're stuck with maintaining that don't actually do what classes are intended to do.

I also have a hunch the next bit of code we write to try to mask this complexity will probably negate the 1% improvement @boonebgorges found above.

I'm still not convinced this is worth it, but I don't want to be a blocker towards motivation and/or progress.

---

What I'd want to see:

• A base class that provides some value to all these screens and actions
• Consider renaming classes away from Action to avoid confusion with WordPress action hooks
• Consider breaking mono-methods up into ones that: check permissions, check the correct screen/action are being called, call a display() method for their output, etc...
• The bp_actions and bp_screens hooks are already abstractions of the bp_uri routing system. Is there anyway these classes can talk directly to that instead of being stubbed in the way they have been?

---

Because of the functional nature of action & filter hooks, we'll never completely get to the point of conditionally loading code/files/functions/classes until we include a dictionary of hooks to files. Other projects (Woo, EDD) are thinking of trying this, and I'm cautioning them against it, too, at least today.

A more complete, albeit totally insane, approach would be to:

• Create wrappers for add_action/add_filter/do_action/apply_filters and hook everything into a central routing system
• Listen for hooks to be executed
• Magically include relative files just-in-time as their relative hooks are called

To do this, we'd probably want to:

• Create a script that generates an autoload-stuff.php file, based on what functions are hooked in where and when
• Add that script to our grunt build process, so it stays up to date as new things are hooked in
• Include that autoloader, which is basically just a big array of files to function calls

---

My above hunch of overhead vs. value applies to the proposed patch and my terrible idea. After introducing this extra code complexity, what does BuddyPress (or it's community of users/developers) gain? Is anything actually easier to use/understand? Have we increased performance or reduced consumption by more than what it took to support it? Are these classes easier to interact with? Have we architected an inviting environment to increase BuddyPress adoption?

[r-a-y]

Thanks for the feedback everyone.

Replies to @boonebgorges follow:

1. is_user_logged_in() checks are to improve the load time for logged-out users. That's why they do not reside in the classes. If we want to unit test these new classes with is_user_logged_in(), we can add additional is_user_logged_in() checks to the class files.

2. late_includes() method strategy was part of an earlier idea to offshore all of our action and screen code until we've actually landed on the component's page. This would have increased our memory savings, but due to concerns with this strategy and to be safe, I compromised on a class-autoloading strategy to prevent potential breakage. The reason I separated activity RSS feeds in the late_includes() method is RSS feeds likely are never touched by other plugins so the potential for breakage is very low. I would prefer all future code to be written in a way that utilizes the late_includes() strategy, but if that isn't desired, I can roll back these changes as well.

3. I had a hard time thinking if nonce checks should be in the class or not. I think we can move the nonce and permissions checks to the class. Don't really have an issue with this.

5. Yeah, could use your recommendation on naming for this. Or we just omit the new class additions for these functions.

---

Replies to @johnjamesjacoby follow:

As I mentioned in my reply to Boone in point 2, I prefer my earlier idea to load all action and screen code when we're on the component page rather than the class-autoloading in the current patches. To me, that is the cleanest way of doing this within our current APIs instead of introducing all these new classes and files.

To be honest, my idea in #7666 (load template functions only when the template loop is initialized) will dramatically increase even more memory savings and I would rather spend time working towards a solution for that.

I think the idea to create wrappers for add_action() and add_filter() would introduce even more complexity than it already is for developers new to BuddyPress. Relying on Grunt to do this properly (much like how Composer writes its autoload.php file on generation) would take awhile to write and build. I do like this idea though and would wonder how it would all look like.

I honestly think BuddyPress is at an important juncture where we need to start optimizing before we increase the codebase with even more features and that's what my recent tickets have been exploring.

We have to do something now rather than later. (By "now", I mean in the next few releases and not necessarily v3.0.)

[boonebgorges]

As I mentioned in my reply to Boone in point 2, I prefer my earlier idea to load all action and screen code when we're on the component page rather than the class-autoloading in the current patches. To me, that is the cleanest way of doing this within our current APIs instead of introducing all these new classes and files.

Yes. After the back-and-forth on this ticket, I'm coming around to this. Basically, the trade-off is that late-loading of screen/action functions will introduce a small chance of backward compatibility breakage, for anyone who is manually calling (or hooking) a screen function in a non-standard place. However, in all my years doing weird things with BuddyPress, I have never even been tempted to do such a thing, so I think it's a reasonable risk.

I see that the old patches have been removed from this ticket. So I've reconstructed what I think the late_includes() strategy looked like, and applied it to the Activity component. @r-a-y does this capture it more or less correctly? What do others think about returning to @r-a-y's original, simpler idea? (I feel like I brought on this whole debacle by making this comment, so mea culpa :( ) See attached patch.

[boonebgorges]

Naming conventions etc can maybe be improved, and I haven't audited for code that ought to be put elsewhere, but 7218.diff​ shows the gist of the strategy.

[DJPaul]

I support this too (other than the @is_file bit). I even like it. :)

@ray I sincerely apologise to you for helping having made this ticket get so off-track originally.

Do you want to check the reconstructed patch, and drop it in? I can spend time checking for functions that need to be relocated once the core change is committed, I assume you're pretty fed up with this ticket (and us) by now. :)

[r-a-y]

activity-original.patch is my original patch updated for trunk.

activity-ray-ocd.patch is basically my OCD version of my original idea. bp-activity-actions.php and bp-activity-screens.php have been split up into separate files in the new bp-activity/actions/ and bp-activity/screens/ directories and these files are loaded depending on the current page. This is the patch I'm recommending. Note: I haven't added PHPDoc file headers for each new file yet, but I'll do so once we're ready to commit.

For unit tests, I ran into a bug with our mention unit tests (#7703). The patch in that ticket should be committed before applying this patch and running PHPUnit. Also, PHPUnit does not like conditional file loading when running multiple tests. For an example, see ​https://laracasts.com/discuss/channels/testing/testing-single-method-works-but-multiple-methods-fail. It appears that all files must be included at once before running our unit test suite. Instead of cluttering up the BP_Activity_Component class with this PHPUnit logic, I'm including all action and screen code in our PHPUnit loader file.

---

For the late_includes() method, as I mentioned in the ticket description, I've chosen to run this on the 'bp_setup_canonical_stack' hook at priority 20 because that is the earliest when bp_current_component() and bp_current_action() is fully set up and should be safe to use conditional fucntions like bp_is_current_component(). @boonebgorges chose 'bp_template_redirect' at priority 0, which runs later, but this might be too late for plugins that have chosen to remove hooks on 'bp_init' at the default priority of 10.

Also in Boone's 7218.diff, I'm not a fan of the existing BP_Component::includes() method as it does too many checks for superfluous files (view ​#BB3193 to see what I mean). So I'm not a fan of re-using this logic for the late_includes() method. My late_includes() method just includes files with the require line, no need to be fancy here.

Let me know what you think.

[boonebgorges]

Thanks @r-a-y :-D

The screen/action OCD patch seems fine to me. Any performance benefits to splitting the files up will be extremely minimal, but the main benefit - of better organized routing functions - seems worth the change. If there are no other objections from the peanut gallery, let's go with this pattern.

I've chosen to run this on the 'bp_setup_canonical_stack' hook at priority 20

Got it. I'd chosen 'bp_template_redirect' because that's closer to when the 'bp_actions' and 'bp_screens' hooks are actually fired, but you're right that this makes it very hard to unhook them. The only suggestion I'd make is that piggybacking on bp_setup_canonical_stack feels brittle to me, especially since the canonical-stack logic isn't 100% connected to the determination of the current component. Since bp_setup_canonical_stack is run at bp_init::5, perhaps bp_late_includes() could be hooked at bp_init::7 or something like that. This would be much easier to reason about in bp-core-actions.php, and is still early enough for bp_init::10 callbacks, which I think is our main concern.

I'm not a fan of the existing BP_Component::includes() method as it does too many checks for superfluous files

Yup, totally agreed, I was just throwing something together :-D

[DJPaul]

Yep do it.

[r-a-y]

Since bp_setup_canonical_stack is run at bp_init::5, perhaps bp_late_includes() could be hooked at bp_init::7 or something like that. This would be much easier to reason about in bp-core-actions.php, and is still early enough for bp_init::10 callbacks, which I think is our main concern.

We can't run the late includes hook on 'bp_init' at priority 7 because the 'bp_setup_nav' hook runs on 'bp_init' at priority 6 and does checks for the nav screen functions (see bp_core_register_nav_screen_function() and the is_callable() checks).

So we would need to load before priority 6. The 'bp_setup_canonical_stack' at priority > 10 is the earliest, with the latest being 'bp_setup_nav' at priority 0 to be safe.

Edit - Could do add_action( 'bp_init', 'bp_late_include', 6 ) if it is listed just before the 'bp_setup_nav' call if that is more semantic with our current 'bp_init' action hooks.

Let me know which hook is preferred and I can start committing stuff on the weekend.

[boonebgorges]

We can't run the late includes hook on 'bp_init' at priority 7 because the 'bp_setup_nav' hook runs on 'bp_init' at priority 6 and does checks for the nav screen functions (see bp_core_register_nav_screen_function() and the is_callable() checks).

Oh yeesh. What a mess.

Could do add_action( 'bp_init', 'bp_late_include', 6 ) if it is listed just before the 'bp_setup_nav' call if that is more semantic with our current 'bp_init' action hooks.

Is it deterministically true that it'll always run before bp_setup_nav as long as it's added there first? If there are situations where a race condition would result, it would be a horriffic nightmare to debug. Until someone invents an integer between 6 and 7 (I propose we call it "Smarch") I guess we should go with your original suggestion.

[r-a-y]

Is it deterministically true that it'll always run before bp_setup_nav as long as it's added there first?

I did a quick test, and this appears to be the case, but I guess we want to be on the safe side.

Until someone invents an integer between 6 and 7 (I propose we call it "Smarch")

Did another quick test and technically, you can use a decimal for the priority, but it has to be set as a string.

The following example fires in numerical order:

<?php
add_action( 'get_header', function() {
        echo 'fires after';
}, 10 );

add_action( 'get_header', function() {
        echo 'fires in between';
}, '9.5' );

add_action( 'get_header', function() {
        echo 'fires before';
}, 9 );

I'm guessing we don't want to do this though :)

Will stick with 'bp_setup_canonical_stack' at priority 20, unless we want to use something else.

[r-a-y]

In 11883:

Activity: Move some functions from bp-activity-actions.php to bp-activity-functions.php

Some of the functions from bp-activity-actions.php are not related to
activity page actions, so these are being moved to the all-purpose
bp-activity-functions.php file (for better or worse!).

The intent is to remove and split up the bp-activity-actions.php and
bp-activity-screens.php files until these functions are actually
needed.

See #7218.

[r-a-y]

In 11884:

Core: Introduce hook, 'bp_late_include'.

This hook is meant to be used to load conditional files on certain pages
so we are not always loading code unnecessarily.

This fires on the 'bp_setup_canonical_stack' at priority 20 when we
have ensured that the canonical stack is set up and before we register
navigation items that do checks for screen functions.

We'll be using this hook across all components to offshore code until it
is required.

See #7218.

[r-a-y]

In 11885:

Activity: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Activity component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on an BuddyPress activity page. Now, we only load this
code when needed.

A special case is required for PHPUnit as PHPUnit doesn't like conditional-
included files when running multiple unit tests. So for PHPUnit, we
include all action and screen code at once, just like before.

See #7218.

[r-a-y]

I've committed the action and screen conditional loading for the Activity component.

My goal is to do a couple of the other components this week.

[DJPaul]

Any more of this planned for 3.0, @r-a-y?

I want to ship beta 1 next Wednesday 4th April, and would prefer any more of this sort of refactoring gets in for beta 1, otherwise we wait for the 3.1 release.

[r-a-y]

Thanks for the prompt, @djpaul.

Will definitely spend some time over the weekend to implement the other components.

[r-a-y]

In 11922:

Groups: Move some functions from bp-groups-actions.php to bp-groups-functions.php

Some of the functions from bp-groups-actions.php are not related to group
page actions, so these are being moved to the all-purpose
bp-groups-functions.php file.

The intent is to remove and split up the bp-groups-actions.php and
bp-groups-screens.php files until these functions are actually
needed.

See #7218.

[r-a-y]

In 11923:

Groups: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Groups component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress groups page. Now, we only load this code
when needed.

See #7218.

[r-a-y]

In 11924:

Groups: Conditionally load bp-groups-activity.php

Benefits those that have not enabled the Activity component.

See #7218.

[r-a-y]

In 11925:

Messages: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Messages component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress messages page. Now, we only load this
code when needed.

See #7218.

[r-a-y]

In 11926:

Settings: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Settings component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress settings page. Now, we only load this
code when needed.

See #7218.

[r-a-y]

In 11927:

Members: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Members component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress members page. Now, we only load this
code when needed.

See #7218.

[r-a-y]

In 11928:

XProfile: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
XProfile component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress extended profile page. Now, we only load
this code when needed.

See #7218.

[r-a-y]

In 11929:

XProfile: Load activity, notifications, and settings files if active.

Benefits those that do not have those components active.

See #7218.

[r-a-y]

Typing this on a mobile device as it's getting late here, but the plan is to finish this ticket up for the remaining components (friends, blogs, and notifications) tomorrow.

For the groups component (r11923), I structured the screen functions a little differently since that component has a fair bit more functions than any other component.

I also opened #7738 as I found some action functions that are no longer being used.

Let me know if anyone has any changes to recommend.

[r-a-y]

In 11931:

Friends: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Friends component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress friends page. Now, we only load this
code when needed.

See #7218.

[r-a-y]

In 11932:

Friends: Conditionally load bp-friends-activity.php

Benefits those that have not enabled the Activity component.

See #7218.

[r-a-y]

In 11933:

Notifications: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Notifications component, utilizing the 'bp_late_include' hook introduced
in r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress notifications page. Now, we only load
this code when needed.

This commit also moves over the unused bp_notifications_screen_settings()
function from bp-notifications-screens.php to
bp-notifications-functions.php. This function is a candidate for future
deprecation.

See #7218.

[r-a-y]

In 11934:

Blogs: Conditionally load action and screen functions.

This commit conditionally loads action and screen function code for the
Blogs component, utilizing the 'bp_late_include' hook introduced in
r11884.

Previously, we loaded these functions at all times, which is unnecessary
when a user is not on a BuddyPress blogs page. Now, we only load this
code when needed.

See #7218.

[r-a-y]

In 11935:

Groups: After r11923, fix a group's "Send Invites" page.

Filename was wrong!

Anti-props r-a-y.

See #7218.

[DJPaul]

@r-a-y please would you provide an update? Is there more work to be done here (if so what) and when might that get done?

[r-a-y]

No more updates left. I left this ticket open, just in case I found some bugs after committing.

Will close for now. Thanks for everyone's feedback on this ticket!