TokenProcessor - fix greetings tokens

[mattwire]

Overview

replaceGreetingsTokens must be called before replaceContactTokens because replaceContactTokens removes all 'invalid' tokens (including the {contact.email_greeting} etc. so they don't get replaced.

Found while working on emailapi extension and tokenprocessor.

Before

Greetings tokens not replaced.

After

Greetings tokens replaced.

Technical Details

TokenCompatSubscriber is a compatibility layer between contact tokens and the new token processor. So this change only affects code that uses the tokenProcessor (which is just scheduled reminders).

Also see CRM/Core/BAO/MessageTemplate.php where it is called in the same order as this PR.

Aside, I don't understand why we have separate functions for contact greetings and contact tokens - and why replaceContactTokens can't just call replaceGreetingTokens or similar (currently it's the other way around and replaceGreetingTokens calls replaceContactTokens internally! Ugh!

Comments

@aydun any chance of a quick review so we can get this into 5.24 with all the other tokenProcessor improvements that have already gone in. @eileenmcnaughton

[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).

[eileenmcnaughton]

@mattwire can we replicate in a test?

[aydun]

@mattwire This seems right.

Note also that the docblock for replaceGreetingTokens() says: "::replaceContactTokens() may need to be called after this method, to replace tokens supplied from this method." - which agrees with the reordering in this PR.

As you note, very little currently uses this.

• General standards
  • (r-explain) PASS
  • (r-user) PASS:
  • (r-doc) PASS
  • (r-run) PASS : Not run, but looks good.
• Developer standards
  • (r-tech) PASS:
  • (r-code) PASS
  • (r-maint) Undecided : would be good to have a test
  • (r-test) PASS

[totten]

I'm going to +1 the need for some test coverage to demonstrate the greeting token scenario.

• These tokens are edge-casey/obscure.
• The patch changes the mechanics of iterative substitution used in TokenCompatSubscriber. Iterative substitution is risky/hard to do well. If one is to maintain such a thing, you need test-coverage.
• TokenCompatSubscriber was/is a stop-gap until someone can fully remove iterative substitution. If one aims to replace it (eventually), then you need test-coverage to validate the replacement.

[mattwire]

@eileenmcnaughton @totten @aydun It appears in some cases greetings tokens are replaced when called after replaceContactTokens but in other cases (like my real-world testing) not. This test proves that it does work per this PR - but the test actually seems to work without the PR too!

Ok to merge?

[totten]

@mattwire The test looks sensible to me.

Guess we've rolled a hard dice on figuring that edge-case, and I don't want to drag this out more. On the upside: even if the test doesn't reproduce the exact bug, it does still mitigate the risk of forgetting to fix greeting-tokens during a replacement of TokenCompatSubscriber (which, for me, seems more valuable than patching-up the current implementation).

I made a small edit - when doing assertions in a loop, it helps to have some assertion outside the loop which checks the size/quantity/number of iterations. (If getRows() returned [], then the loop wouldn't do any assertions...)