Allow preserving sender and DSN in DefaultsEmailAdjuster

Thanks for the issue and patch.

Unfortunately there is a conflict with ✨ Enhancements to builder and adjuster annotations Fixed , which makes the DefaultsEmailAdjuster run always, without any presence in the policy GUI. This was done because several people had accidentally deleted it, and then raised bug reports.

However I think this issue can still work. DefaultsEmailAdjuster is intended to set defaults😃. So I think it's OK to skip setting a field if it's already been set - doing it always without having a 'preserve' setting.

ok. should I rework this patch to always check for existing values or should I try to contribute to ✨ Enhancements to builder and adjuster annotations Fixed instead?

Please rework this patch to always check for existing values, thanks.

Sorry I've thought some more and I've got some concerns.

In the IS:

in our use case we need to have user-selectable outgoing addresses

This module already has a GUI form for user-selectable values for any email setting, which is PolicyEditForm. It has some features/advantages:

1. As an important security feature, it blocks arbitrary DSNs, because otherwise it's possible to run any command using Sendmail with a custom command setting.
2. It exposes address fields including From, but not Sender, as in my understanding Sender should be set by site policy and needs to be an authorised address.

So perhaps the ideal solution to your situation is this: ✨ Enhancements to Email Policy embedding Active (item 3).

1. Embed PolicyEditForm in your form, probably there would be a new helper MailerHelper::renderTemporaryPolicy().
2. On submit, it would give a MailerPolicyInterface.
3. Pass that when you send the email: EmailFactory::newTypedMail($type, $subtype, $policy)

What do you think?

thank for the input.

Our workflow is a bit more complicated and involves the usage of queues.

• User entities have a reference to trasnsportDsn
• Users can create specific content (that in turn will trigger sending emails)
• A cronjob gathers all pending mail content entities and prepares them for delivery by gathering the trasnsportDsn referenced in the entity-creator (user) and passes information on to a queue
• Dedicated queue creates emails with gathered information and sends the mails off

I don't see the benefit in making the Email interface extra hard to use programmatically and enforcing the GUI.

As for your concern regarding arbitrary / rouge transportDsn: What if instead we would add a check into the Email object making sure the given transportDsn is a valid (existing) one? (though I do have to admit that if someone already has his hands in at that level - being able to execute / manipulate php - I don't see the bonus of this extra added security.

I don't see the benefit in making the Email interface extra hard to use programmatically and enforcing the GUI.

I'm not forcing you to use the GUI, it was just a suggestion. My job as maintainer is to balance the needs of the whole community. What's "hard to use" for one case might be helping another developer from making a mistake.

If you are programmatically setting email fields, then the recommended way is within an EmailBuilder. This would avoid the problem entirely as your code would then run after DefaultsEmailAdjuster. If the DSN is stored on the user entity, then you could write this: newEntityEmail($type, $subtype, $user). Does that solve your problem?

though I do have to admit that if someone already has his hands in at that level - being able to execute / manipulate php - I don't see the bonus of this extra added security.

What you do on your site is of course up to you. As maintainer, the situation I intend to avoid is this: a Drupal user, even a full admin, gaining ability through the GUI to run an arbitrary system (shell) command with privilege of the web user. On that basis, any form field for entering a DSN is a risk. DsnTransport::validateConfigurationForm() has code to protect against this. On the other hand, if the DSN comes from code, we have to trust it, as obviously in code you can already do anything.

What if instead we would add a check into the Email object making sure the given transportDsn is a valid

Interesting idea, thank you for suggesting it. We'd need to figure out how to treat a DSN as trusted if generated from code.

My job as maintainer is to balance the needs of the whole community. What's "hard to use" for one case might be helping another developer from making a mistake.

I agree on that. Than again being able to call Drupal\symfony_mailer\Email::setTransportDs without safeguard about it being replaced was what triggered me writing this patch in first place. So in a sense, we already are in the situation your described and as an active community member I do also seek to improve modules for everyone.

If you are programmatically setting email fields, then the recommended way is within an EmailBuilder

Good to know. What I did after poking around for a bit was to take a look at what the test-form does and create an new email object by calling Drupal\symfony_mailer\EmailFactoryInterface::newTypedEmail. I do still think that this should be the way to go as it is actually very transparent and would give module developers a very nice interface to work with emails (especially in comparison to the standard drupal way of handling mails).

a Drupal user, even a full admin, gaining ability through the GUI to run an arbitrary system (shell) command with privilege of the web user

full ack again. however I do fail to see this being possible due to the suggested changes: sender and dsn not being overridden.

Interesting idea, thank you for suggesting it. We'd need to figure out how to treat a DSN as trusted if generated from code.

In case I managed to introduce ambiguity: All used DSN are added through the symfony_mailer form and are only referenced by users. I did not create an extra place of DSN origin nor do I have the feeling that "bring you own DSN" would be a good thing. I'm merely selecting the DSN by the user who initiated the mail creation.

ps.: thanks for taking your time. hope we find a nice solution.

Thanks for a lot of extra detail, which makes many things clearer. Your system is a fascinating example of how this module can be used in a way I had never imagined.

Good to know. What I did after poking around for a bit was to take a look at what the test-form does

Fair point, and many apologies, it wasn't actually a good example, see 📌 TestEmailBuilder should follow guidelines for writing an email builder Fixed . My recommendation is documented here → . I agree it shouldn't be mandatory to comply, but also you might be more likely to hit bugs like this if not doing so😃, and you might miss out on some future features.

FYI reasons for the recommendation include: clean architecture allowing overriding; control of the order things happen in (specifically relevant here); render context switching - see comment in Mailer::send() which you might wish to read; potential for future features such as retrying the send or handling sending on another thread (the work to do is encapsulated entirely in the parameters).

(especially in comparison to the standard drupal way of handling mails)

Actually I think it's the opposite - in Drupal Core it's mandatory to pass $params for the email to be built within hook_mail() which corresponds to the EmailBuilder, likely for some of the same reasons I listed above.

All used DSN are added through the symfony_mailer form and are only referenced by users

Good idea.

hope we find a nice solution.

If you still prefer not to use an EmailBuilder then you are welcome to go ahead as discussed in #6.

I raised 📌 Emphasis correct usage of EmailFactoryInterface Active and 📌 Safeguard transport DSN Fixed