Closed (fixed)
Project:
Mailer Plus (DSM+)
Version:
2.x-dev
Component:
User interface
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
20 Sep 2023 at 18:12 UTC
Updated:
13 Jun 2025 at 18:29 UTC
Jump to comment: Most recent
Comments
Comment #2
djg_tram commentedFrom 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:
2) AFAIK it isn't possible to configure sender in the GUI in other similar modules
Comment #3
djg_tram commentedWhile 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.
Comment #4
sokru commentedOne 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.
Comment #5
adamps commentedThe 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?
Comment #6
sokru commentedCreated an issue for #4: #3392544: Default sender address should take langcode into account
Comment #7
marioki commentedEdit: misunderstanding
Comment #10
longwaveI 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.
Comment #11
pvbergen commentedI 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?
Comment #12
pvbergen commentedComment #13
longwaveI suppose a third option is to add a checkbox to
FromEmailAdjusterwhich also sets the sender?Comment #14
adamps commentedFor 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.
Comment #17
rodrigoaguileraI 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?
Comment #18
adamps commentedThanks.
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 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 enhanceBaseEmail::setAddress()also to accept sender. Then we don't have to override theAddressAdjusterBase::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 toEmail::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.
Comment #19
adamps commentedI'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.
BaseEmail::$addressesinitialiser and remove separate$sendervariableBaseEmail::setSender/getSenderto usesetAddress/getAddress. Add a check in setAddress() to assert/throw if$name == sender && count($addresses) > 1.Email::getSymfonyEmail()remove the call to$this->inner->sender()that is outside the loop and instead add it back inside the loop withif ($name == sender) {...} else {}.assertAddress().Now senderEmailAdjuster doesn't need to override build().
Thanks again
Comment #20
rodrigoaguileraMuch clear now
I rebased the branch and committed the refactoring following your guidance. Thanks!
Comment #26
adamps commentedGreat thanks