Skip to:
Content

Opened 23 months ago

Closed 3 months ago

#7218 closed enhancement (fixed)

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

Reported by: r-a-y Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc: leho@…

Description

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?

Attachments (5)

7218.activity-v3.patch (81.4 KB) - added by r-a-y 5 months ago.
7218.groups-v3.patch (119.1 KB) - added by r-a-y 5 months ago.
7218.diff (5.2 KB) - added by boonebgorges 4 months ago.
7218.activity-original.patch (43.3 KB) - added by r-a-y 4 months ago.
7218.activity-ray-ocd.patch (86.2 KB) - added by r-a-y 4 months ago.

Download all attachments as: .zip

Change History (88)

#1 @DJPaul
23 months ago

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.

#2 @DJPaul
23 months ago

  • Milestone changed from Awaiting Review to Under Consideration

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


23 months ago

#6 @mercime
22 months ago

  • Status changed from new to reopened

#7 @DJPaul
22 months ago

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?

#8 @r-a-y
22 months ago

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 to bp-activity-akismet.php? I'll experiment and report back.

Version 1, edited 22 months ago by r-a-y (previous) (next) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


22 months ago

#10 @r-a-y
21 months ago

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.

#11 @r-a-y
21 months ago

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.

Last edited 21 months ago by r-a-y (previous) (diff)

#12 @DJPaul
21 months ago

  • Milestone changed from Under Consideration to 2.8

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

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


20 months ago

#14 @lkraav
20 months ago

  • Cc leho@… added

#15 @r-a-y
19 months ago

I'm still keen on this.

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

#16 @boonebgorges
18 months ago

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

#17 @DJPaul
17 months ago

  • Milestone changed from 2.8 to 2.9

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

#18 @hnla
15 months ago

  • Keywords dev-feedback added

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


15 months ago

#20 @r-a-y
14 months ago

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.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


14 months ago

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


14 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


14 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


13 months ago

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


13 months ago

#26 @r-a-y
13 months ago

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

Last edited 13 months ago by r-a-y (previous) (diff)

#27 @boonebgorges
13 months ago

  • Keywords dev-feedback removed

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

#28 @r-a-y
12 months ago

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.

Last edited 12 months ago by r-a-y (previous) (diff)

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


12 months ago

#30 @boonebgorges
12 months ago

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.

#31 @r-a-y
12 months ago

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.

Last edited 12 months ago by r-a-y (previous) (diff)

#32 @r-a-y
12 months ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

Bumping to 3.0 and marking as early.

#33 @r-a-y
7 months ago

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.

#34 @boonebgorges
7 months ago

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.

#35 @DJPaul
7 months ago

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!

#36 @DJPaul
6 months ago

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

#37 @r-a-y
6 months ago

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

#38 @DJPaul
6 months ago

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' );
}}

#39 @r-a-y
6 months ago

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.

#40 @boonebgorges
6 months ago

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.

#41 @DJPaul
6 months ago

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

#42 @DJPaul
6 months ago

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.

#43 @DJPaul
6 months ago

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.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


5 months ago

#45 @r-a-y
5 months ago

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?

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


5 months ago

#47 @DJPaul
5 months ago

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.

#48 @DJPaul
5 months ago

  • Keywords commit added

#49 @DJPaul
5 months ago

  • Keywords commit removed

#50 @DJPaul
5 months ago

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

#51 @r-a-y
5 months ago

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.

#52 @boonebgorges
5 months ago

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?
  1. 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?
  1. 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.
  1. Not a question, but good catch on bp_groups_update_orphaned_groups_on_group_delete() :)
  1. 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

Last edited 5 months ago by boonebgorges (previous) (diff)

#53 @johnjamesjacoby
5 months ago

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?

#54 @r-a-y
5 months ago

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

Last edited 5 months ago by r-a-y (previous) (diff)

#55 @boonebgorges
4 months ago

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
4 months ago

#56 @boonebgorges
4 months ago

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.

#57 @DJPaul
4 months ago

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

#58 @r-a-y
4 months ago

  • Keywords early removed

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.

#59 @boonebgorges
4 months ago

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

#60 @DJPaul
4 months ago

Yep do it.

#61 @r-a-y
4 months ago

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.

Last edited 4 months ago by r-a-y (previous) (diff)

#62 @boonebgorges
4 months ago

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.

#63 @r-a-y
4 months ago

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.

Last edited 4 months ago by r-a-y (previous) (diff)

#64 @r-a-y
4 months ago

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.

#65 @r-a-y
4 months ago

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.

#66 @r-a-y
4 months ago

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.

#67 @r-a-y
4 months ago

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.

#68 @DJPaul
3 months ago

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.

#69 @r-a-y
3 months ago

Thanks for the prompt, @djpaul.

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

#70 @r-a-y
3 months ago

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.

#71 @r-a-y
3 months ago

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.

#72 @r-a-y
3 months ago

In 11924:

Groups: Conditionally load bp-groups-activity.php

Benefits those that have not enabled the Activity component.

See #7218.

#73 @r-a-y
3 months ago

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.

#74 @r-a-y
3 months ago

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.

#75 @r-a-y
3 months ago

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.

#76 @r-a-y
3 months ago

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.

#77 @r-a-y
3 months ago

In 11929:

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

Benefits those that do not have those components active.

See #7218.

#78 @r-a-y
3 months ago

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.

#79 @r-a-y
3 months ago

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.

#80 @r-a-y
3 months ago

In 11932:

Friends: Conditionally load bp-friends-activity.php

Benefits those that have not enabled the Activity component.

See #7218.

#81 @r-a-y
3 months ago

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.

#82 @r-a-y
3 months ago

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.

#83 @r-a-y
3 months ago

In 11935:

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

Filename was wrong!

Anti-props r-a-y.

See #7218.

#84 @DJPaul
3 months ago

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

#85 @r-a-y
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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!

Note: See TracTickets for help on using tickets.