Skip to content
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

Add mailbox validator #194

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions EmailValidator/Validation/Error/IllegalMailbox.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

namespace Egulias\EmailValidator\Validation\Error;

use Egulias\EmailValidator\Exception\InvalidEmail;

class IllegalMailbox extends InvalidEmail
{
const CODE = 995;
const REASON = "The mailbox is illegal.";

/**
* @var int
*/
private $responseCode;

/**
* IllegalMailbox constructor.
*
* @param int $responseCode
*/
public function __construct($responseCode)
{
parent::__construct();

$this->responseCode = $responseCode;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't this line nor the private var. It is enough to define constants as you have done.
Please leave the class with only the constants.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this place private var is necessary because responseCode contains SMTP response code https://www.greenend.org.uk/rjk/tech/smtpreplies.html

Users can understand the reason, why mailbox is illegal.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.
What happens is that InvalidEmail does not provide a method for accessing responseCode, which is private. Thus, user won't be able to get the code.
The only way for user, AFAIK, to get the code would be to overwrite intherited __toString (see https://www.php.net/manual/en/class.exception.php and https://www.php.net/manual/en/language.exceptions.extending.php), the only method allowed to be overwritten:

public function __toString()
{
  return ": SMTP response code was {$this->responseCode}, exception reason,code {$this->message} - {$this->code}";
}

Another solution would be create an empty interface EmailValidationException, make InvalidEmail implement it and then

InvalidMailbox extends \InvalidArgumentException implements EmailValidationException
{
    private $reasons = [550 => 'Requested action not taken: mailbox unavailable', 551 => ...]
    public function __construct($responseCode)
    {
        $selectedReason = isset($reasons[$responseCode]) ? $reasons[$responseCode] : "Unknown reason";
        parent::__construct($selectedReason, $responseCode);
    }
}

And adapt all the necesary code to allow for this new type of exception by requiring the interface EmailValidationException instead of the concrete class InvalidEmail.

What do you think?

}
}
240 changes: 240 additions & 0 deletions EmailValidator/Validation/MailboxCheckValidation.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
<?php

namespace Egulias\EmailValidator\Validation;

use Egulias\EmailValidator\EmailLexer;
use Egulias\EmailValidator\Exception\InvalidEmail;
use Egulias\EmailValidator\Validation\Error\IllegalMailbox;
use Egulias\EmailValidator\Warning\NoDNSMXRecord;
use Egulias\EmailValidator\Warning\SocketWarning;
use Egulias\EmailValidator\Warning\Warning;

class MailboxCheckValidation implements EmailValidation
{
const END_OF_LINE = "\r\n";

/**
* @var InvalidEmail
*/
private $error;

/**
* @var Warning[]
*/
private $warnings = [];

/**
* @var int
*/
private $lastResponseCode;

/**
* @var int
*/
private $port = 25;

/**
* @var int
*/
private $timeout = 10;

/**
* @var string
*/
private $fromEmail = '[email protected]';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not looks like a sensible default. If it is a required configuration, require it via constructor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix


/**
* MailboxCheckValidation constructor.
*/
public function __construct()
{
if (!extension_loaded('intl')) {
throw new \LogicException(sprintf('The %s class requires the Intl extension.', __CLASS__));
}
}

/**
* @inheritDoc
*/
public function getError()
{
return $this->error;
}

/**
* @inheritDoc
*/
public function getWarnings()
{
return $this->warnings;
}

/**
* @return int
*/
public function getLastResponseCode()
egulias marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->lastResponseCode;
}

/**
* @return int
*/
public function getPort()
{
return $this->port;
}

/**
* @param int $port
*/
public function setPort($port)
egulias marked this conversation as resolved.
Show resolved Hide resolved
{
$this->port = $port;
}

/**
* @return int
*/
public function getTimeout()
{
return $this->timeout;
}

/**
* @param int $timeout
*/
public function setTimeout($timeout)
egulias marked this conversation as resolved.
Show resolved Hide resolved
{
$this->timeout = $timeout;
}

/**
* @return string
*/
public function getFromEmail()
{
return $this->fromEmail;
}

/**
* @param string $fromEmail
*/
public function setFromEmail($fromEmail)
egulias marked this conversation as resolved.
Show resolved Hide resolved
{
$this->fromEmail = $fromEmail;
}

/**
* @inheritDoc
*/
public function isValid($email, EmailLexer $emailLexer)
{
$mxHosts = $this->getMXHosts($email);

$isValid = false;
foreach ($mxHosts as $mxHost) {
if ( ($isValid = $this->checkMailbox($mxHost, $this->port, $this->timeout, $this->fromEmail, $email)) ) {
break;
egulias marked this conversation as resolved.
Show resolved Hide resolved
}
}

if ( ! $isValid ) {
$this->error = new IllegalMailbox($this->lastResponseCode);
}

return $this->error === null;
}

/**
* @param string $email
*
* @return array
*/
protected function getMXHosts($email)
{
$variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : INTL_IDNA_VARIANT_2003;

$hostname = $email;
if ( false !== ($lastAtPos = strrpos($email, '@')) ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things.

  1. Please don't assign and evaluate.
  2. Please check for truth

So, what about this:

$lastAtPos = (bool) strrpos($email, '@')
if ($lastAtPos) { ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

$hostname = substr($email, $lastAtPos + 1);
}
$hostname = rtrim(idn_to_ascii($hostname, IDNA_DEFAULT, $variant), '.') . '.';
Copy link
Owner

@egulias egulias Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid mutating variables this way, it is hard to follow.
Maybe

$hostname = $this->extractHostname($email);

private function extractHostname($email)
{
  $lastAtPos = strrpos($email, '@')
  $variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : INTL_IDNA_VARIANT_2003;
  if ((bool) $lastAtPos) { 
    $removedAT = substr($email, $lastAtPos + 1);
    return rtrim(idn_to_ascii($removedAT, IDNA_DEFAULT, $variant), '.') . '.';
  }
  return rtrim(idn_to_ascii($email, IDNA_DEFAULT, $variant), '.') . '.';
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix


$mxHosts = [];
$result = getmxrr($hostname, $mxHosts);
if ( ! $result ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove space between ! and $ in all the files :).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

$this->warnings[NoDNSMXRecord::CODE] = new NoDNSMXRecord();
}

return $mxHosts;
}

/**
* @param string $hostname
* @param int $port
* @param int $timeout
* @param string $fromEmail
* @param string $toEmail
* @return bool
*/
protected function checkMailbox($hostname, $port, $timeout, $fromEmail, $toEmail)
egulias marked this conversation as resolved.
Show resolved Hide resolved
{
$socket = @fsockopen($hostname, $port, $errno, $errstr, $timeout);

if ( ! $socket ) {
$this->warnings[SocketWarning::CODE][] = new SocketWarning($hostname, $errno, $errstr);

return false;
}

if ( ! ($this->assertResponse($socket, 220) ) ) {
return false;
}

fwrite($socket, "EHLO {$hostname}" . self::END_OF_LINE);
if ( ! ($this->assertResponse($socket, 250) ) ) {
return false;
}

fwrite($socket, "MAIL FROM: <{$fromEmail}>" . self::END_OF_LINE);
if ( ! ($this->assertResponse($socket, 250) ) ) {
return false;
}

fwrite($socket, "RCPT TO: <{$toEmail}>" . self::END_OF_LINE);
if ( ! ($this->assertResponse($socket, 250) ) ) {
return false;
}

fwrite($socket, 'QUIT' . self::END_OF_LINE);

fclose($socket);

return true;
}

/**
* @param resource $socket
* @param int $expectedCode
*
* @return bool
*/
private function assertResponse($socket, $expectedCode)
{
if ( ! is_resource($socket) ) {
return false;
}

$data = '';
while (substr($data, 3, 1) !== ' ') {
if ( ! ( $data = @fgets($socket, 256) ) ) {
$this->lastResponseCode = -1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you actually need this to be class level property? I haven't found this to be "reused", just overwritten. Making it local to the scope will prevent errors or unexpected manipulations from the outside.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, is needed property.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my comment was misleading. The variable is needed, that´s correct. However, instead of using a class property I think it can be a local (that is, declared and used within the method) var.
Using

function Bar {
  $lastResponseCode = null;
}

Instead of

class Foo {
  private $lastResponseCode;
  function Baz {
     $this->lastResponseCode...
  }
}


return false;
}
}

return ($this->lastResponseCode = intval( substr($data, 0, 3) )) === $expectedCode;
}
}
13 changes: 13 additions & 0 deletions EmailValidator/Warning/SocketWarning.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Egulias\EmailValidator\Warning;

class SocketWarning extends Warning
{
const CODE = 996;

public function __construct($hostname, $errno, $errstr)
{
$this->message = "Error connecting to {$hostname} ({$errno}) ({$errstr})";
}
}
40 changes: 40 additions & 0 deletions Tests/EmailValidator/Validation/MailboxCheckValidationTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace Egulias\Tests\EmailValidator\Validation;

use Egulias\EmailValidator\EmailLexer;
use Egulias\EmailValidator\Validation\Error\IllegalMailbox;
use Egulias\EmailValidator\Validation\MailboxCheckValidation;
use Egulias\EmailValidator\Warning\NoDNSMXRecord;
use PHPUnit\Framework\TestCase;

class MailboxCheckValidationTest extends TestCase
{
public function testValidMailbox()
{
$validation = new MailboxCheckValidation();
$this->assertTrue($validation->isValid('[email protected]', new EmailLexer()));
egulias marked this conversation as resolved.
Show resolved Hide resolved
}

public function testInvalidMailbox()
{
$validation = new MailboxCheckValidation();
$this->assertFalse($validation->isValid('[email protected]', new EmailLexer()));
}

public function testDNSWarnings()
{
$validation = new MailboxCheckValidation();
$expectedWarnings = [NoDNSMXRecord::CODE => new NoDNSMXRecord()];
$validation->isValid('[email protected]', new EmailLexer());
$this->assertEquals($expectedWarnings, $validation->getWarnings());
}

public function testIllegalMailboxError()
{
$validation = new MailboxCheckValidation();
$expectedError = new IllegalMailbox(550);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why where you expecting code 550? What does it means. This test is failing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

550 - it is smtp response code
mailbox unavailable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. Do you mind leaving a comment in the code please?

$validation->isValid('[email protected]', new EmailLexer());
$this->assertEquals($expectedError, $validation->getError());
}
}