We are using this module and the phpmailer module that is implemented by the http://drupal.org/project/messaging class. There are scenarios (especially during running cron) that the other module loads the PHPMailer class first.

Here is how to prevent that:

  // Include the PHPMailer class (which includes the SMTP class).
  if (!class_exists('PHPMailer')) {
    require_once(drupal_get_path('module', 'smtp') .'/phpmailer/class.phpmailer.php');
  }
CommentFileSizeAuthor
#6 smtp_libraries.diff2.78 KBdesignerbrent
#1 smtp-541942.patch787 bytesjdwfly
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jdwfly’s picture

Version: 6.x-1.0-beta3 » 6.x-1.x-dev
FileSize
787 bytes

Made a patch for this against the dev version. Might work for regular version too.

markus_petrux’s picture

Related issue in the messaging module queue: #541938: using PHPMailer twice crashes messaging_mailer

SMTP could also try first using libraries module. See the patch attached in the above mentioned issue.

That way, sites using SMTP with no other module loading PHPMailer class may take advantage of libraries. ie. the class could be installed out side the module directory, which makes upgrades a lot easier. And here's a good reason to keep 3rd party libraries off the modules directories: #546584: Modules or themes with too many files kill drupal_system_listing performances. ie. less subdirectories under modules directory means less job for drupal_system_listing().

markus_petrux’s picture

Issue tags: +Libraries

tagging

ShutterFreak’s picture

Yes, it would be a great idea to install phpmailer under sites/all/libraries, like the Wysiwyg library already does for the 3rd party HTML editors.

z3cka’s picture

I agree with ShutterFreak. this library should be able to live in sites/all/libraries

The problem I had was when updating this module with drush up, it moves the phpmailer library with it to the backup directory along with the module files.

If it was in the libraries directory, we would not have this problem. I will look into writing a patch that would accomplish this.

designerbrent’s picture

Title: PHPMailer class could already be defined » Move PHPMailer out of the module folder, check for class already defined
FileSize
2.78 KB

Attached is a patch that allows you to place phpmailer outside of the module folder. It looks in 3 locations:

Additionally this adds the hook_requirements function to cleanly warn the user that the phpmailer doesn't exist.

z3cka’s picture

The patch works with phpmailer in sites/all/libraries ! Thanks designerbrent, nice job (now I don't have to TRY to write this patch). +1 for adding to the next release.

Tested on a fresh install with the following modules enabled:

Administration menu          6.x-1.6
 Admin role                   6.x-1.3
 Drupal core                  6.19
 SMTP Authentication Support  6.x-1.0-beta5
esteewhy’s picture

+1 for using libraries.module and keep externals out of module dir.

akalata’s picture

Can also confirm sites/all/libraries success using this patch.

fr34ck’s picture

This patch work perfectly!
Thank you!

Is possible to include this patch in the next release?

franz’s picture

Hi, nice job, looks great. I have a few questions before proceeding:

- Isn't there going to be a dependency on .info for libraries? If so, patch must include.
- SMTP had some issues with 5.x versions of PHPMailer, is it really working?
- There are patches that need to be applied to 2.x versions of PHPMailer. Instructions for these are missing.

Looking forward to those patches also for the D7 version

yettyn’s picture

Thought I should add my thoughts to this as I privately been working on refactoring the module to work with D7 as current dev version is non-working as is.

I was also thinking of and in benefit of adding phpmailer as a library but while it may make sense to D6 alone, I've changed my mind regarding D7. The main reason for this, imho, is that a library like this shouldn't be needed by other modules, and they should really use this module instead if they have need for it. The further conclusion of this is also that there is no point in having several modules offering a smtp service, which is what phpmailer really have to offer D7. The other features are already present as I see, Mime Mail and Html Mail etc.

Futher more phpmailer nice but rather bloated with things we don't need, although alternatively one could do it as adding the full library and change other modules to use these other parts instead of what they use now. I haven't done any investigation here though which modules and what that may be. So adding the full library, just to use the smtp class is bulky in my view and it also has the side effect it cannot be stored in Drupal CVS.

Another small but rather important detail is that phpmailer need to be patched to work with smtp module, which makes it less suitable to live in sites/all/libraries.

The best solution in my view is to take the parts we need and re-release it as GPL (it's now LGPL, which allows to to be modified and re-released) and as a part of this module. I'm willing to take on that job and indeed already have a version that works with current D7 dev version. This way this module will work "out of the box" which I think is what most want to have.

Also, there is a high probability that phpmailer won't have any more updates, at least not for the parts that concerns us, so it shouldn't be any maintainability hog and we could streamline it to fit our needs. What Drupal really need is SMTP to work out the box, and in core, which won't happen before D8 so lets make this something that easily can be dropped into core when the time comes.

Simon Georges’s picture

polskikrol’s picture

Has the patch mentioned above be applied to 6.x-1.0-beta6 when that comes out?

designerbrent’s picture

@yettyn: While I agree with you that there should be no use outside of this module for the PHPMailer library, the real problem for me was that EVERY time I updated the smtp mailer via Drush, it would delete the PHPMailer folder. Then I had to go out and find the right file to download (No small task in itself!) and get it back into place. When you use the libraries method, that is not a problem as you can update the module and the library remains.

franz’s picture

@designerbrent

I think that yettyn suggested, actually, that the module should include the library itself on future releases (we're talking about D7 actually), so it would be 100% out-of-the-box. =)

But concerning D6, we could work on with libraries as well.

markus_petrux’s picture

Maybe there is no need to include the PHPMailer library itself. That's just a class that can be extended to suit custom extensions, but it is not a trivial class, it deals with something complex. Moving it here would duplicate the effort made by others. In fact, this is what the PHPMailer modules does, and it is a nice method to reuse the job made by others, so that each party can be specialized, one knows about SMTP stuff, and the other knows how to integrate that in Drupal.

Maybe these modules could even join forces so that code reuse can be enhanced even more.

Simon Georges’s picture

@markus_petrux, there is #878334: SMTP Authentication vs. PHPMailer module regarding the merge / integration of these two modules.

franz’s picture

@markus_petrux, the join suggestion is good. But both modules are supposed to be doing the same thing. One has the name of the library, the other doesn't. It makes no sense saying it would duplicate efforts, since PHPMailer duplicated the function of SMTP an year ago, and using the same library.

Both modules should integrate perfectly with Drupal, so you don't have to use both. Also, there is the possibility that SMTP become an in-core feature, and that doesn't go well with libraries. I really think that, for D7 the effort must be to include the library, and that's not hard to do.

designerbrent’s picture

Is there anyway that patch #15 could get integrated in the 6.x version anytime soon? Would really help us out.

franz’s picture

uh, there is no patch at #15...

designerbrent’s picture

Sorry! I meant the patch at #6.

franz’s picture

Any idea whether an existing installation (phpmailer inside module folder) is also working without troubles? If someone can confirm this, please put issue on RTBC

akalata’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed, patch in #6 does not adversely affect an existing installation with phpmailer included.

franz’s picture

Status: Reviewed & tested by the community » Fixed

Commited.

z3cka’s picture

hooray! thanks for the commit franz! oadaeh told me that this patch had been committed. I shall start using the dev version today!

Status: Fixed » Closed (fixed)
Issue tags: -Libraries

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