core#644 - extract function to return correct mailbox header #13407
Conversation
|
(Standard links)
|
|
@MegaphoneJon question do we need to apply similar fixes in CRM/Member/Form/Membership.php and CRM/Contribute/Form/AdditionalInfo.php and similar or maybe CRM/Contribute/Form/Task/Invoice.php ? |
|
ping @mattwire |
| 'id' => $header, | ||
| 'return' => ['contact_id.display_name', 'email'], | ||
| 'sequential' => 1, | ||
| ])['values'][0]; |
There was a problem hiding this comment.
This will give a PHP notice if the id is not found
There was a problem hiding this comment.
That's probably a good thing though, right? The only legitimate use case in which this code path executes would require the id to be found. Or are you suggesting I should use getsingle so it throws an error rather than a notice?
There was a problem hiding this comment.
The old and this new code pretends that is_numeric($from) will always be a valid email ID, so it doesn't need any additional handling for empty result. Or I could have suggested:
$result = CRM_Utils_Array::value(0, civicrm_api3('Email', 'get', [
'id' => $from,
'return' => ['contact_id.display_name', 'email'],
'sequential' => 1,
])['values']);
if (empty($result)) {
Civi::log()->warning(ts('Invalid From email ID - ' . $from);
}
...I think we can ignore this.
I think so, also looks like it would be an issue in |
|
I have attached the patches and the e-mail addresses are correct now. |
I'll patch these files too - but I'm curious how you two found these functions? And whether there are other places I should check? |
|
@MegaphoneJon knowing that you can send receipts from the backend contrib forms (membership renewal membership signup contribution forms etc) I just started looking for similar places that might also be susceptible |
|
Thanks @seamuslee001. These all call |
|
Jenkins re test this please |
|
@MegaphoneJon @mattwire I managed to replicate the issue in a unit test so posted it here #13408 and was able to confirm this fixes the problem and outputs a sensible from email address. |
|
I have confirmed that this PR fixes the issue. @seamuslee001 isn't #13408 is supposed to fail without this patch? |
|
Cool. Based on @mattwire @seamuslee001 and my review I am going to merge this PR. Also it has UT in PR #13408 (I will trigger the test build again to ensure that the patch fixes the test failure and thus this bug). Thanks! |
|
#13408 test build passed now :) |
Reverse commit e28a209 Add a patch for civicrm/civicrm-core#13407 This is CiviCRM's proposal to fix dev/core#758
|
@aydun has asked if we can put the in tomorrow's 5.11.0 release -it looks like it's not already in? |
| @@ -389,6 +389,11 @@ public static function sendTemplate($params) { | |||
|
|
|||
| CRM_Utils_Hook::alterMailParams($params, 'messageTemplate'); | |||
|
|
|||
| // Core#644 - handle contact ID passed as "From". | |||
There was a problem hiding this comment.
I know this commit has already been merged, but I feel like this block where the FROM address is "fixed" should be before the alterMailParams hook is called. otherwise we're sending bad data to that hook and then fixing it.
There was a problem hiding this comment.
@lcdservices That makes sense. If you submit a PR I'll review it?
There was a problem hiding this comment.
@MegaphoneJon here you go: #13776
Overview
Core#357 identified an issue where emails sent with a "From" address of the logged-in user send the contact ID as the "From" address instead of the correct "From" address. This PR extracts the fix to
CRM_Utils_Mail, cleans it up a bit for readability, and applies it to membership renewal notifications.Before
Membership renewal notifications sent a "From" address that's the contact ID of the logged-in user.
After
Membership renewal notifications send the logged-in user's actual name/email.