Hi.

Finally got some more work done on this part of my website.

After a long period of debugging, I've just discovered why I was getting new nodes, but not the outgoing notification emails: og_mailing list's _og_mailinglist_send_raw_email function uses the system function (which is a disabled function in my installation) and has a hard coded path for the binary.

How straight forward would it be to use PHPMailer - or Drupal's own notifications system?

Regards,

Nigel

CommentFileSizeAuthor
#32 og_mailinglist_replace_sendmail.patch21.27 KBnigelcunningham
#30 phpmailer.patch1.33 KBnigelcunningham
#30 sendmail.patch17.18 KBnigelcunningham
#24 phpmailer.patch1.97 KBnigelcunningham
#24 phpmailer-usage.patch11.29 KBnigelcunningham
#12 0001-Use-PHPMailer-to-send-raw-emails.patch5 KBnigelcunningham

Comments

tobias’s picture

tobias commented

Hi Nigel - this looks useful. Thanks for sharing. Can you share some of your troubleshooting tactics here? I am also eager to troubleshoot and understand where things might be going wrong on my site.

Corollary to what you are describing (I think) - I also would like to understand what might be involved to send out richtext posts to OGM subscribers that include formatting.

Thanks!

Tobias

nigelcunningham’s picture

nigelcunningham commented

In this case, I used a couple of well placed watchdog statements to show me what was going on. Another possibility would be using the xdebug extension.

I'd love to use the existing notifications/messaging framework - it lets you send in different formats and using SMS and so on as well. I'm sure Kyle has good reasons for not using it, but I'm not sure at the moment what they are.

tobias’s picture

tobias commented

Hi Nigel -

I wonder if you and Kyle could collaborate to get your watchdog statements into the code with an option to turn on/off debugging. Would make it easier for all of us trying to test the code.

I personally am happy to say goodbye to the existing notifications/messaging framework - or at least the notifications/subscriptions interface which hasn't served my community particularly well. OGM's group membership and mail delivery interface is much simpler, esp with the per-group block feature we've been discussing.

But it's true that this doesn't mean we have to say goodbye to notifications altogether - there are some very handy things in there like you say. I'd like to be able to offer subscription by SMS at some stage - but this is not an immediate priority.

Cheers,

Tobias

kyle_mathews’s picture

kyle_mathews commented
Issue tags: +ogm-1.0

I hadn't ever looked really at how PHPMailer sends email until this morning but it looks as if OGM could and should definitely switch to using PHPMailer. It has a lot more robust way of sending email then ogm's hardcoded sendmail function.

After spending about an hour trying to convert things over, I've given up for now as it's proving harder than anticipated. If you'd like to hack on it, look at this branch: http://github.com/KyleAMathews/og_mailinglist/tree/send_all_emails_phpma... Also, I created a test script for testing changes at testing/testing_mailer_integration.php. Run that with drush scr testing_mailer_integration.php

Cool beans. I'll set this as a 1.0 blocker. I'm going to push out the beta without it but this is definitely something we'll want to figure out before 1.0 as it'll make sending messages a lot more flexible. Heck, with complete PHPMailer integration, ogm would even work on Windows!

On integrating w/ notifications/messaging. That'd be nice someday perhaps. See http://groups.drupal.org/node/42688#comment-115832 But a lot of work would have to be done on those modules + ogm and I'm not volunteering at the moment. If you're interested in discussing that further, please create a new issue.

nigelcunningham’s picture

nigelcunningham commented

Thanks Kyle. It's going to be Saturday (AEST) or later before I can look at this again, but I definitely will.

kyle_mathews’s picture

kyle_mathews commented

A bit more information about the difficulties I ran into. The main problem is the PHPMailer function for sending mail through sendmail, smtp, and the php mail() function are all protected functions. The PHPMailer Send() function is what you normally use with PHPMailer insists on building the headers + body of the email itself whereas I want to keep that within ogm as an important design principle for ogm is it doesn't affect the email in the slightest other than modifying the headers some + adding the footer. I'm not sure that we could use PHPMailer solely to send email w/o starting to change how the emails look/feel in subtle and not-so-subtle ways.

