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 FallbackMailer #28

Closed
wants to merge 1 commit into from
Closed

Conversation

@JanTvrdik
Copy link
Contributor

JanTvrdik commented May 20, 2016

Simple mailer which can retry sending mail multiple times with either one ore multiple mailers.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented May 20, 2016

Some open questions:

  • Should the total number of retries by $retryCount or $retryCount * count($this->mailers)?
  • Should we log the exceptions? I don't know how to do it without depending on Tracy\ILogger. Or should we just emit warnings?
@dg

This comment has been minimized.

Copy link
Member

dg commented May 20, 2016

To throw, to log is task for a diffent layer.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented May 20, 2016

Poetic but I don't understand what you mean. Does that mean that I should move the FallbackMailer to Nextras because it cannot be implemented in Nette?

@dg

This comment has been minimized.

Copy link
Member

dg commented May 20, 2016

I tried to say that FallbackMailer should not log exceptions. It is not its responsibility. It can provide some callback, but not log them.

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:pr/fallback_mailer branch 3 times, most recently from fbf7221 to f8fb0d5 May 20, 2016
*/
public function send(Message $mail)
{
for ($i = 0; $i < $this->retryCount; $i++) {

This comment has been minimized.

Copy link
@milo

milo May 21, 2016

Member

What about to check, that mailers is not empty?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 23, 2016

Author Contributor

Good point, I'll add the check in constructor.

}
}

throw $e;

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 23, 2016

Author Contributor

self note: wrap this with another SendException instance

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Jun 5, 2016

This is ready to merge from my point of view.

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:pr/fallback_mailer branch from 1f06b15 to c0bb198 Jun 5, 2016
/**
* @param int
*/
public function setRetryCount($retryCount)

This comment has been minimized.

Copy link
@dg

dg Jun 6, 2016

Member

All setters in nette should return $this. But even better is to omit anything that is not really needed. I think that except addMailer() is not need nothing. And with addMailer() should be if (!$mailers) { throw ... tested later.

@dg dg closed this in 22d3ee5 Jun 25, 2016
@JanTvrdik JanTvrdik deleted the JanTvrdik:pr/fallback_mailer branch Jun 27, 2016
@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 27, 2016

Wow. Good job. 👍


I was not sure it could be part of nette/mail package. I use for every project on my local devstack something that I called FileMailer. Do you think it could be also part of package?

Implementation is really small and cool.

<?php
use Nette\Mail\IMailer;
use Nette\Mail\Message;

final class FileMailer implements IMailer
{

    /** @var string */
    private $path;

    /**
     * @param string $path
     */
    public function __construct($path)
    {
        $this->path = $path;
    }

    /**
     * @param Message $mail
     */
    public function send(Message $mail)
    {
        @mkdir($path, 0777, TRUE);
        file_put_contents($this->path . date('Y-m-d H-i-s') . microtime() . '.eml', $mail->generateMessage());
    }

}

Should I prepare PR with tests? @JanTvrdik @dg

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Jun 27, 2016

@f3l1x What do you use it for?

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 27, 2016

Dump message to folder, open in editor, check design especially for newsletter, verify all EML options/requirements (html vs plain) etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.