genkgo / mail

Library to send e-mails over different transports and protocols (like SMTP and IMAP) using immutable messages and streams. Also includes SMTP server.
https://mail.readthedocs.io/
Other
402 stars 21 forks source link

Too many constructors #24

Closed mpyw closed 7 years ago

mpyw commented 7 years ago

If we

we need to write many constructors.

$message = $message
    ->withHeader(new From(new Address(new EmailAddress('mur@example.com', '三浦')))
    ->withHeader(new Subject('Hello World'))
    ->withHeader(new To(new AddressList([
        new Address(new EmailAddress('kmr@example.com'), '木村'),
        new Address(new EmailAddress('szk@example.com'), '鈴木'),
    ])));

So more static helper methods should be provided on From and To.

$message = $message
    ->withHeader(From::fromEmailAddressAndName('mur@example.com', '三浦'))
    ->withHeader(new Subject('Hello World'))
    ->withHeader(To::fromMultipleRecipients([['kmr@example.com', '木村'], ['szk@example.com', '鈴木']]));

Deeply structured classes have powerful robustness, however, they seem like Java way. It's not suitable for PHP. So I suggest providing more casual interfaces.

mpyw commented 7 years ago

I think Guzzle interfaces have good balance between casualness and robustness.

Request Options — Guzzle Documentation

frederikbosch commented 7 years ago

Deeply structured classes have powerful robustness, however, they seem like Java way. It's not suitable for PHP. So I suggest providing more casual interfaces.

I cannot agree on that statement. Because it looks like Java it must be bad? Why is it not suitable? Says who? However, I do understand what you mean by saying there should be more static constructors.

In your comment you give two suggestions.

// current
->withHeader(new From(new Address(new EmailAddress('mur@example.com', '三浦')))
// suggested
->withHeader(From::fromEmailAddressAndName('mur@example.com', '三浦'))

That is saving almost nothing. So I think we should not include that suggestion.

However, this is saving a lot.

// current
$list = [['kmr@example.com', '木村'], ['szk@example.com', '鈴木']];
foreach ($list as [$address, $name]) {
   $message = $message->withHeader(new From(new Address(new EmailAddress($address), $name)))
}
// suggested, changed fromMultipleRecipients to fromList
->withHeader(To::fromList([['kmr@example.com', '木村'], ['szk@example.com', '鈴木']]));

The last suggestion also fixes the first one, because then you can write.

// suggestion 2
->withHeader(To::fromList([['kmr@example.com', '木村']]));

// suggestion 1
->withHeader(From::fromEmailAddressAndName('mur@example.com', '三浦'))
// current
->withHeader(new From(new Address(new EmailAddress('mur@example.com', '三浦')))

So, I would suggest that you add a PR for the fromList method. Would you be able to do that?

mpyw commented 7 years ago

fromList sounds good. But I alternatively suggest fromEmailAddress receive second argument.

->withHeader(From::fromEmailAddress('mur@example.com', '三浦'))
frederikbosch commented 7 years ago

Merged.