Add Events around Zend_Email

[rjocoleman]

Current we have several ways of sending email - Email, Template, Newsletter and Queue.

There are no events around any of these ways of sending email.
There is no way to send to authenticated SMTP servers.

Currently only way to send via SMTP is to rewrite core via third party extensions - there are many extensions that do this (paid, free, open-source etc). These extensions often conflict with each other and are sometimes bundled with packages.

Events are useful for a number of use-cases, for example:

1. Using a different Transport (SMTP, API)
2. Logging of outgoing emails
3. Manipulating emails before they're sent (such as automatic attachments)

Configuration in System Configuration -> Advanced -> System -> Mail Sending Settings includes support for Host and Port but these configuration elements have comments saying they're "for Windows servers only". These configuration values are used for PHP's ini_set method to configure PHP's mail extension.

Zend_Mail handles mail sending, which supports Transports to take care of the actual delivery. However no transport is configured, which defaults to passing to PHP's mail() function that tries to use sendmail installed on Linux servers (or SMTP on windows). Having sendmail installed and configured on Linux servers is often not possible these days especially in shared hosting environments.

This PR:

1. Adds the following events (which should cover every instance of Zend_Mail):

• email_send_before / email_send_after
• email_queue_send_before / email_queue_send_after
• email_template_send_before / email_template_send_after
• newsletter_send_before / newsletter_send_after

2. Uses utf-8 for Mage_Core_Model_Email (utf-8 is used for every other email type but this one seems to have been forgotten).

3. Instantiates a Zend_Mail_Transport and passes this to the relevant before event. This transport is used for sending if it is set by the event.

This allows developers to hook these events and set transports or manipulate the Zend_Mail object without rewriting core.

Nothing is done with these events - they're merely dispatched.

Fixed Issues (if relevant)

1. Fixes Make the email encoding of Zend_Email configurable #1366 (use utf-8 for Zend_Mail)

Manual testing scenarios (*)

1. I have written an extension to use a SMTP transport for each of the email sending models via the before events https://github.com/icecubenz-open-mage/Icecube_CustomEmailServer (this also includes example code for the after events). This extension provides a way to configure a SMTP server then send a test email from each model via system configuration.

Questions or comments

I have modelled these events after the popular SMTP Pro Extension,

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[henryhayes]

@rjocoleman I don't understand the point of these pourtions of code:

$transport = new Varien_Object();

and

if ($transport->getTransport()) {
    $mail->send($transport->getTransport());
} else {
    $mail->send();
}

It seems redundant. You're always setting $transport = new Varien_Object(); on line 136, meaning it will always be set as an empty Varien_Object.

Could you elaborate please?

Are we asuming that the $transport object will be set or modified by an observer?

[rjocoleman]

Zend_Mail handles mail sending, which supports Transports to take care of the actual delivery. However no transport is configured, which defaults to passing to PHP's mail() function that tries to use sendmail installed on Linux servers (or SMTP on windows). Having sendmail installed and configured on Linux servers is often not possible these days especially in shared hosting environments.

@henryhayes

Could you elaborate please?
Are we asuming that the $transport object will be set or modified by an observer?

The answer to this is both yes (if a merchant chooses to) and no (backwards compatible) - to elaborate:

Context, the standard email handling is using ZF's Zend_Mail which supports transports.
Transports are not used and the current handling tries to use php's mail() which is not super useful for many.

In many cases there are hosting level work arounds or modules like Aschroder_SMTPPro used (and many similar), which completely overwrite the mail sending classes. They are often a source of conflicts. I have seen a lot of these, they all work in pretty much the same way. They also are often bundled with other extensions.

As a solution, in this PR the flow is:

1. $transport = new Varien_Object(); create a new empty object called $transport
2. Dispatch an event that includes this transport and the other mail objects.
3. When it comes to sending the mail - if there's an actual transport on the $transport object use it, if not don't (i.e current behaviour).

This allows observers, using the standard Mage observer pattern, act on the dispatched event e.g. email_queue_send_before and set a transport if they so desire.

The if statement you've quoted is essentially defence / backward compatibility - to use the transport or not.

This should be completely backwards compatible.

To further illustrate my point:

I have written a proof of concept module that does just this here: https://github.com/icecubenz-open-mage/Icecube_CustomEmailServer

This module has a walk through of email in Magento 1/OpenMage in the readme that explains several of the issues: https://github.com/icecubenz-open-mage/Icecube_CustomEmailServer#history

In short, this module:

1. Set a bunch of SMTP related settings in system config
2. Observes email_queue_send_before (and others)
3. If it's enabled, and SMTP settings are set, use this SMTP transport to send the mail rather than mail()

This exact method can be applied to any other mail capable transport e.g. HTTP APIs such as Sendgrid, SES etc (some of these already exist for Zend_Mail)

This PR can be used with our without the linked module, the linked module is simply an example of it in usage.
As an aside, I have been using this patch and module in production for around 10 months (and sent around 300k emails over that time).

Mail via SMTP (and similar) aren't the only use-cases, the same system of observers in this way also work for adding attachments. A common use case in third party modules is adding PDF attachments with packing slips, invoices etc.

[henryhayes]

Excellent contribution with outstanding concept example of usage by @rjocoleman in the module: https://github.com/icecubenz-open-mage/Icecube_CustomEmailServer

Code review accepted.

[henryhayes]

Mail via SMTP (and similar) aren't the only use-cases, the same system of observers in this way also work for adding attachments. A common use case in third party modules is adding PDF attachments with packing slips, invoices etc.

A +1 from @henryhayes on this one. It's absolutely essential and should have been part of the original Magento 1 core.

[luigifab]

It will not work when developers use directly new Zend_Mail().
But this is a good addition (ok, not tested, I using an overload of Zend_Mail).

Can you use [] instead of array() and update EVENTS.md?

[fballiano]

@rjocoleman would it be possible for you to fix the conflicts and implement the changes requested by @luigifab?

[rjocoleman]

@fballiano this is done now - thanks!

[sreichel]

Cant merge b/c conflicts :(

[rjocoleman]

@sreichel it's just EVENTS.md I'll rebase it if you want, but that will remove all the approvals again..

[sreichel]

@sreichel it's just EVENTS.md I'll rebase it if you want, but that will remove all the approvals again..

EVENTS.md moved to docs/ ...

With saying "i cant merge" i mean github does not let me merge it until conflicts are resolved. Yep, it needs to be approved again ... :(

[rjocoleman]

@sreichel rebased. thanks

[addison74]

Do not merge it yet. I would like to get one more approval from a maintainer.

[luigifab]

Don't forget to update numbers.

[fballiano]

I'll merge and update right after, just not to invalidate the reviews

[fballiano]

merged and cherry picked to v20

also updated EVENTS.MD with commit 816248f which also has been cherrypicked to v20