Setting Reply-To and From causes excessive database overhead in cases where addresses don't match a user

[boonebgorges]

When setting the Reply-To and From headers of an outgoing email, BP_Email uses the BP_Email_Recipient class, which does a WP user lookup:
​https://buddypress.trac.wordpress.org/browser/tags/3.2.0/src/bp-core/classes/class-bp-email.php?marks=698#L688
​https://buddypress.trac.wordpress.org/browser/tags/3.2.0/src/bp-core/classes/class-bp-email.php?marks=770#L759

If the installation uses a From or Reply-To address that does not match a WP user account, the WP user lookup will fail. And because of WP's get_user_by() internals, failed lookups are not cached. ​https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/class-wp-user.php#L214

One or two extra database queries is not a serious problem, but in cases where many emails are sent in a single request - as in BuddyPress Group Email Subscription, or as when the group admin checks "notify members of this change" - the result can be dozens or hundreds of additional database hits. See ​https://redmine.gc.cuny.edu/issues/10720 for a real-world case.

I imagine it's very common, especially for larger installations, to use a From and Reply-To address on outgoing emails that does not correspond to a WP user. There are many use cases: custom Reply-To for reply-by-email or for bounce handling; an inbox that is shared by members of a support team; a no-reply inbox.

A few suggested approaches:

1. Don't use BP_Email_Recipient in set_replyto() and set_from(). Aside from general DRY principles, it looks like the primary purpose for this class in the case of *recipients* is to validate email addresses and to get canonical display names for the "To" field. These motivations don't apply, or apply differently, in the case of email *senders*. Specifically, the $from_address and $from_name are trusted values that go through WP filters wp_mail_from and wp_mail_from_name, so it's not clear that the extra lookup is necessary. Skipping the use of BP_Email_Recipient in this case would mean a non-trivial patch, since BP_Email expects the BP_Email_Recipient interface. We could, for example, introduce a BP_Email_User interface that is implemented by both BP_Email_Recipient and BP_Email_Sender.

2. Provide a short-circuit filter in BP_Email_Recipient that allows plugins or sites to skip the database lookup. For example, something near the beginning of ​https://buddypress.trac.wordpress.org/browser/tags/3.2.0/src/bp-core/classes/class-bp-email-recipient.php#L46; a plugin would then be able to filter, and if a certain email address is passed, then it could send back a set of dummy values, or something like that.

I'm happy to work up a patch, but was hoping first to get some architectural feedback from @djpaul.

[r-a-y]

I've opted for a slightly, different approach. Keep BP_Email_Recipient the way it is, but disable user lookups for the Reply-To and From email addresses.

See 8003.disable-user-lookup.patch.

[DJPaul]

Good spot with this!

it looks like the primary purpose for this class in the case of *recipients* is to validate email addresses and to get canonical display names for the "To" field.

Yep.

I'm happy to work up a patch, but was hoping first to get some architectural feedback from @djpaul.

I think the better solution is to convert BP_Email_Recipient to an interface, and create classes to represent those likely to have a WordPress user account, and those not, like you suggest.

I don't like the idea of adding a filter or optional function parameter to control the behaviour because it goes against the email API design's intentions.

If @boonebgorges you want a fix for 4.0 but don't have time or desire to do the full interface rewrite, I think in set_replyto() etc that if we pass null to the BP_Email_Recipient constructor for $email_or_user, $this->address will be empty, which should avoid the get_user_by('email') lookup in the get_user() method. We can then manually set the class' address and name properties manually.

[boonebgorges]

Thanks for the quick feedback, @DJPaul.

IMO, no rush to sneak a quick fix into 4.0. If I'm the first person to see this issue in the wild, then it won't hurt to let it slide for another release.

I've attached a quick patch that adds the following:

• An interface BP_Email_Address, which describes classes with get_name(), get_address(), and their corresponding setters()
• An abstract base class BP_Email_Person that implements BP_Email_Address and has some default logic (and filters) for these methods
• A new BP_Email_Sender that extends (without modifying) BP_Email_Person. This class is then used for setting up the Reply-To and From fields in BP_Email
• BP_Email_Recipient now extends BP_Email_Person. This took just a few small changes. I've converted the get_address() etc methods to wrappers for the parent methods, in order to maintain backward compatibility with the _recipient_ filters.

All this (especially the interface) might be a little overarchitected given that BP doesn't currently use type-hinting or dependency injection or other OOP techniques that would necessitate an actual interface. But it does give us a framework for introducing that kind of stuff into the BP_Email stack in the future. Alternatively, we could scrap the interface, and just have an abstract base class.

[johnjamesjacoby]

Patch is OK.

I don’t prefer the Person part of BP_Email_Person class name, but don’t have a better suggestion. I’m not sure if User or Member are better or worse. Mostly, it’s that we don’t use Person anywhere else, and nothing really ties an address to a person exactly.

[boonebgorges]

I'm not a big fan of the 'Person' terminology either, but "member" and "user" already have specific meanings in WP/BP, meanings that we are specifically trying to avoid (since From etc may not point to any "member" or "user" in the BP/WP sense). I suppose that, strictly speaking, what From and Reply-To and To and CC and BCC and others all have in common is that they are email addresses, so maybe the nomenclature should reflect that instead.

[boonebgorges]

Circling back to this ticket, I think that Participant may be a more descriptive and less objectionable term than Person. Let's ship it.

[johnjamesjacoby]

Participant is a good idea. bbPress uses it as it’s general Subscriber type role, and it seems fitting here also. 🚢

[boonebgorges]

In 12353:

Emails: Improve performance when setting Reply-To and From headers.

The new BP_Email_Participant framework allows the Reply-To and From email
headers to be set without requiring an empty and potentially expensive WP
user lookup.

Fixes #8003.