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.

Sign up for GitHub

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 events to transactional email sending #42

Closed
wants to merge 1 commit into from
Closed

Add events to transactional email sending #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions app/code/core/Mage/Core/Helper/Mail.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php

class Mage_Core_Helper_Mail extends Mage_Core_Helper_Abstract
{
/**
* Return a mailer instance to be used by the Mage_Core_Model_Email_Template
*
* The used factory helper and method can be configured at global/email/factory_helper.
* Default is Mage_Core_Helper_Mail::getMailer.
*
* @return Zend_Mail
*/
public function getMailer()
{
return new Zend_Mail('utf-8');
}
}
55 changes: 39 additions & 16 deletions app/code/core/Mage/Core/Model/Email/Template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class Mage_Core_Model_Email_Template extends Mage_Core_Model_Template
* Configuration path for default email templates
*/
const XML_PATH_TEMPLATE_EMAIL = 'global/template/email';
const XML_PATH_MAIL_FACTORY = 'global/email/factory_helper';
const XML_PATH_SENDING_SET_RETURN_PATH = 'system/smtp/set_return_path';
const XML_PATH_SENDING_RETURN_PATH_EMAIL = 'system/smtp/return_path_email';
const XML_PATH_DESIGN_EMAIL_LOGO = 'design/email/logo';
Expand Down Expand Up @@ -140,11 +141,20 @@ protected function _getLogoAlt($store)
/**
* Retrieve mail object instance
*
* Configure the factory helper at global/email/factory_helper using the format class::method.
* Default is Mage_Core_Helper_Mail::getMailer.
*
* @return Zend_Mail
*/
protected function _getMail()
{
return new Zend_Mail('utf-8');
$factory = Mage::getConfig()->getNode(self::XML_PATH_MAIL_FACTORY);
$parts = explode('::', (string) $factory);
if (count($parts) != 2) {
throw new Exception(sprintf('Failed to get mail factory class and method.'));
}
$mailer = call_user_func(array(Mage::helper($parts[0]), $parts[1]));
return $mailer;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

/**
Expand Down Expand Up @@ -349,25 +359,31 @@ public function getProcessedTemplate(array $variables = array())
$variables['this'] = $this;
}

if (isset($variables['subscriber']) && ($variables['subscriber'] instanceof Mage_Newsletter_Model_Subscriber)) {
$processor->setStoreId($variables['subscriber']->getStoreId());
}

if (!isset($variables['logo_url'])) {
$variables['logo_url'] = $this->_getLogoUrl($processor->getStoreId());
}
if (!isset($variables['logo_alt'])) {
$variables['logo_alt'] = $this->_getLogoAlt($processor->getStoreId());
}

$processor->setIncludeProcessor(array($this, 'getInclude'))
->setVariables($variables);
$processor->setIncludeProcessor(array($this, 'getInclude'));

$this->_applyDesignConfig();
$storeId = $this->getDesignConfig()->getStore();
try {
$processedResult = $processor->setStoreId($storeId)
->filter($this->getPreparedTemplateText());
$processor->setStoreId($storeId);
$transport = new Varien_Object(array(
'variables' => new Varien_Object($variables),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have applied this change, but slightly refactored this part: instead of creating an object, I pass the original array and then reassign it after event is dispatched.

And in the code below the magic methods of "transport" are not used, because it is knowingly Varien_Object which has no such method. So why spend additional execution time on invoking it?

UPD:
Actually, introducing the event specifically in this way results in a logical error. Supposedly, the event is to override store_id, template text and variables. But the _applyDesignConfig() has already been performed with an old store ID -- so at first we should dispatch event, reassign the variables from transport and only then "emulate design". Otherwise, in current implementation, the event would affect only "logo_url" and "logo_alt" variables below.
I have refactored this part as well, so now event will be invoked before "emulating" and rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, Anton.

I used a Varien_Object for the variables array because that enables the use of nice chained calls like if ($observer->getEvent()->getTransport()->getVariables()->getSubscriber()) in event observers, but regarding the overhead, you are right of course.
This variation is more work for the developer bus less performance overhead during execution:

$variables = $observer->getEvent()->getTransport()->getVariables();
if (isset($variables['subscriber']) && $variables['subscriber']) {

A brief explanation regarding the event placement: I added the event in this way, because this is how the setting of the store_id of the newsletter/subscriber currently works in Magento 1.
Currently it only affects the "logo_url" and "logo_alt" variables. The _applyDesignConfig() processing isn't affected because the store_id is only set on the processor instance, not the core/design_package.
I wondered about that, and I'm glad you agree this is a bug.

'template_text' => $this->getPreparedTemplateText(),
'store_id' => $storeId
));
// Use observer to modify variables and template processor settings
Mage::dispatchEvent('email_template_filter_before', array(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an unwritten convention among core developers to have module name prefix in the event names. So I have changed this to core_email_template_filter_before and core_email_template_send_before below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, I think that convention makes a lot of sense.

'processor' => $processor,
'transport' => $transport
));
if (!isset($variables['logo_url'])) {
$variables['logo_url'] = $this->_getLogoUrl($transport->getStoreId());
}
if (!isset($variables['logo_alt'])) {
$variables['logo_alt'] = $this->_getLogoAlt($transport->getStoreId());
}

$processedResult = $processor->setVariables($transport->getVariables()->getData())
->filter($transport->getTemplateText());
} catch (Exception $e) {
$this->_cancelDesignConfig();
throw $e;
Expand Down Expand Up @@ -486,6 +502,13 @@ public function send($email, $name = null, array $variables = array())
$result = false;
$this->_sendingException = null;
try {
// Note: the email body already has been processed, changes to variables will have no effect
Mage::dispatchEvent('email_template_send_before', array(
'mailer' => $mail,
'variables' => new Varien_Object($variables),
'template' => $this,
'store_id' => $this->getDesignConfig()->getStore()
));
$mail->send();
$result = true;
} catch (Exception $e) {
Expand Down
8 changes: 7 additions & 1 deletion app/code/core/Mage/Core/etc/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@
<page_type>
<render_inherited>0</render_inherited>
</page_type>
<design_fallback>
<allow_map_update>1</allow_map_update>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change relevant to the contribution topic? I don't think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this was a mistake, sorry.

</design_fallback>
</dev>
<email>
<factory_helper>Mage_Core_Helper_Mail::getMailer</factory_helper>
</email>
</global>
<frontend>
<routers>
Expand Down Expand Up @@ -291,7 +297,7 @@
</web>
<admin>
<startup>
<page>dashboard</page>
<menu_item_id>dashboard</menu_item_id>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looks like an accidental change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

</startup>
<url>
<use_custom>0</use_custom>
Expand Down
19 changes: 19 additions & 0 deletions app/code/core/Mage/Newsletter/Model/Observer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,23 @@ public function scheduledSend($schedule)

$collection->walk('sendPerSubscriber', array($countOfSubscritions));
}

/**
* Set the subscriber store id on the transport object
*
* Set the subscriber store id on the transport object so the logo configuration for the
* subscriber store is read (which is not necessarily the current one).
*
* @param Varien_Object $observer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will be the Varien_Object, but you need to realize that it will be instance of Varien_Event_Observer. And in the method body it is used incorrectly: the event data should be obtained through Varien_Event_Observer::getEvent(), so that's how you'd get transport:

$transport = $observer->getEvent()->getTransport();

Technically your solution would work though. If you inspect the Mage_Core_Model_App::dispatchEvent() implementation, you'll find this:

$observer->addData($args);

It is contradictory to the concept of event subscriber and potentially can lead to a logical error, if the $args happen to have the same key as something within $observer. We could not remove this in M1 because of backwards-compatibility requirements and actually this issue is not addressed yet.

The bottom line: use $observer->getEvent()->getSomething() instead of $observer->getSomething(). We should remove this legacy logic in M2 at some point. I have fixed this particular occurrence while applying the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification.
In Magento 1 I always use Varien_Event_Observer, but I saw some changes in Magento 2 in that regard. Some observers (like in Mage_Backend) don't use any type for the argument, and most of the observers in Magento 2 return the observer instance $this. WIll this be changed again?

Personally I second deprecating the event instance wrapper for the event parameters :)

* @return Mage_Newsletter_Model_Observer
*/
public function emailTemplateFilterBefore($observer)
{
// Only match newsletter subscriber events
$subscriber = $observer->getTransport()->getVariables()->getSubscriber();
if ($subscriber && $subscriber instanceof Mage_Newsletter_Model_Subscriber) {
$observer->getTransport()->setStoreId($subscriber->getStoreId());
}
return $this;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to return a value here. Removed from the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment. Most observers in Magento 2 return the observer instance, so I thought that might be the new recommended practice.

Thanks again for all your detailed comments, I highly appreciate it!

}
}
10 changes: 10 additions & 0 deletions app/code/core/Mage/Newsletter/etc/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@
<newsletter>
<tempate_filter>Mage_Newsletter_Model_Template_Filter</tempate_filter>
</newsletter>
<events>
<email_template_filter_before>
<observers>
<newsletter>
<class>Mage_Newsletter_Model_Observer</class>
<method>emailTemplateFilterBefore</method>
</newsletter>
</observers>
</email_template_filter_before>
</events>
</global>
<adminhtml>
<events>
Expand Down