When sending an email, only the store mail address is included in the From header in commerce_store_mail_alter. Why not use a slightly more fancy From header that also includes the store name?
- $message['from'] = $current_store->getEmail();
+ $message['from'] = $current_store->label() . "<" . $current_store->getEmail() . ">";
This will make email programs display the name of the store as the sender instead of the email address.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3121168-store_name_in_mail_header_with_method-17.patch | 8.26 KB | neograph734 |
| #17 | 3121168-store_name_in_mail_header-17.patch | 7.1 KB | neograph734 |
Comments
Comment #2
neograph734Comment #4
neograph734Updated the tests as well.
Comment #6
neograph734Comment #7
neograph734I think that adding a space between the name and address would make it even better.
Test <test@example.com>instead ofTest<test@example.com>Comment #8
longwaveThis doesn't work for me for the order receipt mail - that sets the From address separately, so the check in
commerce_store_mail_alter()skips the change.Comment #9
neograph734You are right. I've attached a new patch that also updates the order receipt mail and add note to the maintainers to consider removing the from header there and leave everything to
commerce_store_mail_alter()instead.That however, would be a change in the mail id, and could be considered a backwards compatibility breaking change.
Comment #10
longwaveThe ID isn't the problem - the commerce mail handler prefixes all the IDs with "commerce_" already - it's just that "from" is already set.
Comment #11
neograph734That was some thoughtless copy-pasting. This one looks better.
Comment #12
neograph734Ah thanks, I was not aware of that. Yet I think the note is still partially relevant as the whole order receipt mail handler should be able to work without the From as well. Why not delegate that to
commerce_store_mail_alter()?Comment #14
neograph734Last attempt and otherwise I will have a look again after the weekend.
This patch removes the defined From header from OrderReceiptMail. It was no different from the default behavior, but was unnecessarily defining it in a second place. This keeps all processing in one place. Also great that there is no BC issue.
Comment #15
longwaveYeah, I think removing the "from" from OrderReceiptMail is the way to go.
However commerce_store_mail_alter() assumes the "current store" is the correct one, and that might not be the case, where e.g. an admin is resending an order receipt - it would be better if commerce_store_mail_alter() checked $params for an order and used the store relating to that order, falling back to the current store otherwise.
Comment #16
neograph734Well I did not think of that before, but I bet that is the very reason why it was explicitly set...
Would it? For stock commerce maybe, but the commerce infrastructure is bigger and I can imagine (did not check) the same mail handler being used by commerce_wishlist or commerce_invoice as well. In such cases you will not have an order but instead a wishlist or invoice. That would flaw the logic. Instead, the store could be provided to the mail handler. But that would make commerce_store_mail_alter() rather useless.
Comment #17
neograph734I've reverted the removal of 'from' from #14. I believe this to be the safest version for now until there is a better alternative.
The alternative version of the patch adds a method to the Store entity to generate the 'from' header in a centralized place.
Comment #18
lisastreeter commentedShould the subject be translatable?
Current patch uses
sprintf:+ 'from' => sprintf('%s <%s>', $order->getStore()->getName(), $order->getStore()->getEmail()),Issue #2924159 Provide a way to customize the order receipt email subject has a patch that includes update to the From header:
Comment #19
neograph734That should not make a difference right? The t() function does not translate placeholders, so the effect is the same?
Comment #20
lisastreeter commented@Neograph734 Re #19
Good point! Thanks, wasn't thinking of how t() works. Comment #18 can be ignored :)
Comment #21
neograph734Thanks for the confirmation Lisa :).
Have you (or any of the other people subscribed to this thread) tested the patch? Would be great if somebody could RTBC this so we can get it into a release.
Comment #22
lisastreeter commentedI was waiting until I had this in production to comment. I can confirm that patch #17 is working properly.
Comment #25
jsacksick commentedI went with the extra method, so we don't have to repeat the same logic in multiple places, thanks everyone!