Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core#644 - extract function to return correct mailbox header #13407

Merged
merged 2 commits into from Jan 7, 2019
Merged

core#644 - extract function to return correct mailbox header #13407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a718097
core#644 - extract function to return correct mailbox header
MegaphoneJon Jan 5, 2019
4335f22
rename function and vars; move fix to common mail sending function
MegaphoneJon Jan 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 1 addition & 7 deletions CRM/Contact/Form/Task/EmailCommon.php
View file
Open in desktop
Expand Up @@ -400,13 +400,7 @@ public static function submit(&$form, $formValues) {
// dev/core#357 User Emails are keyed by their id so that the Signature is able to be added
// If we have had a contact email used here the value returned from the line above will be the
// numerical key where as $from for use in the sendEmail in Activity needs to be of format of "To Name" <toemailaddress>
if (is_numeric($from)) {
$result = civicrm_api3('Email', 'get', [
'id' => $from,
'return' => ['contact_id.display_name', 'email'],
]);
$from = '"' . $result['values'][$from]['contact_id.display_name'] . '" <' . $result['values'][$from]['email'] . '>';
}
$from = CRM_Utils_Mail::formatFromAddress($from);
$subject = $formValues['subject'];

// CRM-13378: Append CC and BCC information at the end of Activity Details and format cc and bcc fields
Expand Down
5 changes: 5 additions & 0 deletions CRM/Core/BAO/MessageTemplate.php
View file
Open in desktop
Expand Up @@ -389,6 +389,11 @@ public static function sendTemplate($params) {

CRM_Utils_Hook::alterMailParams($params, 'messageTemplate');

// Core#644 - handle contact ID passed as "From".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lcdservices That makes sense. If you submit a PR I'll review it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MegaphoneJon here you go: #13776

if (isset($params['from'])) {
$params['from'] = CRM_Utils_Mail::formatFromAddress($params['from']);
}

if ((!$params['groupName'] ||
!$params['valueName']
) &&
Expand Down
22 changes: 22 additions & 0 deletions CRM/Utils/Mail.php
View file
Open in desktop
Expand Up @@ -577,4 +577,26 @@ public static function format($fields) {
return $formattedEmail;
}

/**
* When passed a value, returns the value if it's non-numeric.
* If it's numeric, look up the display name and email of the corresponding
* contact ID in RFC822 format.
*
* @param string $from
* contact ID or formatted "From address", eg. 12 or "Fred Bloggs" <fred@example.org>
* @return string
* The RFC822-formatted email header (display name + address)
*/
public static function formatFromAddress($from) {
if (is_numeric($from)) {
$result = civicrm_api3('Email', 'get', [
'id' => $from,
'return' => ['contact_id.display_name', 'email'],
'sequential' => 1,
])['values'][0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will give a PHP notice if the id is not found

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@monishdeb monishdeb Jan 7, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$from = '"' . $result['contact_id.display_name'] . '" <' . $result['email'] . '>';
}
return $from;
}

}