Skip to:
Content

Opened 21 months ago

Closed 18 months ago

Last modified 18 months ago

#7286 closed enhancement (fixed)

BP Email: Allow end user to specify which PHPMailer should be used.

Reported by: dcavins Owned by: djpaul
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.5.0
Component: Emails Keywords: has-patch
Cc: dcavins

Description

In the BP_PHPMailer class, it's currently not possible to specify which PHPMailer is used. It would be handy in some instances, like working with HTTP API mailers, to be able to use a custom PHPMailer class.

Attachments (1)

7286.1.patch (916 bytes) - added by dcavins 21 months ago.
Add a filter on $phpmailer before loading the standard version in BP_PHPMailer

Download all attachments as: .zip

Change History (14)

@dcavins
21 months ago

Add a filter on $phpmailer before loading the standard version in BP_PHPMailer

#1 @dcavins
21 months ago

  • Summary changed from BP Email: to BP Email: Allow end user to specify which PHPMailer should be used.

#2 @dcavins
21 months ago

I've added a patch that just adds a filter on $phpmailer at the beginning of BP_PHPMailer::bp_email(). When talking with @djpaul about my request, he had a different idea:

The better way would be to accept a phpmailer instance somewhere up the chain, that gets passed to the Bp email object.

Which could be the addition of a new var in BP_Email and a helper method BP_Email::get_phpmailer() to create the PHPMailer instance and fetch it. I can work up a patch that goes that route if it would be useful for more cases than the one I'm solving.

#3 @DJPaul
21 months ago

We can and will make this better for the next BP. I am learning up on some more modern PHP suggestions for architecting classes, and I think there's one or two things we could use/experiment with inside the BP_Email class. It'll probably be a month into 2.8 before I post up a suggestion, but even if I get abudcted by aliens for the entire cycle, you've already supplied a good-enough patch -- so that's our fallback.

#4 follow-up: @DJPaul
21 months ago

To loop back. I've recently read "Modernizing Legacy Applications in PHP" by Paul Jones (on Lulu.com) which introduced me to the concept of Dependency Injection and how to migrate legacy code to modern practices and techniques. I want to use BP_PHPMailer as a guinea pig for trying out some of this. The change we should make is actually very simple:

Add constructor to BP_PHPMailer, and pass in PHPMailer object. Store it as a new property on the class. Remove the existing static variable.

  • Key thing is to create the PHPMailer object outside the class and push it in ("dependency Injection").

We'd add the filter for the class name of the PHPMailer object outside the class, inside the bp_send_email function, just before we create the BP_PHPMailer class. We can reuse 99% of @dcavins' first patch for this.

#5 in reply to: ↑ 4 @dcavins
21 months ago

Replying to DJPaul:
That's funny, I was just migrating a site to a new server yesterday and needing to remember how BP Mail works. Your suggestion sounds good to me, and it increases flexibility in a way, because different plugins could pass in whatever PHPMailer they need to use when calling the delivery class--if they were calling the class directly.

Looking into the plugins taking advantage of the BP Email system that I know of--"BP Emails for bbPress" and also @r-a-y's updates to "BP Group Email Subscription"--it looks like they both use bp_send_email() but don't work with the classes directly, so they shouldn't be affected by changing the signature of the BP_PHPMailer class. Are there plugins you know of that are doing something fussier?

#6 @DJPaul
21 months ago

No, I don't know anyone using it for anything interesting. We might make the PHPMailer constructor argument optional, though i'm not feeling it because it's anti-DI, but we might need to do that. We'll see once someone's put together a patch and thought about it more, anyway.

#7 @DJPaul
20 months ago

I'll try to get to this at the weekend/next week.

#8 @r-a-y
20 months ago

Are there plugins you know of that are doing something fussier?

I have one plugin that specifically looks for BP_PHPMailer:
https://github.com/r-a-y/bp-email-type/blob/master/bp-email-type.php#L87

However, I do not think the proposals outlined in this patch will affect that plugin though.

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


19 months ago

#10 @DJPaul
18 months ago

I'm still not sure about changing the constructor; but I think putting in this patch does not remove this option should I feel a bit stronger about it in the future. This filter would continue to be supported after a change to allow us to use DI in this class, so let's just do it.

#11 @DJPaul
18 months ago

The patch referred to changing the class but the implementation here actually changes the object, so i've updated the comments appropriate. Let me know if this doesn't fit with your use-case, @dcavins.

#12 @djpaul
18 months ago

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

In 11387:

Emails: add filter to change PHPMailer object used to deliver emails.

Fixes #7286

Props dcavins

#13 @dcavins
18 months ago

That'll work for my "use an API mailer class" case perfectly. Thanks.

Note: See TracTickets for help on using tickets.