So two potential ways around this. One, I'm not 100% sure this will work as I don't code OO stuff much but I think we could extend the PHPMailer to create our own Send function which we can then use to call the other send functions. The other way I've thought of is to just port those sending functions right into ogm. This would of course be a decent amount of work + mean a higher maintenance cost. Neither attractive things.

nigelcunningham’s picture

nigelcunningham commented
Issue tags: -ogm-1.0

I'm no php expert, so I might be about to talk nonsense. If so, excuse me.

I've been looking up PHP related packages and functions and am wondering if using the Pear Mail package might be an option.

Regards,

Nigel

nigelcunningham’s picture

nigelcunningham commented
Issue tags: +ogm-1.0

Oooh. I didn't change anything how did I manage to remove the tag?

nigelcunningham’s picture

nigelcunningham commented

Working on this at the moment (same changes mentioned in the message a minute ago).

I've reworked the function to use PHPMailer, and am now making an override for PHPMailer so we can preserve the date and only send the email on to mailing list recipients (I haven't tested, but suspect that people cc'd will get a copy from the poster and from the mailing list software at the moment).

kyle_mathews’s picture

kyle_mathews commented

On people getting dup emails -- that won't happen as actually, all email clients afaik filter out duplicate emails based on message-ids. Back in the bad ol' days before #799418: Don't rewrite message-ids for incoming emails was fixed, I had some complicated logic to remove cc'd people from the email going out from Drupal. Now, everyone just gets sent the email and those clients who'd already received it just filter out the dupe.

nigelcunningham’s picture

nigelcunningham commented

Great :)

nigelcunningham’s picture

nigelcunningham commented
Status: Active » Needs review
StatusFileSize
new 0001-Use-PHPMailer-to-send-raw-emails.patch5 KB

Attaching patch for review.

kyle_mathews’s picture

kyle_mathews commented

Very nice! I didn't know that you could just put everything into the PHPMailer body and make it work.

Ok, I tested and played around with this quite awhile today and got it working.

Three things I had to do to a) get it working and b) make it so Gmail would filter out duplicates.

1) Sending via SMTP wasn't working so I commented that out. I think this might be provide a good case for depending on the PHPMailer module as they've already worked out how to let people use PHPMailer and configure through the web interface who their SMTP provider is etc.

2) The message-id needed to be set in the mailer object. Before I changed that, there was two message-ids being sent.

3) And as it turns out, in GMail at least, you need both the message-id and date header to be identical for email duplicates to be ignored. So as you pointed out in your commit, PHPMailer doesn't let you set the Date field right now. I got around that by commenting out line 1081 in PHPMailer where they set the Date and then adding it back as a custom header. Telling everyone to patch the class.phpmailer.php file is a non-starter. So I think before this can go in, we need to get a patch into PHPMailer which let's us override the Date similar to how they handle the message-id where they only provide a message-id if one isn't already set.

My commit is at http://github.com/KyleAMathews/og_mailinglist/commit/9a200b1c9225df62411.... You can add my branch as a remote branch and fetch my commit from it.

kyle_mathews’s picture

kyle_mathews commented
Title: Depends on system function and hardcodes sendmail path » Send all emails through PHPMailer removing dependency on having a Sendmail compatible server

Oh, and the PHPMailer issue queue is here: http://sourceforge.net/tracker/?group_id=26031&atid=385707

nigelcunningham’s picture

nigelcunningham commented

Thanks for the review, Kyle.

Sorry about the SMTP part - I had to use SMTP and was using a username and password to send too, so was thinking we should add some system wide configuration options for that. Does the PHPMailer module already provide such an interface for doing authenticated SMTP as well? If so, that's another reason to start using it.

Re the message id, I wonder if that's somehow transport related - I remember checking the message ids were the same. I'll look again.

Re the date, do you want to go to the PHPMailer guys, or do you want me to?

kyle_mathews’s picture

kyle_mathews commented
Status: Needs review » Needs work

Yeah -- I need to look more closely at the PHPMailer module to see what they offer as far as configuration. If they've already got that pretty well setup, it would be nice to suggest as an alternative if you need advanced SMTP setup. Most people would use the normal Sendmail / PHPMailer setup for sending email. But if they needed advanced options they could install the PHPMailer module. OGM's phpmailer_load function could check first if the phpmailer module is enabled and if it is, use their load function instead.

