Problem/Motivation

Please, allow the modification of Sender in the Mailer settings. While we can change it when actually sending the message from code, we can't do so on the settings page.

Command icon 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

djg_tram created an issue. See original summary.

djg_tram’s picture

From https://www.drupal.org/project/symfony_mailer/issues/3388098 :

AdamPS: However I have my doubts as

1) The description for the site mail field says this:

The From address in automated emails sent during registration and new password requests, and other notifications. (Use an address ending in your site's domain to help prevent this email being flagged as spam.)

2) AFAIK it isn't possible to configure sender in the GUI in other similar modules

djg_tram’s picture

While item 1 has its merits, I don't think item 2 would be of any importance. :-)

Actually, the code allows it, so I can't really see any reason not to allow from the UI as well.

sokru’s picture

One use case is to use the translated site_name configuration. The code https://git.drupalcode.org/project/symfony_mailer/-/blob/1.x/src/Address... hardcodes the Sender header as default language Site name.

adamps’s picture

The Sender should come from site configuration. If you want to change it, set "site mail" in the Basic site settings - that's exactly what that setting is for, clearly documented in the description. So why is another setting needed?

#4 I agree is a bug. This issue GUI setting could be a workaround. The correct fix is a code change to use the correct translation (fix the code) - please can you raise a separate issue?

sokru’s picture

marioki’s picture

Edit: misunderstanding

longwave made their first commit to this issue’s fork.

longwave’s picture

Version: 1.3.1 » 1.x-dev
Status: Active » Needs review

I have a requirement to set a different sender for certain emails, so I needed to be able to change this via policy. I added a simple plugin to custom code to do so, I've ported it here as part of Symfony Mailer itself if this is useful.

pvbergen’s picture

I created a similar issue in #3419179: Add EmailAdjuster to use primary from as sender.
I also add a MailAdjuster, which automatically takes the primary from address and sets it as a sender.

I would propose to combine the plugins to provide more configuration:
- Take the primary from address or
- Manually set a sender email.

Or should we add separate adjusters?

pvbergen’s picture

longwave’s picture

I suppose a third option is to add a checkbox to FromEmailAdjuster which also sets the sender?

adamps’s picture

Status: Needs review » Needs work

For me, this solution is a clearer UX than #3419179: Add EmailAdjuster to use primary from as sender.

1) If do this we need to provide a clear warning in the UI, for example in a description text. Firstly normally don't use this, set the site address instead. Secondly make sure to use a valid domain so as to avoid getting classed as spam - we could copy the wording from core.

2) Message.php will anyway add a sender based on From. So all we need to do is set the sender to blank. This ensures that it copies the final from - rather than the current value which might later be altered by a hook.

3) For a new feature, please add a test.

rodrigoaguilera made their first commit to this issue’s fork.

rodrigoaguilera’s picture

Version: 1.x-dev » 2.x-dev
Status: Needs work » Needs review
Issue tags: +ddd2025

I adjusted the code for 2.x. Added the description text copied from Drupal core and tests.

I don't understand point 2 from comment #14. Can you explain more what is needed?

adamps’s picture

Status: Needs review » Needs work

Thanks.

1) #14.1 has two parts and you did the second part. Please also add something like this: 'The sender defaults to the site email address configured in "Basic site settings". You should only use this adjuster if you need a different value.' It's becoming quite long, so I suggest a separate form field, something like $form['warning'] = XXX (this also saves duplicating the original description).

I don't understand point 2 from comment #14

I also find it confusing😃. I believe I had intended #14 be a reply to #13: add an checkbox to FromEmailAdjuster to also set the sender. I was concerned about the case that both "from" and "sender" are present, and someone changes one but forgets the other. My point was that if this option is set, then we could delete the default sender and the value would be copied from 'from'.

Now that you have written SenderEmailAdjuster I'm willing to stick with it. However build() is wrong - it misses 2 of the 3 cases in the parent class. Potentially a neat solution is to enhance BaseEmail::setAddress() also to accept sender. Then we don't have to override the AddressAdjusterBase::build() method at all, and it allows removing various bits of special case code in BaseEmail (and assertSender() can call assertAddress()). It requires a small adjustment to Email::getSymfonyEmail() - inside the loop we check for sender, and do something like $this->inner->sender($value[0]). If you make a start then I can add suggestions as needed.

OR if you are enthusiastic to switch to the alternative solution, add a checkbox to FromEmailAdjuster then I can instead comment about that.

adamps’s picture

I've thought some more and I like SenderEmailAdjuster. It's consistent with the others (else who knows to look for sender inside from), and it means that if a site actually needs from and sender different for some reason then they can.

Expanding on the previous comment, I believe the key to making an adjuster for Sender is to make it work like the other addresses.

  1. Add sender to BaseEmail::$addresses initialiser and remove separate $sender variable
  2. Rewrite BaseEmail::setSender/getSender to use setAddress/getAddress. Add a check in setAddress() to assert/throw if $name == sender && count($addresses) > 1.
  3. In Email::getSymfonyEmail() remove the call to $this->inner->sender() that is outside the loop and instead add it back inside the loop with if ($name == sender) {...} else {}.
  4. MailerTestTrait::assertSender() can now be written in terms of assertAddress().

Now senderEmailAdjuster doesn't need to override build().

Thanks again

rodrigoaguilera’s picture

Status: Needs work » Needs review

Much clear now

I rebased the branch and committed the refactoring following your guidance. Thanks!

rodrigoaguilera changed the visibility of the branch 3388651-allow-customize-sender to hidden.

rodrigoaguilera changed the visibility of the branch 3388651-allow-customize-sender-2.x to hidden.

rodrigoaguilera changed the visibility of the branch 3388651-allow-customize-sender-2.x to active.

rodrigoaguilera changed the visibility of the branch symfony_mailer-3388651-3388651-allow-customize-sender-2.x to hidden.

adamps’s picture

Status: Needs review » Fixed

Great thanks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.