[buddypress-trac] [BuddyPress Trac] #7218: Only load component action and screen code when we're on the component's page

[buddypress-trac]

#7218: Only load component action and screen code when we're on the component's
page
-----------------------------+-----------------------
 Reporter:  r-a-y            |       Owner:
     Type:  enhancement      |      Status:  reopened
 Priority:  normal           |   Milestone:  3.0
Component:  Core             |     Version:
 Severity:  normal           |  Resolution:
 Keywords:  has-patch early  |
-----------------------------+-----------------------

Comment (by boonebgorges):

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

 == Benchmarks ==

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

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

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

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

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

 == Questions ==

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

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

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

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

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

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

 @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

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7218#comment:52>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac