Extending \Swift_TransportException

[romaricdrigon]

Hi,

Closes #27 No BC break!

[hkdobrev]

@romaricdrigon Thanks for this! I love it when there's no BC breaks!

However you need to add the $previous exception to the constructor signature to preserve BC and adhere to the Liskov substitution principle. You should pass the all three arguments to the parent exception: https://github.com/swiftmailer/swiftmailer/blob/5.x/lib/classes/Swift/TransportException.php

I will reopen the issue and set this PR to close it when merged using https://help.github.com/articles/closing-issues-via-commit-messages/.

[hkdobrev]

@romaricdrigon Actually if you don't override the constructor of the exception altogether, it will all work. There is no need to set the code explicitly when this is done by the parent.

[romaricdrigon]

Actually that's a trap, but SwiftMailer exceptions don't handle nor the code not the previous one. I'm adding it.

[hkdobrev]

@romaricdrigon But they are - https://github.com/swiftmailer/swiftmailer/blob/5.x/lib/classes/Swift/SwiftException.php The Switft_SwiftException extends Exception and passes the code and previous exception. And then the built-in PHP exception class kicks in - http://php.net/manual/en/class.exception.php

[romaricdrigon]

Check Swift_TransportException, they are not!

The code has to be set then. I c

[hkdobrev]

@romaricdrigon I can actually merge just your first commit as it's everything that's needed.

[romaricdrigon]

(end) I will keep the previous in the signature, but I can't set it it will be lost :(

[hkdobrev]

I'm not sure I follow you, Swift_TransportException is passing the previous argument to its parent and the whole chain of exception classes in SwiftMailer is doing that. https://github.com/swiftmailer/swiftmailer/blob/5.x/lib/classes/Swift/TransportException.php#L25-L28

[hkdobrev]

This is happening in the PHP built-in Exception class:

$this->code = $code;
$this->previous = $previous;

[romaricdrigon]

The constructor signature changed in 5.4.1 (semver...) - before the 2 other arguments were lost: https://github.com/swiftmailer/swiftmailer/blob/v5.4.0/lib/classes/Swift/SwiftException.php

\Exception::previous is private I believe. The best I can come with without requiring 5.4.1 at least is:

    public function __construct($message = '', $code = 0, Exception $previous = null)
    {
        parent::__construct($message);

        // Before SwiftMailer 5.4.1, Swift_TransportException does not handle code and preivous
        \Exception::__construct($message, $code, $previous);
    }

[hkdobrev]

@romaricdrigon Thanks for this! I didn't realize they have changed that only recently. It's really bad they have done it so late.

What do you think of bumping the minimum SwiftMailer version to 5.4.1 and handling the exceptions nicely? I mean if one wants to use the Swift_TransportException would need to update to the newest Swiftmailer when they will actually get nicer SwiftMailer exceptions altogether.

And of course previous versions of this postmark library would still work with older versions of SwiftMailer.

[romaricdrigon]

Bumping required SwiftMailer version is up to you. That looks like a big bump and maybe some BC, I'm not sure semver is really respected there. Because then we would be requiring the latest stable version.

On the other hand, with the previous snippet Exceptions will have all their properties set correctly.

[romaricdrigon]

Actually PHP silently drops inexistent method parameters. So to achieve maximum compatibility we could do this:

    public function __construct($message = '', $code = 0, Exception $previous = null)
    {
        // Before SwiftMailer 5.4.1, Swift_TransportException does not handle code and previous
        // So we call grandparent to be sure everything is set
        \Exception::__construct($message, $code, $previous);

        parent::__construct($message, $code, $previous);
    }

For SwiftMailer < 5.4.1, calling the Exception constructor makes sure everything is set. For SwiftMailer >= 5.4.1, code and previous are given and then can be used.

[hkdobrev]

@romaricdrigon I'm not really sure what this does exactly:

\Exception::__construct($message, $code, $previous);

Does it call the Exception constructor, but in the context of $this?

[romaricdrigon]

Exactly. Visibility in PHP is at class, not instance, level.

[hkdobrev]

@romaricdrigon

What do you think of that?

public function __construct($message = '', $code = 0, Exception $previous = null)
{
    parent::__construct($message, $code, $previous);

    // In SwiftMailer <5.4.1, Swift_SwiftException does not conform to built-in Exception signature
    $this->code = $code;
}

If people use SwiftMailer <5.4.1, they will suffer from the previous problem anyway and if they have upgraded everything would be working correctly across Postmark and SwiftMailer exceptions.

[romaricdrigon]

This issue I see with that approach is that we overwrite the code, this we override any logic that may be in constructors. Nothing huge probably for real use case.

Le vendredi 9 octobre 2015, Haralan Dobrev notifications@github.com a écrit :

@romaricdrigon https://github.com/romaricdrigon

What do you think of that?

public function construct($message = '', $code = 0, Exception $previous = null){ parent::construct($message, $code, $previous); // In SwiftMailer <5.4.1, Swift_SwiftException does not conform to built-in Exception signature $this->code = $code;}

If people use SwiftMailer <5.4.1, they will suffer from the previous problem anyway and if they have upgraded everything would be working correctly across Postmark and SwiftMailer exceptions.

— Reply to this email directly or view it on GitHub https://github.com/OpenBuildings/postmark/pull/28#issuecomment-146845733 .

Romaric Drigon

[hkdobrev]

@romaricdrigon Thanks for everything! I have added the changes I've suggested in a following commit: https://github.com/OpenBuildings/postmark/commit/8909fc0512346e11267c21ce95b4bb0112db31fd and this is released in 0.3.3. Cheers! :beers:

[romaricdrigon]

My pleasure, thanks for this really useful library :)