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.

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

Align most of the tokens in the token processor handling with the legacy handling #19806

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 15, 2021

Overview

There are 2 ways in the code that tokens can be rendered -

  • legacy Utils_Token class (the main way)
  • the token processor (the preferred way)

We want to consistently use the token processor - but in order to do so we need to align it as a replacement.

Currently it is not rendering a few fields and presenting the db values, rather than the formatted values of the custom fields.

Before

The 'main' token routine was rendering tokens per the image on the right - the token processor was rendering them per the image on the left.

image
image
image
image

After

Both render them per the image on the right

The main differences are that the token processor was outputing the data directly from the database but now it is formatting. In addition it was unable to render multiple option values (that is not captured in the screen shot as I had to fix that to get the test to complete.

Technical Details

This revists 2 changes

  1. https://issues.civicrm.org/jira/browse/CRM-13161 - the expectations that $value might hold an array like [0 => 'id', 1 => 'label'] no longer appear to be true -and I've added tests for all fields that are offered up
  2. The decision not to format this data at this point seems like a non decision a0fcf99#diff-3f93091ccbe99a7c1ae88473057539b1021099b37852cfea68ff948f9f8d1a86R66 - it looks like the custom data was not available and hence code was added to fetch it whereas in all other places it was already available and code was added to format it

Comments

If we merge this it unblocks a lot of code rationalisation but it DOES change token behaviour and we need to warn people. I believe it affects scheduled reminders and CiviMail in some cases.

We probably should just do a pre-upgrade message and warn people.

This is groundwork for #19806

@civibot
Copy link

civibot bot commented Mar 15, 2021

(Standard links)


// FIXME: for some pseudoconstants we get array ( 0 => id, 1 => label )
if (is_array($value)) {
$value = $value[1];
Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Mar 15, 2021

Choose a reason for hiding this comment

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

This line causes an enotice without this cleanup - it was added as part of a fix for https://issues.civicrm.org/jira/browse/CRM-13161 - however, all those greetings and gender fields are now tested & don't appear to have regressed (based on the tests I ran locally) with this change and I don't think the format described is used anymore.

The enotice directly mapped to the token processor having multi-value custom field data

@eileenmcnaughton eileenmcnaughton force-pushed the msg_compat branch 3 times, most recently from 788de7f to dab9349 Compare March 16, 2021 06:29
@mattwire
Copy link
Contributor

@eileenmcnaughton Needs rebase

@eileenmcnaughton eileenmcnaughton force-pushed the msg_compat branch 2 times, most recently from 44b431f to 37ed8dc Compare March 16, 2021 21:59
@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire - looks like it included something I was reviewed - removed

@eileenmcnaughton eileenmcnaughton force-pushed the msg_compat branch 3 times, most recently from 3676b8b to 9113f16 Compare March 19, 2021 01:23
@eileenmcnaughton
Copy link
Contributor Author

I now have a table of the specific impacts which we can link to in the preUpgrade hook - perhaps

"CiviCRM has standardised some of the tokens for contact custom fields. If you use these in scheduled reminders or CiviMails or other custom flows then you might need to confirm you are still happy with the output. In general we are switching from outputting the values stored in the database to the display values"

Data type or token CRM_Utils_Token Token Processor
Custom date field Site formatted date eg. 01/20/2020 12:00:00.am SQL date eg2020-01-20 00:00:00
Option values Labels eg. Purple Names e.g P
Contact reference fields Name eg.Mr Orson Welles, Id eg.7
Link Html for link eg Raw eg.https://civicrm.org

@magnolia61
Copy link
Contributor

For consideration: token values vs token labels. The values make it a lot easier in smarty message templates to use conditionals, as labels can be changed and reworded for custom fields over time. best of both worlds would be to offer both. Maybe with an extra parameter: token:value / token:label Probably not within the scope of this consolidation effort but I though i;d mention it here.

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 22, 2021
Switches to defaulting to flexmailer, if enabled.

This should be merged in conjunction with civicrm#19806
in order to keep those on the default in consistent state
(ie they will already be getting display-rendering for custom fields
and that will still be the case if 19806 is merged)
@totten
Copy link
Member

totten commented Mar 23, 2021

@eileenmcnaughton Brilliant research and brilliant test :)

There's a small MC in a test-class, but I don't think it's substantive. I'll give this some r-run and file a longer review.

@eileenmcnaughton
Copy link
Contributor Author

@totten thx - rebased

@totten
Copy link
Member

totten commented Mar 24, 2021

  • General standards
    • (r-explain) Pass (Provisional, maybe tweak with info about r-tech)
    • (r-user) Discussion: Doesn't affect UI, but it does affect token compat. More discussion under r-tech.
    • (r-doc) Discussion (Depends on what you think of my r-tech comments)
    • (r-run) Pass: I tested this by creating several custom fields of different types -- and then creating an Activity and a matching scheduled reminder rule, and including a long list of tokens in the scheduled reminder. I got the same results reported by Eileen.
  • Developer standards
    • (r-tech) Discussion: So this obviously is the main issue. I think that -- for dates -- the change is clearly better. For option-values, contact-refs, and links, I think it is better to be aligned with traditional templates -- but I also agree with @magnolia61 that the old output is useful, and there may be some templates/use-cases for which it really is better to use the raw value. Alas, adding a notation like :label or :value would be a considerable sub-project. But I don't think it's too hard to support a notation like {raw_contact.created_date}. So... I think what we should is throw up something like https://lab.civicrm.org/extensions/rawtoken/ and tell anyone who cares to go use that extension.
    • (r-code) Pass
    • (r-maint) Pass
    • (r-test) Pass (provisional, last run)

@eileenmcnaughton
Copy link
Contributor Author

@totten right those are the same conclusions I drew - other than me not knowing about the raw_token

I have communicated this in more than one dev digest and even included the change table - so I think we should merge this and wordsmith up some sort of pre-upgrade hook message to pre-warn people before upgrading

@totten
Copy link
Member

totten commented Mar 24, 2021

@eileenmcnaughton I guess there isn't a gitlab issue for this? So maybe the upgrader message should link to the PR for discussion?

@eileenmcnaughton
Copy link
Contributor Author

@totten that sounds good - would you be OK to write the message as a follow up PR (I have a feeling you would do so much copy editing you might be better to write it & I can review :-)

@eileenmcnaughton
Copy link
Contributor Author

I would link to the actual table here - #19806 (comment)

@totten
Copy link
Member

totten commented Mar 26, 2021

would you be OK to write the message as a follow up PR (I have a feeling you would do so much copy editing you might be better to write it & I can review :-)

That seems like some kind of tennis move. 😉

Yeah, merging, and I'm working on the upgrader.

@totten totten merged commit b190ef9 into civicrm:master Mar 26, 2021
@eileenmcnaughton eileenmcnaughton deleted the msg_compat branch March 26, 2021 21:36
@totten
Copy link
Member

totten commented Mar 27, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants