Fix values passed to tokenValues hook #16623
Conversation
|
(Standard links)
|
de2bce0 to
6d4f895
Compare
|
Betty and I reviewing PRs and we came across this one. @mattwire when this is ready for review can you let us know and can you also let u know how we could test this? Is there any place in civicrm where tokens did not work and work after this change? |
CRM/Mailing/BAO/Mailing.php
Outdated
| if (!$contact || is_a($contact, 'CRM_Core_Error')) { | ||
| CRM_Core_Error::debug_log_message(ts('CiviMail will not send email to a non-existent contact: %1', | ||
| [1 => $contactId] | ||
| )); |
There was a problem hiding this comment.
Any reason to remove this error? It helps in identfying the problem when an invalid $contactId is sent to retrieve the token values. We also use this elsewhere in the code - https://github.com/civicrm/civicrm-core/blob/master/Civi/Token/TokenCompatSubscriber.php#L64
There was a problem hiding this comment.
@jitendrapurohit No we can leave that error in - I've added it back in.
There was a problem hiding this comment.
Hmm, feels something is wrong here. $contact after this PR is in the form of [ contact id => details] which is expected by tokenValues() hook. But if you look down in this file, all the handling is done based on $contact[details] variable. Eg https://github.com/civicrm/civicrm-core/blob/master/CRM/Mailing/BAO/Mailing.php#L1160 which might break after this gets merged?
There was a problem hiding this comment.
@jitendrapurohit You are right - I've updated the PR and moved the call to tokenValues up above the is_a.. error check so we only do the reset on the array once - but we do it after the call to tokenValues - so I think this is ok now?
| @@ -88,12 +88,9 @@ public function onEvaluate(TokenValueEvent $e) { | |||
| $contact = array_merge($contact, $row->context['tmpTokenParams']); | |||
| } | |||
|
|
|||
| $contactArray = !is_array($contactId) ? [$contactId => $contact] : $contact; | |||
There was a problem hiding this comment.
I don't see any reason for $contactId to arrive in array format here so this looks fine to me. Objections if any @eileenmcnaughton @totten ?
There was a problem hiding this comment.
All noted issues are addressed. This should be good to merge now. Thanks @mattwire
|
Merging based on @jitendrapurohit review |
Overview
Originally identified by @MegaphoneJon in the emailapi extension. Also ping @magnolia61 https://lab.civicrm.org/extensions/emailapi/-/merge_requests/6
There are places in CiviCRM core that are not calling this hook correctly (and not in accordance with the documentation). Depending on the "route" through core this leads to missing tokens or unexpected data.
It has been documented correctly for some time:
See: https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_tokenValues/
Before
Some calls from core not using correct parameters for
hook_civicrm_tokenValuesAfter
All core calls using correct (documented) parameters for
hook_civicrm_tokenValues.Technical Details
Parameter 1 should be an array of keyed contactID => detail.
Parameter 2 should always be an array of contact IDs.
Comments
There is a chance that this will cause some breakage to custom integrations if they are using custom tokens and this hook. But they are likely already experiencing breakage when called via "other" routes.