From 1b11f2587b8f78759ddc46e155272dcaf4882722 Mon Sep 17 00:00:00 2001 From: John Linhart Date: Fri, 26 May 2017 15:50:45 +0200 Subject: [PATCH 1/4] Added unit tests of the validateEmail method - failing for now --- .../Tests/Helper/MailHelperTest.php | 88 +++++++++++++++++-- 1 file changed, 82 insertions(+), 6 deletions(-) diff --git a/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php b/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php index 26fef5b98f5..b13cdf4a46b 100644 --- a/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php +++ b/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php @@ -13,6 +13,7 @@ use Mautic\CoreBundle\Factory\MauticFactory; use Mautic\EmailBundle\Entity\Email; +use Mautic\EmailBundle\Helper\MailHelper; use Mautic\EmailBundle\MonitoredEmail\Mailbox; use Mautic\EmailBundle\Swiftmailer\Exception\BatchQueueMaxException; use Mautic\EmailBundle\Tests\Helper\Transport\BatchTransport; @@ -82,7 +83,7 @@ public function testQueueModeThrowsExceptionWhenBatchLimitHit() $swiftMailer = new \Swift_Mailer(new BatchTransport()); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); // Enable queue mode $mailer->enableQueue(); @@ -110,7 +111,7 @@ public function testQueueModeDisabledDoesNotThrowsExceptionWhenBatchLimitHit() $swiftMailer = new \Swift_Mailer(new BatchTransport()); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); // Enable queue mode try { @@ -131,7 +132,7 @@ public function testQueuedEmailFromOverride() $transport = new BatchTransport(); $swiftMailer = new \Swift_Mailer($transport); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); $mailer->enableQueue(); $email = new Email(); @@ -172,7 +173,7 @@ public function testBatchMode() $transport = new BatchTransport(true); $swiftMailer = new \Swift_Mailer($transport); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); $mailer->enableQueue(); $email = new Email(); @@ -203,7 +204,7 @@ public function testQueuedOwnerAsMailer() $transport = new BatchTransport(); $swiftMailer = new \Swift_Mailer($transport); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); $mailer->enableQueue(); $mailer->setSubject('Hello'); @@ -258,7 +259,7 @@ public function testStandardOwnerAsMailer() $transport = new SmtpTransport(); $swiftMailer = new \Swift_Mailer($transport); - $mailer = new \Mautic\EmailBundle\Helper\MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); + $mailer = new MailHelper($mockFactory, $swiftMailer, ['nobody@nowhere.com' => 'No Body']); $mailer->setBody('{signature}'); foreach ($this->contacts as $key => $contact) { @@ -279,6 +280,81 @@ public function testStandardOwnerAsMailer() } } + public function testValidateValidEmails() + { + $helper = $this->mockEmptyMailHelper(); + $addresses = [ + 'john@doe.com', + 'john@doe.email', + 'john.doe@email.com', + 'john+doe@email.com', + 'john@doe.whatevertldtheycomewithinthefuture', + ]; + + foreach ($addresses as $address) { + // will throw Swift_RfcComplianceException if it will find the address invalid + $helper::validateEmail($address); + } + } + + public function testValidateEmailWithoutTld() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('john@doe'); + } + + public function testValidateEmailWithSpaceInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo hn@doe.email'); + } + + public function testValidateEmailWithCaretInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo^hn@doe.email'); + } + + public function testValidateEmailWithApostropheInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo\'hn@doe.email'); + } + + public function testValidateEmailWithSemicolonInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo;hn@doe.email'); + } + + public function testValidateEmailWithAmpersandInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo&hn@doe.email'); + } + + public function testValidateEmailWithStarInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo*hn@doe.email'); + } + + public function testValidateEmailWithPercentInIt() + { + $this->setExpectedException(\Swift_RfcComplianceException::class); + $this->mockEmptyMailHelper()::validateEmail('jo%hn@doe.email'); + } + + protected function mockEmptyMailHelper() + { + $mockFactory = $this->getMockFactory(); + + $transport = new SmtpTransport(); + $swiftMailer = new \Swift_Mailer($transport); + + return new MailHelper($mockFactory, $swiftMailer); + } + protected function getMockFactory($mailIsOwner = true) { $mockLeadRepository = $this->getMockBuilder(LeadRepository::class) From eb437ab5a69d0c3a58b0da7a14c52ece914e8e60 Mon Sep 17 00:00:00 2001 From: John Linhart Date: Fri, 26 May 2017 15:51:13 +0200 Subject: [PATCH 2/4] ValidateEmail method updated so the unit tests would pass --- app/bundles/EmailBundle/Helper/MailHelper.php | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/app/bundles/EmailBundle/Helper/MailHelper.php b/app/bundles/EmailBundle/Helper/MailHelper.php index 44837a70c57..3f469fc791a 100644 --- a/app/bundles/EmailBundle/Helper/MailHelper.php +++ b/app/bundles/EmailBundle/Helper/MailHelper.php @@ -476,12 +476,12 @@ public function send($dispatchSendEvent = false, $isQueueFlush = false, $useOwne * * @param bool $dispatchSendEvent * @param string $returnMode What should happen post send/queue to $this->message after the email send is attempted. - * Options are: - * RESET_TO resets the to recipients and resets errors - * FULL_RESET creates a new MauticMessage instance and resets errors - * DO_NOTHING leaves the current errors array and MauticMessage instance intact - * NOTHING_IF_FAILED leaves the current errors array MauticMessage instance intact if it fails, otherwise reset_to - * RETURN_ERROR return an array of [success, $errors]; only one applicable if message is queued + * Options are: + * RESET_TO resets the to recipients and resets errors + * FULL_RESET creates a new MauticMessage instance and resets errors + * DO_NOTHING leaves the current errors array and MauticMessage instance intact + * NOTHING_IF_FAILED leaves the current errors array MauticMessage instance intact if it fails, otherwise reset_to + * RETURN_ERROR return an array of [success, $errors]; only one applicable if message is queued * * @return bool */ @@ -1252,17 +1252,19 @@ public function setFrom($address, $name = null) */ public static function validateEmail($address) { - static $grammer; + $invalidChar = strpbrk($address, '\'^&*%'); - if ($grammer === null) { - $grammer = new \Swift_Mime_Grammar(); + if ($invalidChar !== false) { + throw new \Swift_RfcComplianceException( + 'Address in mailbox given ['.$address. + '] contains the invalid character: '.substr($invalidChar, 0, 1) + ); } - if (!preg_match('/^'.$grammer->getDefinition('addr-spec').'$/D', - $address)) { + if (!filter_var($address, FILTER_VALIDATE_EMAIL)) { throw new \Swift_RfcComplianceException( 'Address in mailbox given ['.$address. - '] does not comply with RFC 2822, 3.6.2.' + '] does not comply with RFC 882' ); } } @@ -1614,7 +1616,7 @@ protected function logError($error, $context = null) // Clean up the error message $errorMessage = trim(preg_replace('/(.*?)Log data:(.*)$/is', '$1', $errorMessage)); - $this->fatal = true; + $this->fatal = true; } else { $errorMessage = trim($error); } From 4fd90fd38056d5b7936c0df21d31f7f3de072b8b Mon Sep 17 00:00:00 2001 From: John Linhart Date: Fri, 26 May 2017 16:02:51 +0200 Subject: [PATCH 3/4] Make PHP 5.6 happy --- .../Tests/Helper/MailHelperTest.php | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php b/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php index b13cdf4a46b..2ff24b4de3f 100644 --- a/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php +++ b/app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php @@ -55,9 +55,6 @@ class MailHelperTest extends \PHPUnit_Framework_TestCase ], ]; - /** - * - */ public function setUp() { defined('MAUTIC_ENV') or define('MAUTIC_ENV', 'test'); @@ -282,7 +279,7 @@ public function testStandardOwnerAsMailer() public function testValidateValidEmails() { - $helper = $this->mockEmptyMailHelper(); + $helper = $this->mockEmptyMailHelper(); $addresses = [ 'john@doe.com', 'john@doe.email', @@ -299,56 +296,63 @@ public function testValidateValidEmails() public function testValidateEmailWithoutTld() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('john@doe'); + $heper::validateEmail('john@doe'); } public function testValidateEmailWithSpaceInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo hn@doe.email'); + $heper::validateEmail('jo hn@doe.email'); } public function testValidateEmailWithCaretInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo^hn@doe.email'); + $heper::validateEmail('jo^hn@doe.email'); } public function testValidateEmailWithApostropheInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo\'hn@doe.email'); + $heper::validateEmail('jo\'hn@doe.email'); } public function testValidateEmailWithSemicolonInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo;hn@doe.email'); + $heper::validateEmail('jo;hn@doe.email'); } public function testValidateEmailWithAmpersandInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo&hn@doe.email'); + $heper::validateEmail('jo&hn@doe.email'); } public function testValidateEmailWithStarInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo*hn@doe.email'); + $heper::validateEmail('jo*hn@doe.email'); } public function testValidateEmailWithPercentInIt() { + $heper = $this->mockEmptyMailHelper(); $this->setExpectedException(\Swift_RfcComplianceException::class); - $this->mockEmptyMailHelper()::validateEmail('jo%hn@doe.email'); + $heper::validateEmail('jo%hn@doe.email'); } protected function mockEmptyMailHelper() { $mockFactory = $this->getMockFactory(); - $transport = new SmtpTransport(); $swiftMailer = new \Swift_Mailer($transport); From ee599fb46dff002f05efecc5a85a77dcd6cc5d37 Mon Sep 17 00:00:00 2001 From: John Linhart Date: Tue, 30 May 2017 20:54:57 +0200 Subject: [PATCH 4/4] Email validation messages simplified --- app/bundles/EmailBundle/Helper/MailHelper.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/bundles/EmailBundle/Helper/MailHelper.php b/app/bundles/EmailBundle/Helper/MailHelper.php index 3f469fc791a..cb22ebe1d4c 100644 --- a/app/bundles/EmailBundle/Helper/MailHelper.php +++ b/app/bundles/EmailBundle/Helper/MailHelper.php @@ -1256,15 +1256,14 @@ public static function validateEmail($address) if ($invalidChar !== false) { throw new \Swift_RfcComplianceException( - 'Address in mailbox given ['.$address. - '] contains the invalid character: '.substr($invalidChar, 0, 1) + 'Email address ['.$address. + '] contains this invalid character: '.substr($invalidChar, 0, 1) ); } if (!filter_var($address, FILTER_VALIDATE_EMAIL)) { throw new \Swift_RfcComplianceException( - 'Address in mailbox given ['.$address. - '] does not comply with RFC 882' + 'Email address ['.$address.'] is invalid' ); } }