Add mailbox validator

[IvanLutokhin]

No description provided.

[egulias]

Hi @IvanLutokhin ,
really appreciate your PR. I did an exhaustive review, please look at the comments. Let me know any doubt or further comment on the review.
Great work!

[egulias]

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.

[IvanLutokhin]

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.

[egulias]

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?

[egulias]

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.

[IvanLutokhin]

Yes, is needed property.

[egulias]

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...
  }
}

[egulias]

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

[IvanLutokhin]

550 - it is smtp response code
mailbox unavailable

[egulias]

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

[IvanLutokhin]

Thanks for exhaustive review.
I try to fix all issue and comments it.

[egulias]

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?

[egulias]

Thanks for the changes @IvanLutokhin . I have done a couple of comments more. Please also check the test failure for PHP 5.5.

[IvanLutokhin]

Yes, I added getter for property responseCode in InvalidMailbox and resolve magic method __toString()
I think that's enough and interface is not necessary.

I fixed unit test for php 5.5 also. The problem was in deprecated methods phpunit.

[egulias]

Apparently there´s an error for hhvm within Travis, hence the failure. But the rest is passing.

[egulias]

@IvanLutokhin regarding the exception, __toString is a potential solution. However this would be the only error exception that has a getResponseCode method so if someone wants to do something with it the only way would be to test against the class name of the returned error. By providing an specific type that would be easier.
It will also allow you to provide a wider range of error responses and codes following the table you shared, which would be great to have.
Happy to find a solution together!
Thanks again for the time you are investing into the feature!

[IvanLutokhin]

Apparently there´s an error for hhvm within Travis, hence the failure. But the rest is passing.

HHVM does not support composer anymore.
https://hhvm.com/blog/2019/02/11/hhvm-4.0.0.html

As one of the solutions to fix the version HHVM 3.30.

[egulias]

Thanks @IvanLutokhin , I'll do that. I'll be working to merge the PR shortly. Thanks for your patience.

[osavchenko]

@IvanLutokhin do you need any help on this PR?

[clemlatz]

@egulias @IvanLutokhin I'm interested in the feature.
Any reasons why this PR hasn't been merged? Anything I can do to help?