bp_email_set_default_tokens() (or some part of email sending) should switch to root blog

[boonebgorges]

In investigating performance problems on a client site, I've traced a couple of apparently disparate issues back to what seems to be a common source. Briefly, if you trigger the sending of an email, via bp_send_email(), from a non-root site on a Multisite network, it can cause a cascade of problems.

The crux of the issue in the cases I've seen seems to be bp_email_set_default_tokens(). Most of the email API is pretty careful to be self-contained, but there are a few places in bp_email_set_default_tokens() where the function calls are context-specific:

• home_url() will always reference the current site
• get_post_meta() ​https://buddypress.trac.wordpress.org/browser/tags/3.2.0/src/bp-core/bp-core-filters.php#L1085 will fetch the post from the current site rather than the root site (note that $email->get_post_object() doesn't have the same problem, since set_post_object() is set inside of switch_to_blog() in bp_send_email()

The latter case is especially problematic, especially for plugins that intervene in the post/postmeta fetching process. The cases that are causing problems for my client, for example, are linked to a plugin that hooks to get_post_metadata() but gets an ID for a post on the root blog, which it can never find, so it ends up running thousands of database queries.

A couple different strategies for fixing this, ranging from the most to the least targeted:

1. Change the home_url() and get_post_meta() calls to method wrappers. Something like: bp_get_root_domain(), and a new $email->get_preheader() or something like that.

2. Perform all of the internal logic of bp_email_set_default_tokens() inside a switch_to_blog() statement.

3. Perform all of the internal logic of bp_send_email() inside a switch_to_blog() statement.

Strategy 1 is architecturally the nicest, but it also assumes that I've correctly identified all of the context-sensitive links in the chain. Strategy 2 and 3 move progressively more of the email-sending routine into a switched-blog context.

I'd value thoughts from @DJPaul, as well as from @r-a-y, who knows about the details of the specific client project.

[DJPaul]

linked to a plugin that hooks to get_post_metadata() but gets an ID for a post on the root blog, which it can never find, so it ends up running thousands of database queries.

This sounds like a bug in that plugin, if it's making thousands of database queries from one call to bp_send_email. :)

Strategy 1 is architecturally the nicest, but it also assumes that I've correctly identified all of the context-sensitive links in the chain.

I believe you have. I've looked through from bp_send_email onwards and I can't see anything else. A lot of other options the email code uses are site options, which we already handle with bp_get_option.

Swapping home_url with bp_get_root_domain is a good idea.

I'm less convinced about adding a BP_Email class method for the email.preheader part, as this code only sets an (optional) templating token, and I don't believe that fits into the purpose of what the BP_Email class represents. Given it's single-use, I don't immediately see a good reason why it should be a global function either. I think we would be fine to just wrap that filter call in switch_to_blog_ etc.

[DJPaul]

I've just that put into the attached patch (untested) but now I look at it, the more I think adding a preheater helper function makes sense. I'm not sure. Happy to go with what you think.

[boonebgorges]

Thanks for the thoughts, @DJPaul. I'm fine with the more targeted approach.

7996.02.patch​ creates a get_preheader() method on BP_Email. It doesn't totally follow the ->get( 'preheader' ) pattern in the rest of the class, but I wasn't sure whether it *should*, given that the 'preheader' property is something we fetch internally rather than being set by the consumer.

[boonebgorges]

In 12284:

Emails: Ensure that default email tokens are pulled from root blog.

The email post type is always stored on the root blog, so we should be
sure to switch to that blog whenever referencing the post (such as
when pulling postmeta related to the post).

Fixes #7996.