dev/core#2554 Remove code to assign contact to the template in sendEmail

[eileenmcnaughton]

Overview

Remove code to assign contact to the template in sendEmail

Same logic as #21475

Before

Code does a lot of work to generate the variable $contact which is assigned to the template & usable IF CIVICRM_MAIL_Smarty is defined & true

After

Code removed - if sites really want this the can install https://github.com/eileenmcnaughton/legacytokenhelper
However, in my analysis for that I decided it was basically useless as only the requested tokens would be added to the assigned $contact object

Technical Details

@demeritcowboy

Comments

Docs https://lab.civicrm.org/documentation/docs/sysadmin/-/merge_requests/318

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

Is this the same logic as the other one? This removes both hook_tokens and hook_tokenValues. And you can change the $details and hence the $values. I should probably look at what the extension does.

[eileenmcnaughton]

@demeritcowboy yep - the same as the other one - so the $contact is no longer assigned to the template - but the extension would do that for you if you really needed it

[eileenmcnaughton]

Just noting that https://lab.civicrm.org/dev/core/-/issues/2554 is closable once this is merged - not sure if we can put

Fixes dev/core#2554

anywhere to make that happen

[demeritcowboy]

A couple notes. The first one is the main one.

1. If I install the extension, tokens like {contact.display_name} that might be in the email generate an error because the extension tries to evaluate them with smarty.
2. If I don't install the extension but I did have something in a pre-existing message template that tried to access {$contact} (the smarty tpl var), it now gives an E_NOTICE. That might be solvable with some communication around this.
3. This is admittedly a non-standard use of the tokenValues hook but if I had code that did something like change $values[$cid]['display_name'] = 'changed', then it no longer changes {contact.display_name}. Similarly if I have code like $values[$cid]['something'] = 'access me with smarty' and have {$contact.something} in the message then it no longer gets evaluated even with the extension, because the hook doesn't get called as much anymore. They're both an odd use though, because why not just make a proper token for it if you're implementing the hook anyway.

[eileenmcnaughton]

@demeritcowboy so I feel like the main plan here would be to communicate - I have added it to the merge notes & it will be in the dev-digest & release notes

I think removing this line

https://github.com/eileenmcnaughton/legacytokenhelper/blob/master/legacytokenhelper.php#L203 might solve 1 tho?

[eileenmcnaughton]

@demeritcowboy I removed the above line eileenmcnaughton/legacytokenhelper@07ec1bf - I think the extension was meant to ensure assignment not to parse it

[demeritcowboy]

Hmm ... just having the extension installed seems to make it replace {contact.display_name} with the wrong contact (it seems to be the contact who's contact id is the case id, as if it's flipping the ids). And what gets assigned as the {$contact} variable is an array keyed on id (before it was just the array), and it doesn't have the same number of items that used to be in $values.

Is it worth pursuing having the extension? I don't have any real sense of how many people are doing things like accessing {$contact.contact_id} or branching like {if $contact.is_deceased}something 1{else}something 2{/if}

[demeritcowboy]

I added a suggested line to the dev digest draft.

[eileenmcnaughton]

@demeritcowboy ok - let's just use the dev digest to communicate this + release notes & worry about honing the extension if anywone actually cares about the functionality? There are a few copy & paste places affected & they make the code much more unmaintainable for a possible feature but possibly just accident

[eileenmcnaughton]

@colemanw any thoughts? The underlying issue is we spend a tonne of code & complexity to assign $contact to the template - which is only usable with a specific define on the site & only contains keys IF the equivalent token has been requested

[demeritcowboy]

only contains keys IF the equivalent token has been requested

That last part isn't entirely true. It contains a couple like is_deceased regardless, or you can add values if you've used hooks in a weird way.

I'm ok to merge this but people are going to need to test their setups, not just because of this PR, but because there's a large of number of token and messaging things moved around in 5.43.

[eileenmcnaughton]

@demeritcowboy ok - lets' do it - it makes a huge difference to being able to make the code sane & we already have a big communication / testsing task around tokens next release - it will go down better if we can get to the part of adding functionality ;-)

[eileenmcnaughton]

side point

{if $contact.is_deceased}something 1{else}something 2{/if}

can also be expressed as

{if '{contact.is_deceased}'}something 1{else}something 2{/if}

since the token is resolved first

[demeritcowboy]

Is that order something new because you used to have to do {capture} first to get the text?

[eileenmcnaughton]

@demeritcowboy I don't think so - I think we use capture for a combo of reasons we need to use capture & things we are confused about...