Skip to:
Content

Opened 19 months ago

Last modified 36 hours ago

#7218 reopened enhancement

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 early
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 (3)

7218.activity-v3.patch (81.4 KB) - added by r-a-y 5 weeks ago.
7218.groups-v3.patch (119.1 KB) - added by r-a-y 5 weeks ago.
7218.diff (5.2 KB) - added by boonebgorges 9 days ago.

Download all attachments as: .zip

Change History (58)

#1 @DJPaul
19 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
19 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.


19 months ago

#6 @mercime
18 months ago

  • Status changed from new to reopened

#7 @DJPaul
18 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
18 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 in bp-activity-actions.php to bp-activity-akismet.php. I'll experiment and report back.

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

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


18 months ago

#10 @r-a-y
18 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
18 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 18 months ago by r-a-y (previous) (diff)

#12 @DJPaul
17 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.


16 months ago

#14 @lkraav
16 months ago

  • Cc leho@… added

#15 @r-a-y
15 months ago

I'm still keen on this.

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

#16 @boonebgorges
15 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
13 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
11 months ago

  • Keywords dev-feedback added

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


11 months ago

#20 @r-a-y
10 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.


10 months ago

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


10 months ago

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


10 months ago

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


9 months ago

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


9 months ago

#26 @r-a-y
9 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 9 months ago by r-a-y (previous) (diff)

#27 @boonebgorges
9 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
9 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 9 months ago by r-a-y (previous) (diff)

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


9 months ago

#30 @boonebgorges
9 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
9 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 8 months ago by r-a-y (previous) (diff)

#32 @r-a-y
8 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
3 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
3 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
3 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
3 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
3 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
3 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
3 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
3 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
3 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
2 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
8 weeks 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.


6 weeks ago

#45 @r-a-y
5 weeks 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 weeks ago

#47 @DJPaul
5 weeks 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 weeks ago

  • Keywords commit added

#49 @DJPaul
4 weeks ago

  • Keywords commit removed

#50 @DJPaul
4 weeks 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
4 weeks 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
3 weeks 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 3 weeks ago by boonebgorges (previous) (diff)

#53 @johnjamesjacoby
3 weeks 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
3 weeks 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 3 weeks ago by r-a-y (previous) (diff)

#55 @boonebgorges
9 days 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
9 days ago

#56 @boonebgorges
9 days 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
36 hours 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. :)

Note: See TracTickets for help on using tickets.