I would be happy to merge this, except the pipeline that's being added is failing. I haven't really worked with Gitlab CI before so I'm not sure what's wrong with it. If someone with more experience can investigate and fix it, I'm happy to re-review this.
This class was significantly rewritten in #3557052: MailManager isn't really a proper decorator, and it fails to discover attribute-based mail plugins → . Not sure if this issue still exists now.
This is done by Symfony Mailer Lite or Symfony Mailer module. The modules look for libraries called either symfony_mailer_lite or email, depending on which you are using.
zengenuity → changed the visibility of the branch 3556070-fix-crashes-when to active.
zengenuity → changed the visibility of the branch 3556070-fix-crashes-when to hidden.
Okay, I think we should change the module requirements to ^10.3.
zengenuity → made their first commit to this issue’s fork.
I merged the dependencies MR, but the pipeline is still failing.
zengenuity → made their first commit to this issue’s fork.
zengenuity → made their first commit to this issue’s fork.
The renderInNewContext() method is left over from an earlier implementation, and I don't think it's in use any longer. I've removed it from the module instead of upgrading it to use renderInIsolation().
I also switched to using DeprecationHelper in EmailController
It would be great to have someone test with my changes, and then mark as RTBC.
zengenuity → made their first commit to this issue’s fork.
I'm attempting to test this, but I can't seem to get even a minimal example working. Which branch should I be using? I'm currently trying with 3539651-email-content-yaml-plugin.
Here's what I've got. In a module called test_rig:
test_rig.email.yml
test_rig.test_notify:
label: 'Test Rig: Notify'
module: 'test_rig'
key: 'test_notify'
Then, I've got a controller I can call to trigger an email as a test.
<?php
declare(strict_types=1);
namespace Drupal\test_rig\Controller;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\Mailer\DefaultEmail;
/**
* Returns responses for Test Rig routes.
*/
final class TestRigController extends ControllerBase {
public function __construct(
protected readonly \Drupal\Core\Mailer\EmailFactoryInterface $emailFactory,
) {}
public function testNotify(): array {
$email = $this->emailFactory->create('test_rig.test_notify');
$body = [];
$body[] = $this->t('This is a test email from Test Rig module.');
$subject = $this->t('Test Email from Test Rig');
$params = DefaultEmail::createParams($subject, $body);
$email->to('wayne@zengenuity.com')
->langcode('en')
->params($params);
$result = $email->send();
$build['content'] = [
'#type' => 'item',
'#markup' => $this->t('It works!'),
];
return $build;
}
}
When I run this, I get:
Symfony\Component\Mime\Exception\LogicException: An email must have a "From" or a "Sender" header. in Symfony\Component\Mime\Message->ensureValidity() (line 132 of /userdata/dev/sites/core-dev/vendor/symfony/mime/Message.php).
So, it's not picking up the site mail as the default sender. But then, I also tried to add it to the $email object, and it's not clear how to do that. I don't see any methods on that object that would allow me to add headers. The only examples in the test code are for adding headers to the Symfony Email object in an alter.
I believe this form is one that was ported over from Swiftmailer, along with the restriction on selectable text formats. Regardless, it's not something I'm open to changing. If you've got a module on your site that is sending plain-text emails, there's a built-in protection there from XSS vulnerabilities and similar trickery from malicious user content. If this module changes that assumption and starts rendering potentially user-generated content as HTML, we open up a vector for a security issue. The sending module has no idea we're doing that, and may not be written to check for this sort of thing.
The best solution is for modules that want to send HTML emails to do so with a Content-Type: text/html header.
If it's not your own module, you can use hook_mail_alter() to change the Content-Type like this:
<?php
$message['headers']['Content-Type'] = 'text/html';
?>It's also possible to do it with a parameter when sending the email:
<?php
$params['content_type'] = 'text/html';
?>
Finally, as noted above, you could change the text format directly in hook_mail_alter():
<?php
$message['params']['text_format'] = 'your_filter_name';
?>But, if you're altering the messages like any of the last three suggestions, I highly recommend you limit it to specific emails that you know are safe. Don't globally add that to all emails sent from the website.
I'm going to close this as works as designed. The module's current options are the only safe approach for non-technical users. Technical users can implement any of the solutions above.
Security-related token evaluations are not saved in the log. If someone had access to create emails but not administer users, they could use the password reset functionality and email log to take control of other users' accounts. So, we purposely don't allow this.
Tokens that aren't security-related are replaced before being stored in the log. The list of "unsafe" tokens is in the code here: https://git.drupalcode.org/project/easy_email/-/blob/3.0.6/src/Service/E...
zengenuity → made their first commit to this issue’s fork.
Added First Class service back and committed. Thanks everyone.
zengenuity → made their first commit to this issue’s fork.
I moved your previous issue on this topic to the correct project for it: ✨ Add translations for default templates. Active (The templates are in the recipe used by Drupal CMS, not in this module.)
Unfortunately, no one has provided translations in the interim since you posted that issue. As I said in my comment in there, I'm not sure what the process is for getting the translations for the easy email templates into l.d.o. (It may be possible to ship translations with the recipe, as an alternative, but I'm not sure.)
In any case, I don't current maintain any multilingual sites, and I'm not proficient enough in other languages to do these translations. It sounds like you have translations for at least Spanish, so perhaps you could open a MR on the other issue to provide your translations.
Closing this as a duplicate. Future comments should be posted on ✨ Add translations for default templates. Active
@thekurt, your error is not the same as the issue here. You have a more general error with sending email from your environment. You should check the transport settings and make sure they are set up correctly for your environment.
You may find more detailed error messages about the nature of the transport problem in the error log.
@znerol, I've read through the latest MR, and it's pretty complex. I'm having some trouble following it from reading the code alone. I think we have a meeting scheduled later this week, so perhaps you could give us a walkthrough then of the classes and how you've broken up the responsibilities between the EmailBuilder, EmailRenderer, EmailManager, Email plugin instance class, and modulename.emails.yml definitions. I can understand the code class by class, line by line, but what I feel like I'm missing is the high-level overview of how things fit together and why.
Also, I think it would be helpful for review to have a minimal example module that sends a "Hello {{ user }}" HTML email. I see you have some examples in your test module, but I think those implementations could be more complex than the typical email because you're checking edge cases. An example of what we expect most developers to do would be useful for reviewing the DX of the new API.
Looking forward to discussing this further at our next meeting.
I've added code to enable and disable Easy Email Theme from the settings area of Easy Email module. And I hid Easy Email Theme in this issue: ✨ Hide Easy Email Theme from Appearance and Block layout pages Active
This has to be merged along with ✨ Hide Easy Email Theme from Appearance and Block layout pages Active , because once that's merged, we won't be able to access the theme logo settings.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
If we hide the theme on the Appearance page, how would you ever remove it if you wanted to? If we use hidden:true in the info file, it's also a problem that you can't enable it if you install it manually. (as opposed to getting it as part of Drupal CMS)
Using hidden:true hides it on both the Appearance page and on the Block Layout page, so I think it's the correct solution. But, we'd have to build an alternative way to install and uninstall it. I suppose we could detect that the theme is available and/or installed and provide an install/uninstall option within Easy Email's UI.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
zengenuity → created an issue.
Looking at this code example:
<?php
$email = $this->emailManager->getEmailBuilder()
->id('contact.' . $key_prefix . '_mail')
->param('message', $message)
->langcode($recipientLangcode)
->build() # returns a Symfony Email
$this->mailer->send($email->to(...$recipients)->replyTo($message->getSenderMail()));
?>This code suggests that mail ID, parameters, and langcode will not be available at the point the email is being sent. This will complicate the process of anyone attempting to alter the sending of the email, such as rerouting or logging it.
I general, I like the idea of declaring emails with a plugin like this. The YAML syntax is very straightforward and consistent with other plugins, and for more dynamic use cases, developers can use a deriver class. Having the emails formally declared opens up use cases that are difficult to deal with now. In Easy Email, I had to make my own plugin for emails and then declare the core emails myself in order to make them overridable. I think discoverability will be nice for other use cases like ECA, as well.
A couple of specific changes I propose we consider for the plugin definition.
1. Option for template instead of controller: I like the _controller option. It makes rendering an email very similar to rendering a route. But I think many emails could probably be rendered using only a template that would use the provided params as variables. In such cases, the controller is a boilerplate creation of a render array specifying the template. I propose that we allow developers to skip that boilerplate by adding a key to the plugin specification to specify a template instead of a controller. So the developer can choose one or the other. And if they choose neither, we can default to a template that is derived from the email's machine name. The implementation for this could be that we always call a controller, but there's a default controller that we write that renders a template from the email specification if no other controller is specified.
2. Typed data for parameters: In Easy Email, my specification includes data types for the parameters. This allows me to match up the parameters with fields in the email entities, such as entity references for users or Commerce orders. I don't think we should require developers to provide the types for their parameters, but it would be nice to support it if we are able to do so.
3. Consider whether we really should support closures: It's a neat idea, but I'm struggling to figure out the use case where it really helps. Yes, it can be nice to have the definition of the rendering function in the same place as the call site, but it seems like if we encourage this type of behavior, we make it harder for people to override the rendering of emails. In the case where we make everyone who declares an email provide a controller or use a default one that renders a template, that specification will be alterable by any developer who wants to provide their own controller instead. How would that work with a closure? The closure is being created at the same time as the call to send the email is being made, so it seems like it you won't be able to alter it ahead of time. You might be able to catch it before it's rendered, but that's an email by email alter rather than just altering the email specification for all emails of this type. So, I think we should think this one through a little more. Personally, I think a cleaner specification with clearer options for overriding is more important, but I'm open to changing my mind if someone can provide some concrete use cases where the closure is superior.
Regarding the rest of the code in the MR and related commits:
I feel that for a module to send an email should not be so complicated.
I agree with this sentiment. I think sending an email should be roughly as simple as it is now, which only requires calling a single method with module, key, params, etc. The way it's currently implemented here requires the developer to know more about how the email system works than they probably should have to know. I think if we don't support closures, this should be possible.
Next, I think that your code demonstrates the need for the Drupal adaptation layer that has been a controversial topic in our previous discussions. Here are a few key snippets:
<?php
$params += $definition->getDefaults();
$context = [
'id' => $definition->id(),
'module' => $definition->getModule(),
'key' => $definition->getKey(),
'langcode' => $langcode,
];
$this->moduleHandler->alter('email_params', $params, $context);
?><?php
$context = [
'id' => $definition->id(),
'module' => $definition->getModule(),
'key' => $definition->getKey(),
'langcode' => $langcode,
'email' => $email,
] + ($build['#props'] ?? []);
unset($build['#props']);
$this->moduleHandler->alter('email_build', $build, $context);
?><?php
$context['metadata'] = BubbleableMetadata::createFromRenderArray($build);
unset($context['email']);
$this->moduleHandler->alter('email_post_render', $email, $context);
?>These are alter hooks where you're passing around multiple objects. The specification for what is in the params and context changes a bit per call, but it's clear that passing around the params alone or the Symfony email object alone is not enough. If I'm developer, I have to understand which different types of objects I'm going to get in each of these alters, and I have to know what is expected to be in context at each point, as it changes. The Drupal adaptation layer would provide a single object that we could pass around that would encapsulate the context, params, and potentially the final Symfony email. It would simplify the API that developers have to interact with at each point.
Moreover, I think we should consider using the adaptation layer object in place of the initial params array. For example, most email call site code in the current API looks something like this:
<?php
$params = [
'user' => $some_account,
'order' => $commerce_order,
// ...
];
$this->mailManager->mail('commerce_order', 'order_receipt', $to, $langcode, $params, $actually_send);
?>The call site code in your commits is a bit different, but much of the internal code is passing around these same variables. With the adaptation layer, I think we could get to something like this:
<?php
$email = $this->mailManager->new('commerce_order.order_receipt')
->setTo($to)
->addParam('user', $some_account)
->addParam('order', $commerce_order)
->setLangcode($langcode)
->setSend($actually_send);
$this->mailManager->mail($email);
?>I think the latter is easier to read and more discoverable, as the context variables will have specific methods that can be autocompleted in IDEs, found by AI code agents, etc. And now we have one object we can pass around internally, though alter hooks or event subscribers, until we finally get to the point of actually sending the email, where we convert it into a Symfony Email. (We potentially could also have a Symfony Email embedded it that is being updated by the parent object all the time so there's no conversion process at the end. I don't have a strong opinion about that.)
Those are my initial thoughts on the MR and other commits. I think it's a good start on layers 5 and 6, as currently defined in the proposed resolution in 🌱 Requirements and strategy for Core Mailer Active , but I think it would benefit from us discussing layers 3 and 4 in more detail, which is where my comments above are focused. But overall, I'm feeling optimistic. After seeing this work and talking to @adamps last week, I feel like I'm starting to see the outlines of a workable solution. I'm looking forward to discussing this in more detail at our next meeting.
znerol → credited zengenuity → .
I discovered this in 10.5.1, but I see that the code is the same in 11.2.x branch, so I'm moving the issue to that branch.
zengenuity → created an issue.
znerol → credited zengenuity → .
Other than during installation, are there any other times when the theme system is unavailable? Seems unlikely to me, but I'm not sure.
There are already open issues in Drupal CMS and core to prevent cron from running during install. The resolution of those issues should fix the Easy Email issue, as well.
🐛
Installation fails if cron runs in parallel
Active
🐛
Installation fails if cron runs in parallel
Active