Emails: Passing an email address to `bp_send_mail()` does not render `{recipient.name}` token

[r-a-y]

For the bp_send_email() function:

If you pass a user ID:

bp_send_email( 'activity-at-message', 1, $args );

The {{recipient.name}} token will return the name of recipient.

---

However, if you pass an email address of a valid WP user:

bp_send_email( 'activity-at-message', 'test@example.com', $args );

The {{recipient.name}} token will be empty.

Attached patch fixes this.

[DJPaul]

Not actually a bug; this was intended.

I think my idea was that if you passed a user ID to any of the email fields, then you'd know the ID would definitely exist, but if you passed an email, you did not know if it belonged to a user on the site. This is why, where possible, we pass a user ID when we send email.

However, presumably you've found it more convenient to pass an email address in some case, so I have no problems with support being added.

It'd be best to implement this in class BP_Email_Recipient->__construct() -- which will require substantial refactoring, and new tests for the support you're adding -- instead of bp_email_set_default_tokens().

I think you'll be able to use the class BP_Email_Recipient->get_user() method to help. If you're setting the user name and user email from a real user, please also set the user object property.

[DJPaul]

I am also unconvinced that this *needs* to go into 2.5.3, but let's see how much of a change a new patch is.

[r-a-y]

02.patch patches up the BP_Email_Recipient class.

I've fleshed out the unit tests to handle known user email addresses and unknown email addresses.

Question: What is the intention behind the $name parameter in BP_Email_Recipient class constructor? Is it only used when a WP user isn't found? I think it would be beneficial to use this as the main $name even if a WP user is found. View the following unit tests for more info - test_return_with_known_address_and_optional_name() and test_return_with_known_array_and_optional_name().

Let's move this to 2.6.

[DJPaul]

Question: What is the intention behind the $name parameter in BP_Email_Recipient class constructor? ... think it would be beneficial to use this as the main $name even if a WP user is found.

Yes, exactly.

Can't test patch at the moment but looks pretty tasty.

[DJPaul]

I would only suggest we watch how we use empty() here. I've seen it used more and more over the last year in places where it's not strictly necessary. Take this patch:

if ( ! empty( $name ) ) { 
  $this->name = $name; 
}

$name is an optional function argument so it will always be set. It's not an array reference, so we don't need the inherent "is this thing set" that empty() does. This statement is easily simplified to if ( $name ). In my opinion, doing so lowers the code complexity a very tiny bit and increases readability.

[DJPaul]

Other than that empty tweak, this is ready to go in.

[r-a-y]

In 10792:

Emails: Ensure that passing a known WP user email address to bp_send_email() will render the {{recipient.name}} token.

Commit also adds several unit tests for the BP_Email_Recipient class.

Fixes #7044.

[r-a-y]

Other than that empty tweak, this is ready to go in.

I've made your adjustment as noted in comment:6.

FWIW, empty() is slightly faster than doing a plain variable check:
​http://maettig.com/code/php/php-performance-benchmarks.php

Micro-optimization vs. legibility, the eternal battle. Legibility wins this round :)