Posted by batje on August 6, 2009 at 4:31pm
| Project: | Messaging |
| Version: | 6.x-2.1 |
| Component: | PHPMailer |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Libraries |
Issue Summary
We have both http://drupal.org/project/smtp and messaging_phpmailer installed, and they seem to 'bite' eachother.
Only when i ran drush cron from the command-prompt I got the error that class PHPMailer was already defined.
That was a nice error! Here is the fix:
function messaging_phpmailer_drupal_mail($message) {
if (!class_exists('PHPMailer')) {
include_once './'. drupal_get_path('module', 'messaging_phpmailer') .'/PHPMailer/class.phpmailer.php';
}
Comments
#1
I don't have any other modules using phpmailer installed, so I can't test that this solves the problem, but I can at least say that it doesn't break anything. Thanks for the report and fix.
#2
I wonder if a better solution in the long-run wouldn't be to make a separate module just to contain PHPMailer, so that administrators don't have to maintain multiple copies of it in multiple modules (and to avoid problems such as this).
#3
It does make sense, but I read some posts that discuss the changes to the Drupal mailer in D7 are going to be significant and cater for this kind of thing.
#4
http://drupal.org/project/phpmailer
- #563112: PHPMailer support for Libraries API!
[EDIT] I have posted a patch to the above mentioned issue, and aside from depending on Libraries API, the PHPMailer class is loaded on demand. Maybe other modules such as messaging_phpmailer in need of the PHP Mailer class could depend on PHPMailer module?
Anyway, if messaging_phpmailer cannot depend on PHPMailer module, then maybe it could depend on Libraires API. This would allow site administrators place the PHPMailer class on a common location that can be shared with other modules in need for this library. And it makes it a lot easier to upgrade Messaging package versions because the PHPMailer class would located on a separate location, which is the point of the Libraries API.
#5
Ok to use libraries and also checking for PHPMailer. However I'd like it to fallback to current location so people with existing installs don't see them breaking after upgrade. Some chained if-then will be fine.
if (!class_exists('PHPMailer')) {if (module_exists(...)) {
....
} else {
include_once './'. drupal_get_path('module', 'messaging_phpmailer') .'/PHPMailer/class.phpmailer.php';
}
}
#6
How about this?
PS: Before trying the patch, line endings in messaging_phpmailer.install need to be fixed in the repository. It is now using CRLF!
#7
Oops! fixing typo in watchdog message: s/Could/Could not
#8
For the record: here's a good reason to keep 3rd party libraries off the modules directories: #546584: allow to exclude folders from drupal_system_listing. ie. less subdirectories under modules directory means less job for drupal_system_listing().
#9
This looks very good. Just about the logic, could be cleaned up a little bit
if (!class_exists('PHPMailer')) {// First, try using libraries module.
$phpmailer_library = libraries_get_path('phpmailer') .'/class.phpmailer.php';
if (module_exists('libraries') && file_exists($phpmailer_library)) {
include_once $phpmailer_library;
}
else {
// If PHPMailer is not already loaded, then try from module subdirectory.
$phpmailer_library = drupal_get_path('module', 'messaging_phpmailer') .'/PHPMailer/class.phpmailer.php';
if (file_exists($phpmailer_library)) {
include_once $phpmailer_library;
}
}
}
Also note we remove './.' from paths. Does this look good / works?
#10
Thanks for taking the time to review the patch.
1) Removing './.' from paths will work, but PHP will first look at include_path directories. The './' prefix makes PHP ignore include_path altogether. So this is used for perfomance reasons.
http://www.php.net/include
2) We cannot invoke libraries_get_path() if we haven't checked first if libraries module exists.
I tested the patch in #7 with and without PHPMailer class, with and without libraries module. And worked here, but it would be nice if someone else (other than me) can confirm this.
#11
tagging
#12
Fair enough, applied patch in #7
Thanks.
#13
Automatically closed -- issue fixed for 2 weeks with no activity.
#14
Noob question: where would I put that code?
Thanks
A
#15
has this been committed?