Skip to:
Content

BuddyPress.org

#7996 closed defect (bug) (fixed)

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

Reported by: boonebgorges Owned by: DJPaul
Milestone: 4.0 Priority: normal
Severity: major Version:
Component: Emails Keywords: has-patch
Cc:

Description

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:

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.
  1. Perform all of the internal logic of bp_email_set_default_tokens() inside a switch_to_blog() statement.
  1. 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.

Attachments (2)

7996.01.patch (1.1 KB) - added by DJPaul 22 months ago.
7996.02.patch (2.3 KB) - added by boonebgorges 21 months ago.

Download all attachments as: .zip

Change History (6)

#1 @DJPaul
22 months ago

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.

Last edited 22 months ago by DJPaul (previous) (diff)

@DJPaul
22 months ago

#2 @DJPaul
22 months ago

  • Keywords has-patch added

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.

#3 @boonebgorges
21 months ago

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.

#4 @boonebgorges
21 months ago

  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.