Provide additional information to Mail class (original values) #31842
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
eileenmcnaughton
force-pushed
the
misc
branch
from
e525740 to
1fd8fbd
Compare
|
@eileenmcnaughton This looks fine. Have you tried extracting the BCC recipients yet... that's a whole other mess and would be really nice to see them separated out in the call to send() |
CRM/Utils/Mail.php
Outdated
| $originalValues = [ | ||
| 'html' => $params['html'] ?? NULL, | ||
| 'text' => $params['text'] ?? NULL, | ||
| 'attachments' => $params['attachments'] ?? [], |
There was a problem hiding this comment.
@eileenmcnaughton Please can we add in 'Bcc' => $headers['Bcc'] ?? NULL or similar here because of this crazyness: https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Mail.php#L202 which makes it really annoying to get the Bcc recipients back into the headers if they are required. You have to parse the headers, extract the email addresses and compare against the $to array! The missing ones then have to be added back in as Bcc.
At least that's the case for eg. Amazon SES via API.
There was a problem hiding this comment.
Note, we'd need to store the value before unsetting it as well.
There was a problem hiding this comment.
Oh, it might make sense to just add in Cc array keys here as well for completeness though we don't necessarily need it because we can find it in the headers. Would make it easier for anyone wanting to work with this in the future though.
There was a problem hiding this comment.
@mattwire I feel like if we add in Bcc we should add in to, cc & bcc (maybe from too) - but then the format becomes the question - from context it looks like we have already formatted the emails so an array like this probably makes sense (& invites the addition of 'from' too) - ie it's an array of strings where name & email are already formatted into a single string, if both exist
'to' => [
'[email protected]',
"Matt Wire <[email protected]">,
],
'cc' => ['[email protected]', "Matt Wire <[email protected]">],
'bcc' => ['[email protected]', "Matt Wire <[email protected]">],
There was a problem hiding this comment.
Hmm - I guess what you are saying is that ^^ would be no different to what is currently in 'headers'
There was a problem hiding this comment.
@mattwire I've added in 'bcc' - I'm on the fence about cc cos I feel like that would be cc, to and from once we get into the completeness question - which might make sense but I'm not sure.
Also, my read of the code is that Bcc would currently be a single email address so converting to an array through casting works - is that your take?
There was a problem hiding this comment.
Oh dang - it comes in as
"Adams, Clint" <[email protected]>,"[email protected]" <[email protected]>
I kinda hate not converting it to an array - but I don't think I can safely explode it
There was a problem hiding this comment.
Here is my workaround! https://lab.civicrm.org/extensions/ses/-/commit/64344002c28daf4f0f54abcf186c3b8f18bfb5e3
|
@eileenmcnaughton I think this is a great idea. But I've added a "feature request" that would help me too :-) |
eileenmcnaughton
force-pushed
the
misc
branch
from
1fd8fbd to
5652a02
Compare
eileenmcnaughton
force-pushed
the
misc
branch
from
5652a02 to
aa78658
Compare
|
@eileenmcnaughton I've made a suggested change. If you are happy with those I suggest you flag merge-ready |
|
@eileenmcnaughton I've hit a little spanner... see https://github.com/civicrm/civicrm-core/blob/master/ext/flexmailer/src/Listener/DefaultSender.php#L65 Ideally we'd add the extra data there two so we call mail() with the same signature. Also to make things more messy that one passes the first argument as a string (To) instead of array |
Co-authored-by: Matthew Wire <[email protected]>
Co-authored-by: Matthew Wire <[email protected]>
|
@mattwire I've merged your changes but I kinda hate passing bcc as a string - It feels gross. Re the flexmailing calling |
Overview
I'm working through replacing the mail library in an extension - https://github.com/eileenmcnaughton/symfony_mailer - although I think I'd like to see it changed in core - at the moment its hard to do that because our code interprets the html into
bodybefore it gets to the library - which wants to do it itselfWe do have VARIOUS hooks = but so far changing it out in
hook_civicrm_alterMailerseems to intervene in the most places / most reliably & other extensions seem to use itBefore
It is possible to entirely swap out the REALLY OLD Pear mail libraries - but if you do so the library does not receive adequate information about the bcc or the attachments
After
These extra parameters are passed
Technical Details
I think we should consider migrating to symfony_mailer in core given it seems to be winning the mailer adoption battle and move the old mail classes to an extension. But out of scope here
Comments