On message-id -- look at the email headers that got sent out for you. There'll probably be 2 message-ids being added. That's what happened to me. PHPMailer doesn't check if a custom header we add has already been added or not.

If you could, that'd be great. Once you have something working, I'll test it and add a comment on the PHPMailer issue saying I've tested this and it works and is highly needed.

nigelcunningham’s picture

nigelcunningham commented

Looking at the code again, I think we can override the MessageID. I'll give it a try.

I've opened an issue for PHPMailer's Date field at https://sourceforge.net/tracker/index.php?func=detail&aid=3080711&group_....

nigelcunningham’s picture

nigelcunningham commented

Now in my branch:

diff --git a/og_mailinglist_transport.inc b/og_mailinglist_transport.inc
index ab5ab31..c2de74b 100755
--- a/og_mailinglist_transport.inc
+++ b/og_mailinglist_transport.inc
@@ -805,8 +805,7 @@ function _og_mailinglist_add_footer($email, $footer) {
 
 function _og_mailinglist_send_raw_email($headers, $body, $node) {
   $mailer = og_mailinglist_create_mailer();
-  $mailer->IsSMTP();
-  $mailer->Host= "127.0.0.1";
+  //$mailer->SMTPDebug  = 2;
   $mailer->ClearAddresses();
 
   foreach($headers as $key => $value) {
@@ -814,6 +813,9 @@ function _og_mailinglist_send_raw_email($headers, $body, $node) {
       case 'date':
        $mailer->Date = $value;
         break;
+      case 'Message-ID':
+        $mailer->MessageID = $value;
+        break;
       case 'subject':
        $mailer->Subject = $value;
         break;
@@ -836,7 +838,7 @@ function _og_mailinglist_send_raw_email($headers, $body, $node) {
          }
         break;
       case 'from':
-        if (preg_match("/(^<)*<(.*?)>/", $value, $matches)) { 
+        if (preg_match("/([^<]*)<(.*?)>/", $value, $matches)) { 
           $mailer->From = $matches[2];
           $mailer->FromName = $matches[1];
         } else

(Date is still an outstanding issue; also still to look into use of the PHPMailer module as a potential solution for configuring sending methods).

nigelcunningham’s picture

nigelcunningham commented

Ooooooooh. Just started looking at the PHPMailer module, and I learned some more PHP :)

It shows how one can override the date field :)

I'll have a try at modifying the above to use PHPMailer and override the date with that of our incoming message.

kyle_mathews’s picture

kyle_mathews commented

Cool, looked at the issue report and it looks good. Things will move faster if we create the patch for the PHPMailer folks. It should be pretty simple, almost identical to how they handle MessageID. Create a private variable and do the same check they do for MessageID, if the date is set, use that one, if not, create one. If you have time to write the patch in the next few days, that'd be great. If not, I'll get to it probably later this week. You can check out the latest code from SVN here: http://sourceforge.net/projects/phpmailer/develop

nigelcunningham’s picture

nigelcunningham commented

I have a work around for the above issue without needing upstream to fix things: we can override the PHPMailer class and specifically the date function it provides.

It looks like we need to do this anyway, as there is also the issue that we should be retaining the original list of recipients but only delivering email to the mailing list recipients. I have work in progress on overriding the class.

kyle_mathews’s picture

kyle_mathews commented

we should be retaining the original list of recipients but only delivering email to the mailing list recipients

I'm not really sure what you mean here? Could you clarify please?

My only concern with overriding PHPMailer is a) increase maintaince time and b) will your method work if/when we depend on the PHPMailer module for the advanced SMTP setups? E.G. can they load the class and we override afterwards still?

nigelcunningham’s picture

nigelcunningham commented

Morning Kyle.

1) Retaining the original list of recipients but only delivering to the mailing list recipients

Say a person emails ncunningham@crca and list@crca, and list@crca has subscribers a@x.com.au, b@y.com.au and c@z.net. The outgoing email should still have ncunningham@crca and list@crca in the to/cc as per the original email, but it should be delivered to a, b and c's email addresses. The base PHPMailer class picks who to send the email to (assuming I've read it correctly) based on all the to and cc and bcc recipients that are listed. Even if email clients will do the right thing and discard the extra copies, we should be seeking to avoid wasting time and bandwidth sending email that has already been delivered via the proper (direct) delivery method.

