Add events to transactional email sending

[Vinai]

The purpose of the events is to enable modules to

• add attachments
• change email templates
• modify template variables
• add template variables
• modify email content

Also, the patch makes the mailer class factory method configurable.
The purpose is to enable modules to use a diferent mailer without
reverting to a rewrite of Mage_Core_Model_Email_Template.

Additionaly the undeclared dependency on Mage_Newsletter was
removed from Mage_Core_Model_Email_Template, and an event observer
added to the newsletter module taking care the business logic
is not changed.

[speedupmate]

excellent suggestion :) I have rewritten/extended this "Mage_Core_Model_Email_Template" class many time just to be able to change the encoding of the e-mail.

throw in "email_template_send_after" event also? could be useful to hook some followup (logging, CRM flags etc) actions after mail is sent?

[Vinai]

Thought about that, but never had a use case so far. If you had them I think thats a good idea.

[speedupmate]

had few CRM requests and also monitoring/logging related requests that I have previously solved by rewriting/extending and if there is before why not add after :) good initiative from you again.

[colinmollenhour]

The mailer factory is a great feature to add. Just to add some context, I've rewritten before so that I can add some special headers to the Zend_Mail object for use with email services like SendGrid and Mandrill which have features like tagging, url open/click tracking, automatically adding text versions to html-only emails, etc.. So having access to the Zend_Mail object before it is sent with context info like which template is being rendered is helpful indeed.

The only improvement I'd suggest is some way to get the template config name into the template object. Currently, if the user has selected a custom template the database id is passed instead so there is no easy way to find out which template is being actually being sent without doing some sort of reverse lookup of all possible config values. To fix this I think sendTransactional should take a config path rather than the config value. Of course all uses of sendTransactional would need to be updated accordingly.

[Vinai]

Thank you for your comments, Colin.
So far I have envisioned people using
$templateId = $observer->getTemplate()->getTemplateId(); if ($templateId == Mage::getStoreConfig(Mage_Newsletter_Model_Subscriber::XML_PATH_SUCCESS_EMAIL_TEMPLATE))

To implement the change you suggest (which would make such checks much nicer), every time a transactional email is sent the template config path would have to be specified instead of the template id.
I suppose that would be okay, since Magento 2 is breaking bc anyway.
But can you see a better way to add the config path information?

[colinmollenhour]

Magento 1.6 code:

        $email->sendTransactional(
            Mage::getStoreConfig(self::XML_PATH_CONFIRM_EMAIL_TEMPLATE),
            Mage::getStoreConfig(self::XML_PATH_CONFIRM_EMAIL_IDENTITY),
            $this->getEmail(),
            $this->getName(),
            array('subscriber'=>$this)
        );

Proposed Magento 2 code:

        $email->sendTransactional(
            self::XML_PATH_CONFIRM_EMAIL_TEMPLATE,
            self::XML_PATH_CONFIRM_EMAIL_IDENTITY,
            $this->getEmail(),
            $this->getName(),
            array('subscriber'=>$this)
        );

The latter is much better IMO because people (even core devs!) sometimes neglect to specify the store id to Mage::getStoreConfig resulting in cases where the wrong config is used (e.g. cron job sends email but template is set at store scope). The "is_numeric($templateId)" check in sendTransactional could remain in case someone is saving template ids somewhere besides the config, and the $sender could be checked to see if it contains a "/" to determine if it is a config path vs an identity (e.g. "sales"). So it would actually retain bc, but the new standard would be to specify the the config paths only.

Edit:
With the above proposal, an event observer might look like this:

$template = $observer->getTemplate()->getTemplateConfigPath();
if ($template == Mage_Newsletter_Model_Subscriber::XML_PATH_SUCCESS_EMAIL_TEMPLATE) { .. }

[Vinai]

Thanks again for the input. The store id should be specified as an argument, too, since it might be dependent on the template variables, for example the subscriber store id, or it needs to be set on the $email instance. Otherwise, how would the correct store id be identified when the config paths are resolved?

$email->sendTransactional(
    self::XML_PATH_CONFIRM_EMAIL_TEMPLATE,
    self::XML_PATH_CONFIRM_EMAIL_IDENTITY,
    $this->getStoreId(),
    $this->getEmail(),
    $this->getName(),
    array('subscriber'=>$this)
);

or

$email->setStoreId($this->getStoreId())
    ->sendTransactional(
        self::XML_PATH_CONFIRM_EMAIL_TEMPLATE,
        self::XML_PATH_CONFIRM_EMAIL_IDENTITY,
        $this->getEmail(),
        $this->getName(),
        array('subscriber'=>$this)
    );

I personally prefer the first way, because it is too easy to miss specifying the store id to use otherwise.

[Vinai]

I'll implement the suggestions and update the pull request, some time soon (weekend?).

[antonmakarenko]

As far as I remember, this method was added specifically for purpose of rewriting/mocking this class. Well, it turns out to be not the most flexible solution.
Instead of introducing more complexity to it, I'd recommend to look at it from different angle:

• Have the mailer instance in the state of this object (represented in the _mailer attribute) with "lazy initialization" getter. And implement setter.
• Or rather pass the mailer as argument to the send() method

Both approaches supposedly will give more flexibility and remove necessity to rewrite the class. Besides, such changes can be covered by a unit test (unlike the proposed solution which is too coupled to application instance, hence integration tests)

[Vinai]

Thank you, Anton. I prefer your solution.
Now one thing missing is a hook to allow global mailer injection even if the first call to the (lazy loading) factory method is from within a core class.
Usually some very early event like controller_front_init_before be used for that, except that this one is only fired if the routing process is dispatched. cron scripts (for sending newsletters for example) would not be affected.
Is there a method for mailer injection that affects all instances you would suggest? Maybe a new event is needed, like core_email_get_instance_before?

[magento-team]

Vinai, thanks for the contribution. Your patch has been partially applied (with modifications, as mentioned in the code review).
Would you like to contribute the _getMail() change as a separate pull request? You could practice unit/integration tests when implementing it -- that would encourage the best implementation.

[magento-team]

After reviewing and discussing the changes of this contribution, we have concluded that it would be inexpedient to apply it right now without a substantial refactoring. We need to address design issues in the classes responsible for templates rendering/sending, before extending functionality.

So refactoring has been added as a task to the "backlog". There we plan to address design issues and accommodate the original request to be able to customize sending emails without having to override the framework classes.