Skip to:
Content

Opened 21 months ago

Closed 20 months ago

Last modified 20 months ago

#7044 closed enhancement (fixed)

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

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.5.0
Component: Emails Keywords: has-patch commit
Cc:

Description (last modified by 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.

Attachments (2)

7044.01.patch (478 bytes) - added by r-a-y 21 months ago.
7044.02.patch (5.6 KB) - added by r-a-y 21 months ago.

Download all attachments as: .zip

Change History (12)

@r-a-y
21 months ago

#1 @r-a-y
21 months ago

  • Description modified (diff)

#2 @DJPaul
21 months ago

  • Type changed from defect (bug) to enhancement

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.

#3 @DJPaul
21 months ago

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
21 months ago

#4 @r-a-y
21 months ago

  • Milestone changed from 2.5.3 to 2.6

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.

#5 @DJPaul
21 months ago

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.

#6 @DJPaul
21 months ago

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.

#7 @DJPaul
21 months ago

  • Keywords commit added

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

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


21 months ago

#9 @r-a-y
20 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

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.

#10 @r-a-y
20 months ago

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 :)

Note: See TracTickets for help on using tickets.