Problem/Motivation
When creating a custom email if I want to use replacement tokens with the setVariables() method this information is not stored in the queue or applied in the sent email.
Steps to reproduce
Create a custom policy ex:
type: test
sub_type: queue
Create a body like this:
{{ test }}
Create an email like this:
$this->emailFactory->newTypedEmail('test', 'queue')
->setVariable('test','Custom text')
->send();
If you don't add the queue to the policy the email sent will have the text "Custom text"
If you add the queue element to the policy the email will have the text "{{ test }}"
Proposed resolution
Add the variables to the SymfonyMailerQueueItem class
And on the SymfonyMailerQueueWorker class when attempting to send the email
// Attempt to send the email. Considered done when successfully delivered.
$email = $this->emailFactory->newTypedEmail(
$item->type,
$item->subType,
...$item->params,
);
Add the variables if they exist, something like:
if ($item->variables) {
$email->setVariables($item->variables);
}
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | add_addresses-3472047-15.patch | 2.78 KB | robbertnl |
| #9 | no_email_variables-3472047-9.patch | 4.35 KB | styrbaek |
Issue fork symfony_mailer_queue-3472047
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
kallado commentedComment #3
kallado commentedTo add the variables to the SymfonyMailerQueueItem I think it shoul be somewere in QueueSendingEmailAdjuster
probably here:
Where we could use the Email method getVariables() to retrieve the values
Comment #4
kallado commentedComment #5
simonbaeseThanks for the report. I am traveling right now. Will be able to get to the issue next week.
Comment #6
kallado commented@simonbaese Thanks I'm available to help if you need to just a lot of work at the moment to dive into this.
Comment #7
simonbaeseI checked the issue today. Building emails programmatically does not work correctly, as reported. Unfortunately, this applies not only to variables but also to other email properties. The issue stems from the Drupal Symfony Mailer overloading the responsibilities of the email object. Hence, the email object can not be serialized easily to pass data to the queue worker.
Probably, the only way forward is to carry all the build properties to the queue worker and reinstate them again. This approach is slightly ugly and I will sleep on it.
A workaround for now is to use an email builder plugin and delegate the assignment of variables to it.
Bumping to major priority because this bug affects all emails built in the described way.
Comment #8
simonbaeseI am sorry for the delay. This issue is more complicated than expected because of the upstream architecture. If anyone has a good idea of how to approach this issue, please add your comments here.
Comment #9
styrbaekThis patch adds support for email variables in the symfony_mailer_queue module.
The changes include:
1. Adding $email->getVariables() in QueueSendingEmailAdjuster::build() method to ensure variables are stored in the queue
2. Adding 'variables' parameter to the SymfonyMailerQueueItem constructor with corresponding documentation
3. Implementing setVariables() in the processItem() method in SymfonyMailerQueueWorker
4. Defining the SYMFONY_MAILER_QUEUE_NAME constant and updating the QueueWorker annotation
These changes resolve the issue with missing email variables when emails are sent through the queue system, ensuring that all email data is properly transferred and processed.
Comment #11
simonbaeseThe issues mentioned in previous comments remain. The idea behind the Symfony Mailer module is to utilize email builder and adjuster plugins to customize the email being sent. This is possible with the existing plugins or through custom plugins.
Yet, in theory, it is possible to manipulate the
Emailobject before it is sent. For example, one could do the following in the business logic.Just in this short snippet, we have three different cases:
setFrom()is part of theBaseEmailInterface. Yet, this call is not respected by the Symfony Mailer module. One should use theFromEmailAdjusterthrough configuration instead.setTheme()is part of the Symfony MailerEmailInterface. In theory, it may make sense to adjust the theme through code, and the Symfony Mailer module would respect it. Yet, it is better to use theThemeEmailAdjusterthrough configuration instead.As previously mentioned, supporting all methods on the email object would require object serialization for the queue. Additionally, one would need to reconsider how emails are placed in the queue and then reinitialized once they are sent. I would refrain from making this change, as it is contrary to the intent of the Symfony Mailer module.
So, for now, let's add support for adding variables. Can you please review and test the merge request? Note that the email adjuster or builders may set variables. That is why we need to merge the variables.
Comment #12
simonbaeseOne more comment: We will start marking the
SymfonyMailerQueueIteminternal. I suspect that other issues will come up. Currently, changing the queue item is a breaking change, and we must tag a new minor release. I want to avoid that in the future.Comment #13
robbertnl commented@simonbaese Great work. I see variables are added now. But same issue applies for Addresses. I am setting those from code (e.g. email->setTo()). Now sending fails because of this
Comment #14
robbertnl commented@simonbaese maybe add something like this patch for the addresses. But maybe its easier to just add the emailobject in the item object?
Comment #15
robbertnl commentedI added a new patch (#14 had a typo) against the MR for adding (bcc/to/cc/from) email addresses.
Comment #16
simonbaese@robbertnl This is precisely what I described earlier. Unfortunately, the Drupal Symfony Mailer email object is overloaded in its responsibilities and is not easily serializable.
I am biting the bullet here in an attempt to carry over most of the properties that could be set during the email build phase. From a code perspective, this solution is not the most beautiful. But other solutions might be even uglier.
Can you please review the merge request and test the changes in your setup?
Comment #17
simonbaeseComment #18
robbertnl commented@simonbaese, I understand what you mean regarding all the properties. I can't think of a better solution right now either.
But I can confirm that the merge request works in my setup as well and solves the issue
Comment #19
simonbaeseComment #21
simonbaese