-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replaced Zend with PHPMailer #158
Conversation
classes/MailWrapper.php
Outdated
{ | ||
$mail = new PHPMailer(); | ||
$mail->isSendMail(); | ||
$mail->Sender = 'fxdmod-bounces@ccr.buffalo.edu'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This address does not appear to be correct you have an extra f prepended.
composer.lock
Outdated
@@ -2688,6 +2750,28 @@ | |||
"description": "Symfony Yaml Component", | |||
"homepage": "https://symfony.com", | |||
"time": "2015-07-26T08:59:42+00:00" | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not have been added to the lock file. Please regenerate the lock file.
$mail->setFrom($contact, $title); | ||
|
||
$bcc_emails = explode(',', $target_addresses); | ||
|
||
foreach ($bcc_emails as $b) { | ||
$mail->addBcc($b); | ||
$mail->AddBCC($b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/AddBCC/addBCC/
classes/MailWrapper.php
Outdated
|
||
public static function init() | ||
{ | ||
$mail = new PHPMailer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the indentation to use spaces rather than tabs.
} else { | ||
$mail->setSubject("[xdmod] New ". ($linked ? "linked": "unlinked") ." federated user created"); | ||
$mail->Subject = "[xdmod] New ". ($linked ? "linked": "unlinked") ." federated user created"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add spaces before/after concatenation operators.
try { | ||
$mail->send(); | ||
return true; | ||
} catch (Exception $e) { | ||
return $e->getMessage(); | ||
return $e->getMessage() . "\n" . $mail->ErrorInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should prefix the error reporting by the module that it occurred in. For example: "Error sending mail to '%s': %s", $email, $errorMessage.
try { | ||
$mail->send(); | ||
return true; | ||
} catch (Exception $e) { | ||
return $e->getMessage(); | ||
return $e->getMessage() . "\n" . $mail->ErrorInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than return true
in one case and a string in another case, you should re-throw the exception and catch it in the calling routine. This makes error handling easier.
@@ -164,12 +164,12 @@ private function notifyAdminOfNewUser($user, $samlAttributes, $linked, $error = | |||
"Additional SAML Attributes ----------------------------------\n\n" . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only add SAML attributes if $samlAttributes
is not empty.
classes/MailWrapper.php
Outdated
|
||
public static function init() | ||
{ | ||
$mail = new PHPMailer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation.
classes/MailWrapper.php
Outdated
{ | ||
$mail = new PHPMailer(); | ||
$mail->isSendMail(); | ||
$mail->Sender = 'fxdmod-bounces@ccr.buffalo.edu'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a configurable email address. And it appears to have an extra "f".
crontabs/xdmod
Outdated
@@ -0,0 +1,35 @@ | |||
MAILTO=ccr-xdmod-cron@buffalo.edu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be added. Please remove it and rebase your commits.
crontabs/xdtas
Outdated
@@ -0,0 +1,28 @@ | |||
# Edit this file to introduce tasks to be run by cron. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have a bit of work to do...
classes/MailWrapper.php
Outdated
@@ -1,15 +0,0 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were going to keep MailWrapper.php
but change init()
to initPhpMailer($sender)
?
crontabs/xdtas
Outdated
@@ -0,0 +1,28 @@ | |||
# Edit this file to introduce tasks to be run by cron. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file here?
crontabs/xdmod
Outdated
@@ -0,0 +1,35 @@ | |||
MAILTO=ccr-xdmod-cron@buffalo.edu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file here?
html/controllers/mailer/contact.php
Outdated
@@ -72,7 +72,9 @@ | |||
? xd_utilities\getConfiguration('general', 'debug_recipient') | |||
: xd_utilities\getConfiguration('general', 'contact_page_recipient'); | |||
|
|||
$mail = MailWrapper::init(); | |||
$mail = new PHPMailer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above about MailWrapper.php
html/controllers/mailer/contact.php
Outdated
$mail = MailWrapper::init(); | ||
$mail = new PHPMailer(); | ||
$mail->isSendMail(); | ||
$mail->Sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than call \xd_utilities\getConfiguration('mailer', 'sender_email')
multiple times, consider calling it once and setting a variable.
print_r($samlAttributes, true); | ||
"\nE-Mail: " . $userEmail ; | ||
|
||
if(!empty($samlAttributes)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is $samlAttributes
an array? If so, using if( 0 == count($samlAttributes) )
would be a better choice.
return true; | ||
if(!$mail->send()){ | ||
error_log("Mail failed to send to: " . $userEmail . "\n" . $mail->ErrorInfo); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logs the error, but still returns true
. Is that your intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the source code for phpmailer it can be configured whether or not to throw exceptions and it defaults to false. This means that the way we are currenlty using this the mailer class will not throw. We should either:
- Enable exceptions in phpmailer and change the code to catch them (and don't bother checking the return code.
- Leave the phpmailer config at default and don't bother trying to catch exceptions that can never be thrown.
Also can we try to find a way of testing these error cases? Perhaps deliberately switch off postfix on a test system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it supports exceptions I say we configure it that way, since that is consistent with what we do normally.
|
||
$mail->Body = $body; | ||
try { | ||
$mail->send(); | ||
return true; | ||
if(!$mail->send()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces are your friend - they make code far more readable, especially when looking at it in a non-colorized context. This would read better as if( ! $mail->send() ) {
} catch (Exception $e) { | ||
return $e->getMessage() . "\n" . $mail->ErrorInfo; | ||
error_log("Mail failed to send." . $mail->ErrorInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not log the same message being thrown by the exception? It has far more information in it.
self::notifyAdminOfNewUser($newUser, $samlAttrs, ($personId != -2)); | ||
return $newUser; | ||
} catch (Exception $e) { | ||
self::notifyAdminOfNewUser($newUser, $samlAttrs, ($personId != -2), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not going to do anything with the exception that is thrown, you should return false
instead and check the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is being used https://github.com/ubccr/xdmod/blob/xdmod7.0/html/gui/general/login.php#L14 but looks like it should be able to be pretty easily converted to a false in there.
The most recent changes look good, but you will need to add |
classes/MailWrapper.php
Outdated
@@ -1,5 +1,7 @@ | |||
<?php | |||
|
|||
use CCR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review the PHP namespace documentation on how to fix this.
html/controllers/mailer/contact.php
Outdated
$mail->setSubject($subject); | ||
$mail->addTo($recipient); | ||
$mail->Subject = $subject; | ||
$mail->addAddress($recipient); | ||
|
||
//$mail->setFrom($mailer_sender, 'XDMoD'); | ||
|
||
//Original sender's e-mail must be in the 'From' field for the XDMoD Request Tracker to function | ||
$mail->setFrom($_POST['email']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change to the behaviour of the code. the old version of the code would set the from address and the envelope address to the contents of the $_POST['email'] and the new code sets the envelope to the default sender and the from address to $_POST['email'].
Is this change intended? If so then please document the motivation (i.e. what bug was fixed). If not then please revert to the original behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The from address and envelope address are still the same ($_POST['email']), however, what changed was how the Sender value was set. Sender, in the old code, was hard coded. Now, the Sender value has been stored in a variable ($sender) that gets the value stored in the config file, thus allowing Sender to now be a configurable address. When Sender and setFrom have different values, Sender appends itself to setFrom's value (i.e. some.email@yahoo.com via buffalo.edu, for example). If Sender has no value, setFrom sets Sender to its own value (the same).
html/controllers/mailer/sign_up.php
Outdated
|
||
$recipient | ||
= (xd_utilities\getConfiguration('general', 'debug_mode') == 'on') | ||
? xd_utilities\getConfiguration('general', 'debug_recipient') | ||
: xd_utilities\getConfiguration('general', 'contact_page_recipient'); | ||
$mail->addTo($recipient); | ||
$mail->addAddress($recipient); | ||
|
||
// Original sender's e-mail must be in the "From" field for the XDMoD | ||
// Request Tracker to function | ||
$mail->setFrom($_POST['email']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above:
This is a change to the behaviour of the code. the old version of the code would set the from address and the envelope address to the contents of the $_POST['email'] and the new code sets the envelope to the default sender and the from address to $_POST['email'].
Is this change intended? If so then please document the motivation (i.e. what bug was fixed). If not then please revert to the original behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
@@ -105,22 +105,21 @@ | |||
|
|||
$page_title = xd_utilities\getConfiguration('general', 'title'); | |||
$site_address = xd_utilities\getConfigurationUrlBase('general', 'site_address'); | |||
$mailer_sender = xd_utilities\getConfiguration('mailer', 'sender_email'); | |||
$sender = xd_utilities\getConfiguration('mailer', 'sender_email'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other parts of this pull request you call strtolower() on the sender email string. It is not done here. Why do you call tolower at all? Why is it different here?
Do we maybe want to have a helper function (or two) of something like MailWrapper::sendSystemEmail($subject, $body) It seems most of our emails are all sent as the "XDMoD" User and the only difference is the subject and body. Might clean up the code a bit more. Another one might just return the mail object with some of the stuff already set up like the from / to / reply based on whatever email is being sent. |
$mail->Sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
$sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
|
||
$mail = MailWrapper::initPHPMailer($sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is $sender used anywhere else, if not why bother putting it into a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting $sender in a variable and passing it into the MailWrapper class allows $sender to be set to a configurable email address, rather than be hardcoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, but why assign it to a variable outside of the wrapper instead of just sending it in like this:
$mail = MailWrapper::initPHPMailer(strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')));
classes/XDReportManager.php
Outdated
$mail->setFrom($mailer_sender, 'XDMoD'); | ||
$mail = MailWrapper::initPHPMailer($sender); | ||
|
||
$mail->setFrom($sender, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this uses setFrom instead of sender like the other file, should we make these the same
It looks like https://github.com/PHPMailer/PHPMailer/blob/master/class.phpmailer.php#L1011 setFrom internally also sets the sender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setFrom only sets sender if sender has no value assigned to it. An email is sent from the setFrom address on behalf of the sender address. No, they shouldn't be the same.
html/controllers/mailer/contact.php
Outdated
$mail = new PHPMailer(); | ||
$mail->isSendMail(); | ||
$mail->Sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
$mail = MailWrapper::initPHPMailer($sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as others, do we need to store $sender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes because storing that value allows for a configurable email address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing it doesnt allow for the passing it in as a variable to the function does that.
$mail->isSendMail(); | ||
$mail->Sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
$mail->setFrom($mailer_sender, 'XDMoD'); | ||
$sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comments
$mail->Sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
$sender = strtolower(\xd_utilities\getConfiguration('mailer', 'sender_email')); | ||
|
||
$mail = MailWrapper::initPHPMailer($sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comments
if ($error) { | ||
$mail->setSubject("[xdmod] Error Creating federated user"); | ||
$mail->Subject = "[xdmod] Error Creating federated user"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some places we use the xd_utilities\getConfiguration('general', 'title');
should we be using that here instead of [xdmod]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because that is unrelated to the creation of federated users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it is unrelated to that, it is only related to how emails are sent out. However if you mean it is unrelated to the change from ZendMail to PHPMailer, I can agree to that.
classes/XDReportManager.php
Outdated
|
||
$mail->setFrom($mailer_sender, 'XDMoD'); | ||
$mail->setFrom($sender, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some places we use the xd_utilities\getConfiguration('general', 'title');
should we be using that here instead of [xdmod]
html/controllers/mailer/contact.php
Outdated
$mail->setSubject("Thank you for your $message_type."); | ||
$mail->addTo($_POST['email']); | ||
$mail = MailWrapper::initPHPMailer($sender); | ||
$mail->setFrom($sender, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some places we use the xd_utilities\getConfiguration('general', 'title');
should we be using that here instead of [xdmod]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some cases, that would be better to use because it specifies which XDMoD the email is from.
in the places where we use |
classes/MailWrapper.php
Outdated
{ | ||
$mail = new PHPMailer(true); | ||
$mail->isSendMail(); | ||
$mail->Sender = $sender; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we always want the sender to be \xd_utilities\getConfiguration('mailer', 'sender_email')
but we want the from to be configurable from the caller maybe something like this would be more appropriate
public static function initPHPMailer($fromEmail = null, $fromName = '')
{
$mail = new PHPMailer(true);
$mail->isSendMail();
$address = \xd_utilities\getConfiguration('mailer', 'sender_email');
$name = \xd_utilities\getConfiguration('general', 'title');
$mail->Sender = $address;
if($fromEmail !== null) {
$address = $fromEmail;
$name = $fromName;
}
try {
$mail->setFrom($address, $name);
}
catch(phpmailerException $e){
/*
Handle invalid sender address here
*/
}
return $mail;
}
this should reduce code duplication and set up the email to send properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edits to @plessbd example for clarity
public static function initPHPMailer($fromEmail = null, $fromName = null);
{
$mail = new PHPMailer(true);
$mail->isSendMail();
$mail->Sender = \xd_utilities\getConfiguration('mailer', 'sender_email');
$address = ( null !== $fromEmail ? $fromEmail : $mail->Sender );
$name = ( null !== $fromName ? $fromName : \xd_utilities\getConfiguration('general', 'title') );
try {
$mail->setFrom($address, $name);
}
catch (phpmailerException $e) {
/*
Handle invalid sender address here
*/
}
return $mail;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on @smgallo edits:
Doing this could end up with it coming from \xd_utilities\getConfiguration('general', 'title') user@someDomain.tld
|
||
$mail = MailWrapper::initPHPMailer($sender); | ||
$mail = MailWrapper::initPHPMailer($userEmail, \xd_utilities\getConfiguration('general', 'title')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
Also why the change from $sender to $userEmail where is $userEmail defined?
classes/CCR/MailWrapper.php
Outdated
$name = \xd_utilities\getConfiguration('general', 'title'); | ||
$mail->Sender = $address; | ||
|
||
if($fromEmail !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at steves example and wrok to find the best of both worlds
classes/XDReportManager.php
Outdated
$mail = MailWrapper::initPHPMailer($sender); | ||
|
||
$mail->setFrom($sender, 'XDMoD'); | ||
$mail = MailWrapper::initPHPMailer(xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
html/controllers/mailer/contact.php
Outdated
@@ -116,8 +115,7 @@ | |||
|
|||
// ===================================================== | |||
|
|||
$mail = MailWrapper::initPHPMailer($sender); | |||
$mail->setFrom($sender, 'XDMoD'); | |||
$mail = MailWrapper::initPHPMailer(\xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
|
||
// ------------------- | ||
|
||
$mail = MailWrapper::initPHPMailer($sender); | ||
$mail->setFrom($sender, 'XDMoD'); | ||
$mail = MailWrapper::initPHPMailer(\xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
|
||
$mail = MailWrapper::initPHPMailer($sender); | ||
$mail->setFrom($sender, 'XDMoD'); | ||
$mail = MailWrapper::initPHPMailer(\xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
|
||
$mail = MailWrapper::initPHPMailer($sender); | ||
$mail->setFrom($sender, 'XDMoD'); | ||
$mail = MailWrapper::initPHPMailer(\xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new mailwrapper defaults to \xd_utilities\getConfiguration('mailer', 'sender_email')
and xd_utilities\getConfiguration('general', 'title')
no need to pass them in
@@ -142,8 +142,9 @@ public function getLoginLink() | |||
} | |||
private function notifyAdminOfNewUser($user, $samlAttributes, $linked, $error = false) | |||
{ | |||
$userEmail = $user->getEmailAddress(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we getFormalName here as well to be able to add it to the from?
classes/XDReportManager.php
Outdated
"Your$frequency " . $templateConfig['subject'] . " $subject_suffix"; | ||
$mail->Body = $templateConfig['message']; | ||
try { | ||
$mail = MailWrapper::initPHPMailer(null, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to send this stuff in, it will default to what it needs to
html/controllers/mailer/contact.php
Outdated
try { | ||
$mail = MailWrapper::initPHPMailer($_POST['email']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the post have a name we can attach to the email?
html/controllers/mailer/contact.php
Outdated
// ------------------- | ||
|
||
try { | ||
$mail = MailWrapper::initPHPMailer(null, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass anything in the defaults will take care of this.
html/controllers/mailer/sign_up.php
Outdated
|
||
$response = array(); | ||
|
||
try { | ||
$mail = MailWrapper::initPHPMailer($_POST['email']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the post have a name we can pass in? Same with the Replyto
@@ -110,7 +110,7 @@ | |||
|
|||
// ------------------- | |||
|
|||
$mail = MailWrapper::initPHPMailer(\xd_utilities\getConfiguration('mailer', 'sender_email'), 'XDMoD'); | |||
$mail = MailWrapper::initPHPMailer(null, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pass anything in defaults should be fine here.
$mail->Subject = "$page_title: Password Reset"; | ||
$mail->addAddress($recipient); | ||
try { | ||
$mail = MailWrapper::initPHPMailer(null, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass anything in, defaults should be fine
|
||
$message = MailTemplates::passwordReset($user_to_email); | ||
$message = MailTemplates::passwordReset($user_to_email); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a future update, we should probably make this MailTemplates part of the mailwrapper class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On another branch, yes
$mail->Subject = "$page_title: Password Reset"; | ||
$mail->addAddress($recipient); | ||
try { | ||
$mail = MailWrapper::initPHPMailer(null, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass anything in the defaults should be fine.
html/controllers/mailer/contact.php
Outdated
$mail->Subject = $subject; | ||
$mail->addAddress($recipient); | ||
|
||
//$mail->setFrom($mailer_sender, 'XDMoD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add commented out code. See e.g. https://softwareengineering.stackexchange.com/questions/190096/can-commented-out-code-be-valuable-documentation
Description
Removed Zend framework from xdmod and replaced it with PHPMailer. All functions and methods relating to Zend were reconfigured to the corresponding PHPMailer methods/functions.
Motivation and Context
To replace Zend with a more robust framework, i.e. PHPMailer.
Tests performed
To start I continuously signed up to xdmod using a test email I created (test2) and I had a user created (test1) for me which had Manager, User, Principal Investigator, and Developer privileges. Using this test1 account, I started out by completing the "Send Message" and "Request Feature" forms under the "Contact Us" tab (updating code as needed), and repeated this process until both the user and xdmod received the proper emails (I knew xdmod would receive these emails because I set my test1 email to the debug email, so all emails that xdmod would normally receive, my test1 received). Next, to test creating a user, I went into the dashboard and created a test user with only User privileges using the test2 email account. I then sent a password reset email to this account to test the password reset feature (from an admin's account) which was successful. I then logged in as this new user and generated a report via the report manager. I sent an email with an attachment to the user successfully. To test again the "Send Message" and "Request Feature" forms (from a different account than previously) I completed them and it was successful. To then test the user password reset functionality, I logged out completely, proceeded to the "Sign In" link and next to "Trouble logging in?" clicked on "Click Here" which in turn sent a password reset email to the user's email, which is what should happen. To then test the SAML Authentication (logging in via federated credentials) I went back into the "Sign In" link and clicked on the federation login banner.
I then had another colleague sign up with another email to ensure it worked and it did.
Checklist: