#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)
Change History (88)
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
8 years ago
#7
@
8 years 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
@
8 years 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 It's already in another file. Maybe we can move the Akismet-related code in bp_is_active( 'activity', 'akismet' )
.bp-activity-actions.php
to bp-activity-akismet.php
. I'll experiment and report back.
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
8 years ago
#11
@
8 years 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.
#12
@
8 years 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.
8 years ago
#16
@
8 years 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
@
8 years 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.
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
8 years ago
#20
@
8 years 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.
8 years ago
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
8 years ago
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
8 years ago
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
8 years ago
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
8 years ago
#26
@
8 years ago
@boone or @jjj - Can one 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.
#27
@
8 years 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
@
7 years 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.
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
7 years ago
#30
@
7 years 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
@
7 years 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.
#32
@
7 years ago
- Keywords early added
- Milestone changed from 2.9 to 3.0
Bumping to 3.0 and marking as early.
#33
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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.
7 years ago
#45
@
7 years 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.
7 years ago
#47
@
7 years 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.
#50
@
7 years 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
@
7 years 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
@
7 years 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.
- The checks that remain in the
-screens.php
and-actions.php
functions are mostly of the formbp_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 checkingis_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?
- 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 filebp-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 thelate_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 thefeed
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?
- Similar to 1 - 7218.groups-v3.patch has some permissions checks
bp_user_can_create_groups()
, some nonce checks, and even does somebp_core_redirect()
logic. For consistency, I'd be in favor of moving this logic into the new classes.
- Not a question, but good catch on
bp_groups_update_orphaned_groups_on_group_delete()
:)
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 aScreen
class. Same withbp_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
#53
@
7 years 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
andbp_screens
hooks are already abstractions of thebp_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
@
7 years ago
Thanks for the feedback everyone.
Replies to @boonebgorges follow:
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 withis_user_logged_in()
, we can add additionalis_user_logged_in()
checks to the class files.
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 thelate_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 thelate_includes()
strategy, but if that isn't desired, I can roll back these changes as well.
- 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.
- 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.)
#55
@
7 years 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.
#57
@
7 years 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
@
7 years 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
@
7 years 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
#61
@
7 years 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.
#62
@
7 years 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
@
7 years 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.
#67
@
7 years 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
@
7 years 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
@
7 years ago
Thanks for the prompt, @djpaul.
Will definitely spend some time over the weekend to implement the other components.
#78
@
7 years 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.
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.