2) Overriding PHPMailer

I'm writing the support so that it will use DrupalPHPMailer if the Drupal PHPMailer module is there, or PHPMailer itself if the Drupal module is missing. Frankly, I'd prefer just to rely on having the drupal module as that will simplify things, but I'm looking at how I can support both configurations and make things as simple and clean as possible.

nigelcunningham’s picture

nigelcunningham commented
StatusFileSize
new phpmailer-usage.patch11.29 KB
new phpmailer.patch1.97 KB

I've had some success in the quest to get PHPMailer set up as a replacement for the direct invocation of sendmail.

In the process, I've discovered some issues with PHPMailer itself - some class and variable declarations that are private rather than protected, making it impossible for us to easily do the overrides we need. The fixes are in the first attached phpmailer patch, which I'll submit to the PHPMailer developers after hearing your feedback.

phpmailer-usage.patch has the changes to og_mailinglist that I've done so far.

I don't yet have the Drupal PHPMailer module integration done, but imagine that it will probably be not much more than the new file in the second patch, modified to extend the Drupal module's class instead (plus logic to use on class if the Drupal module exists or the PHPMailer module otherwise). I would be nice if PHP allowed you to choose which class you're going to override at runtime, but as I understand it, it doesn't at the moment.

Hope that all makes sense!

kyle_mathews’s picture

kyle_mathews commented

So finally looked through the code. It looks good so far. I didn't understand however why you needed to override the SMTP and Sendmail send functions?

On the PHPMailer patch... I looked over at the issue and it's a ghost town. Then I tried to register w/ Sourceforge to comment and kept getting an error... Stupid Soureforge.

Will this and the shared host effort not work w/o the PHPMailer patch?

kyle_mathews’s picture

kyle_mathews commented

Finally got registered on PHPMailer! #smallvictories

Anyways, when I got there, I noticed you hadn't posted your patch yet. Could you add that and then I'll add a comment saying, "hey! this is great and should be committed!".

nigelcunningham’s picture

nigelcunningham commented

Yeah... I wanted to get your input and make sure it was complete and sufficient and necessary before posting it there.

letapjar’s picture

letapjar commented

my understanding was that this patch is required to make certain variables protected rather than private so they can be over-ridden.

the shared hosting patch depends upon (incorporates really) Nigel's patch from this issue/thread os I don't think it would work w/o these changes.

nigelcunningham’s picture

nigelcunningham commented

I've made some time to do more work on this.

The reason others haven't seen duplicate emails coming via og_m is that og_m explicitly sets the 'To' field to itself only. I believe this is wrong. If I send an email to a and b (where b is an ogm list and 'a' is an individual recipient), email coming from og_m should still have the 'To' line containing both 'a' and 'b', but be sent only to the members of 'b'. Fixing the 'To:' results in 'a' receiving a second copy, so my DeliverTo field is necessary.

Will do more work, and hopefully have a new patch soon.

nigelcunningham’s picture

nigelcunningham commented
Status: Needs work » Needs review
StatusFileSize
new sendmail.patch17.18 KB
new phpmailer.patch1.33 KB

Okay. Here's an updated patch to the PHPMailer library, together with the updated og_m patch (this is a simple diff between your tree and mine, Kyle).

This version will automatically use the Drupal module's class if the Drupal phpmailer module is enabled, or go directly to the library otherwise.

It retains the To & CC values from the original email, but only sends the mail to those who are list members, except in the case of the Mailsend method, where we are unable to override settings. (It should be avoided).

I would consider this issue fixed if this patch is to your liking.

letapjar’s picture

letapjar commented

so does this no longer require overriding/patching of phpmailer itself for the private/protected variables issue?

-edit: nevermind I just looked at the patch - still requires that. no problem

nigelcunningham’s picture

nigelcunningham commented
StatusFileSize
new og_mailinglist_replace_sendmail.patch21.27 KB

I believe this is all finished and ready to be merged. Would someone else please give it a go? (This patch, includes the patch to the PHPmailer library).

nigelcunningham’s picture

nigelcunningham commented

Noone interested in this functionality?

