Skip to:
Content

Opened 18 months ago

Last modified 34 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 (8)

7218.members.patch (23.9 KB) - added by r-a-y 9 months ago.
For the Members component - refreshed for 2.9
7218.akismet.patch (4.9 KB) - added by r-a-y 9 months ago.
Refreshed for 2.9
7218.02.patch (40.5 KB) - added by r-a-y 9 months ago.
For the Activity component - refreshed for 2.9. Also conditionally loads activity RSS feeds and notification settings.
7218.friends.patch (10.4 KB) - added by r-a-y 9 months ago.
For the Friends component - refreshed for 2.9. Also conditionally loads the activity code only if the Activity component is active.
7218.groups.patch (47.8 KB) - added by r-a-y 9 months ago.
For the Groups component - refreshed for 2.9. Also conditonally loads notification settings and the activity and legacy forums code if those components are active.
7218.groups-autoload.patch (145.6 KB) - added by r-a-y 7 months ago.
Groups screen class autoloader
7218.late-includes.patch (941 bytes) - added by r-a-y 7 months ago.
7218.activity-autoload.patch (82.1 KB) - added by r-a-y 7 months ago.

Download all attachments as: .zip

Change History (50)

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


18 months ago

#6 @mercime
17 months ago

  • Status changed from new to reopened

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

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


17 months ago

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

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


15 months ago

#14 @lkraav
15 months ago

  • Cc leho@… added

#15 @r-a-y
14 months ago

I'm still keen on this.

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

#16 @boonebgorges
13 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
12 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
10 months ago

  • Keywords dev-feedback added

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


10 months ago

@r-a-y
9 months ago

For the Members component - refreshed for 2.9

@r-a-y
9 months ago

Refreshed for 2.9

@r-a-y
9 months ago

For the Activity component - refreshed for 2.9. Also conditionally loads activity RSS feeds and notification settings.

@r-a-y
9 months ago

For the Friends component - refreshed for 2.9. Also conditionally loads the activity code only if the Activity component is active.

@r-a-y
9 months ago

For the Groups component - refreshed for 2.9. Also conditonally loads notification settings and the activity and legacy forums code if those components are active.

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


9 months ago

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


9 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 r-a-y. View the logs.


8 months ago

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


8 months ago

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

#27 @boonebgorges
8 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

@r-a-y
7 months ago

Groups screen class autoloader

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

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


7 months ago

#30 @boonebgorges
7 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
7 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.

Version 0, edited 7 months ago by r-a-y (next)

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


34 hours ago

Note: See TracTickets for help on using tickets.