Convert token processing to use token processor #19550
Conversation
|
(Standard links)
|
7977cb2 to
a5f7815
Compare
09d364a to
686ef98
Compare
|
In the future we will probably want to change the |
|
Yep potentially we will extend this function like that - we'll need to be sure we are adding test cover when we extend to other entities. |
|
(I think @totten also has some slightly deeper thoughts about tokens to work through before we commit ourselves to extending this function to other entities & migrating entity-based tokens away from smarty & towards the token processor format) |
a6aec80 to
2d42c23
Compare
|
The remaining test fails are the 'real' difference highlighted above |
| $tokenProcessor->addMessage('html', $mailContent['html'], 'text/html'); | ||
| $tokenProcessor->addMessage('text', $mailContent['text'], 'text/plain'); | ||
| $tokenProcessor->addMessage('subject', $mailContent['subject'], 'text/plain'); | ||
| $tokenProcessor->addRow($contactID ? ['contactId' => $contactID] : []); |
There was a problem hiding this comment.
@totten @eileenmcnaughton To support rendering other tokens here we just need to pass the ID through here. Eg.
[
'contactId' => $contactID,
'contributionId' => $contributionID,
]
Then it would work with this #19584 and contribution tokens would be rendered.
There's a few steps to get there but it's not a long way off - probably review/merge this, review/merge #19550, add testing on contribution tokens like @eileenmcnaughton did with contact tokens, review/merge #16983 + #19071, modify renderMessageTemplate() to accept different entity IDs and get that passed through from eg. contribution invoice workflow template.
There was a problem hiding this comment.
@mattwire - I would definitely want to include full test cover of the supported contribution tokens with any changes.
Also I'd want to delay exposing the api (incl sendtemplate api) until it's settled
2d42c23 to
a611137
Compare
This is a subset of the changes in civicrm#19550 that we should be able to resolve & merge while addressing that takes longer. It still gets us the benefit of adopting a preferred pattern Note that some test changes exist around handling of subject - the code now converts a new line to a space consistently. In addition tests that rely on leakage have been altered as smarty does not leak with this approach
|
As the challenges of the contact tokens are likely to take some time I've pulled out the changes that cover the domain tokens and smarty processing - which is the bulk of the code |
|
@eileenmcnaughton Thanks for your work on this. You should probably include the greetings text in your test (rather than just greeting_id). Also, the custom fields value vs label seems pretty significant, if not a show stopper. |
|
@nganivet the tested fields are what is currently there without this change. But yeah - not sure what to do about the custom field one |
|
@eileenmcnaughton it is very common to have custom fields as part of email templates, so I would propose blocker. Come to think about it, the preferred communication method, contact source and created date might be important for GDPR or similar, but probably to a lesser extend (unless EU chimes in ...) |
|
@nganivet note it's not 'custom fields' that have a problem - the issue is specific to what is rendered for entity reference custom fields - id vs name |
|
@eileenmcnaughton makes sense ... |
|
@nganivet to be clear - the reason it's tricky is that I think the scheduled reminders flow is rendering the tokens like the images on the right and the send receipt flow like the left. So the issue is that they should do the same thing but it's hard to know what that should be in the case of the entity reference custom field (& to a lesser extent fields like 'hash' that I feel shouldn't be exposed & postal-greeting_id which is exposing something that seems like it would cause bugs) |
|
@eileenmcnaughton Understood, and yes, confusing! Since the text substituted for the token is meant to be printed in an email, I suppose it would make a lot more sense to substitute the name/label rather than the id for all entities. The hash might need to be exposed as part of links to profiles. The greeting ids are an internal construct that should not be exposed, but the greeting displays most probably should. |
|
@nganivet looking at the above screenshots the greeting displays seem to work before AND after How would you use a hash in a link to a profile? It's used to generate the checksum but ideally would never be exposed directly |
|
@eileenmcnaughton correct, so we can the greeting ids, and s/hash/checksum/g |
|
@nganivet what does s/hash/checksum/g mean? |
|
vi speak: substitute hash with checksum in my previous response. |
|
OK - checksum is unchanged in both the images above (near the bottom of the last one) |
a611137 to
677be90
Compare
|
I've rebased this after getting the other merged - so now it's time to think about resolving the tokens here. I think there are 3 things we need to reconcile
The first token that needs resolving is hash & there are 2 options
|
|
+1 on removing hash and all greeting id tokens. |
|
I believe this change makes the result less-confusing. So also a +1 from me. |
|
We are currently using {contact.hash} in some of our workflows: I’m aware that this workflow has security issues if we were to expose forms to the public from our CiviCRM instance. I guess we could create some custom field that creates a constant hash or UUID for a contact or set the checksum duration to 0. But each would come with its own set of drawbacks and we need to change our synchronization of that data to an external mail service, which would be somewhat troublesome at least for the checksum. Lowest effort change would probably be to just recreate that token as a custom token. CiviMRF is using the {contact.hash} token aswell: https://github.com/CiviMRF/cmrf_core/blob/7.x-dev/cmrf_webform/cmrf_webform.module#L717 |
…tokens Per discussion on civicrm#19550 (comment) there appears to be agreement that supporting tokens like addressee_id (which resolves to '{contact.individual_prefix}{ }.....') should not be exposed / supported as they seem both unuseful and likely to be breaky. These were exposed unintentionally as part of a change to make them available as WHERE filters on apiv3 civicrm@54e389a The discussion suggests that by contrast we should add support to hash in the token compat subscriber
|
I added #19782 to remove the 3 fields that were agreed above should not be exposed ie email_greeting_id, addressee_id, postal_greeting_id The other field discrepancies need to be fixed in via the tokenCompat class - with the exception of the contact reference field which we still don't know how to deal with |
…tokens Per discussion on civicrm#19550 (comment) there appears to be agreement that supporting tokens like addressee_id (which resolves to '{contact.individual_prefix}{ }.....') should not be exposed / supported as they seem both unuseful and likely to be breaky. These were exposed unintentionally as part of a change to make them available as WHERE filters on apiv3 civicrm@54e389a The discussion suggests that by contrast we should add support to hash in the token compat subscriber
|
test this please |
|
I've added other custom field types to the test so checking that the contact reference is the only one |
|
Nice to see progress on the tokenprocessor. I really hope this will result in more entities. Mostly custom participant tokens which have never worked to far. Thank you all for your efforts in this area!
|
|
Yep - I don't want to expose more functionality until we have the inner workings solid now either through the UI or to extensions. I've been burnt before |
|
I know you are always going for quality of core, the long term view and solid testing and I think you should be in the dev list spotlight yourself for what you bring to civi with those high standards. You lift everyone up to them. Very inspiring! |
|
thanks for your kind words @magnolia61 |
|
This is to update the tokenprocessor to render display values for custom fields #19806 |
677be90 to
3d7e34d
Compare
Note this will test fail on 3 things that worked with the other. The way in which we load tokens means that some values have been exposed that possibly would be better not exposed - but we can either remove them from advertised tokens, fix the tokenCompat class to support them or possibly deprecate by removing from the UI display but continuing to handle. One notable difference is the contact_reference entity id.
3d7e34d to
49ab56a
Compare
|
@totten - we can finally merge this!!! |
|
@totten @mattwire if we merge this in time I might have a go at altering the tokenCompatSubscriber to use apiv4 - it feels like we are feeling confident in the test cover now but might forget that we checked it out later & also people will be expecting a change this release. Big picture we want to reduce BAO_Query object usage & use something more consistent where we can (by in time I mean in time for me to do it before the rc is cut) |
|
@seamuslee001 @mattwire @totten can we get this merged since we merged all the related PRs that led up to it & got it to a test-passing-state? |
Overview
Convert token processing to use token processor
This is intended to be a no-change patch to simply use newer/ more preferred coding conventions - however, it does throw up a small handful is discrepancies between them that need to be considered.
Before
Use of functions like
After
Use of token processor - this in turn calls the tokenCompat subscriber which has functionality to replace the domain & contact tokens & do the smary parsing.
However, it does turn out that there are some minor differences between the 2 approaches & we need to agree what to do about them. The 'expected' on the left is with the existing code & on the right with this change.
Technical Details
The test is in this PR (currently) but is also in a separate PR #19551 - which I recommend merging asap - at which point I'll rebase out of this
I think it would be OK to stop supporting or to deprectate some of these tokens as I believe they were added more or less accidentally - we would need to do some messaging. The contact reference custom field is probably the biggest deal IMHO
Comments