kyle_mathews’s picture

kyle_mathews commented

I released tonight Beta4 with the Postfix integration and some other recent fixes.

Tomorrow I'm going to take a crack at this issue. Assuming the testing all goes well, I'll make another release tomorrow or the next day.

nigelcunningham’s picture

nigelcunningham commented

Cool. Thanks!

aireworth’s picture

aireworth commented

#33 - I'm interested but I don't have a site I can use/test it on currently. I'll be doing some testing between Christmas and New Year, I'll try to try it out then
Colin

kyle_mathews’s picture

kyle_mathews commented

I've spent a good 10-15 hours on this code the last few weeks and I finally got it somewhere I'm happy with.

Go to https://github.com/KyleAMathews/og_mailinglist/compare/master...send_all... and click "Files Changed" to see the complete diff so far of what's changed from the master branch.

I've also taken Nigel's patch to PHPMailer and a bit of my own changes and create a new repository to track our changes to that library. You can follow along on the action for these changes at https://github.com/KyleAMathews/phpmailer. If you want to test the new setup, you'll need to grab phpmailer from there. The next release will require everyone to upgrade PHPMailer from our Github repo. Hopefully we can get our patch into PHPMailer soonish so we won't remain a fork forever. The last release however done by the folks behind PHPMailer, however, was over a year ago so we might be maintaining a fork for awhile. Also of note on this PHPMailer fork is it's the PHP5 only version. If someone out there needs a PHP4 version of PHPMailer, they'll need to make the changes themselves (and hopefully add that to the github repo as well).

What's working?

* OGM can now sends email using PHPMailer through both the PHP mail() function and through Sendmail. Yeah! SMTP will need some more work but more on that later.

What's not working or still needs done?

* The admin page needs a way to select how you'd like to send email, Sendmail, mail(), or SMTP.
* Now that emails aren't written to disk, I'd like to add a debug mode which writes both incoming and outgoing emails to disk for logging/debugging.
* I removed Nigel's SMTP implementation (via integration with the Drupal PHPMailer module) and that'll need to be added back.

Something you did Nigel that I wish you hadn't was add the DeliverTo stuff. I thought that was necessary to get the PHPMailer integration working and that + all the extra classes kept me confused for many an hour. Once I reread your comment above (http://drupal.org/node/911974#comment-3747390) I realized that was a separate attempt to remove duplicate emails that you were seeing. In the future, it'd make things a lot simpler for this maintainer and others trying to help to attack only one problem at a time with your patch. And as it is, if you'd posted another issue, there's a far simpler way to remove the duplicate emails that we could of worked on that didn't require adding two additional class files and overriding a number of PHPMailer methods. Please post another issue and we can discuss that there.

On integrating with the PHPMailer module. I'd rather we not do anything special other than call their loading function (phpmailer_load_library()) in our loading function (if on the admin page the person has selected to use smtp) and then to send emails, use their phpmailer_send() function if possible to avoid code duplication.

So please test and give your feedback. This is pretty exciting stuff. Once this gets in, it'll make OGM vastly more flexible and pave the way to fix #951018: Support for shared hosting environemnts

nigelcunningham’s picture

nigelcunningham commented

Good to hear, Kyle.

If you'd spoken with me, I would have saved you some confusion :) It's not just a matter of avoiding duplication of emails, but of keeping the original recipients. We shouldn't be replacing the to and cc with just og_m. If the original message was to 'a' and 'b' and cc'd to ogm, so should the outgoing message (whilst still only being delivered to ogm recipients).

I viewed it as part of the scope of this issue because I don't consider this issue fixed if we remove the sendmail dependency but put in a broken / semi-working replacement. I realise you might disagree there.

Will have a look when I get a chance.

kyle_mathews’s picture

kyle_mathews commented
Status: Needs review » Fixed

I've integrated and tested the changes I made previously here and with the latest beta8 release, all emails are being sent through PHPMailer! This is quite exciting for me. In addition, for a client, I wrote integration with the Mailgun email service which can handle both the sending and receiving of emails removing the dependency on a MTA.

Go and test it out - http://drupal.org/node/1416068

Status: Fixed » Closed (fixed)
Issue tags: -ogm-1.0

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +ogm-1.0

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.