dev/core#2814 fix tokenCompat to be consistent with unresolved tokens #21568

eileenmcnaughton · 2021-09-22T06:18:50Z

Overview

dev/core#2814 fix tokenCompat to be consistent with unresolved tokens

This fixes the tokenCompat subscriber to replace unresolved tokens with a
blank string in a consistent way.

Prior to this it would crash if smarty was enabled but not all tokens
were resolved & print unresolved tokens if smarty was not enabled.

The inconsistencies appear to be due to 'separate evolution' rather than 'reasons'

Before

if the context had 'contact' set unresolved tokens would be blanked out, otherwise not, potentially crashing smarty

After

consistently replaced at this point

Technical Details

If extensions want to do something different they have many levers to play with by implementing their own render but currently it is inconsistent so they would have to assume this behaviour anyway

@totten I copied & pasted the regex it's currently using from CRM_Utils_Token - but hopefully you have a better option for that

Comments

civibot · 2021-09-22T06:18:53Z

(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 · 2021-09-22T10:22:53Z

@aydun the failing test is one you added that locks in a different behaviour - currently there are 2 behaviours for legacy rather than 'real' reasons IMHO

remove unknown token with whitespace before output
output unknown token

This PR resolves on 1 - but in the test you locked in 2 so I wanted to see if you can make a case for 2 or whether the most important thing about 2 was that it wasn't 3 - smarty fatal error

eileenmcnaughton · 2021-09-22T10:38:29Z

Note I will update here if we resolve this https://lab.civicrm.org/-/ide/project/documentation/docs/sysadmin/tree/case/-/docs/upgrade/version-specific.md/

aydun · 2021-09-22T12:05:01Z

@eileenmcnaughton Good question! It's clear what the test is doing, but not why ... and I don't remember why that was sufficiently important to lock in with a test.

I vaguely recall under some circumstances there was something taking a second pass at resolving the tokens and if this blanked out the unknown token then the second pass had nothing to work with - possibly to do with multiple listeners for the TOKEN_EVALUATE event? Do we have any tests for tokens added by hooks? (I don't see any...)

If we change the behaviour to 1), does https://lab.civicrm.org/extensions/activitytokens still work?

eileenmcnaughton · 2021-09-22T12:18:24Z

@aydun yes - MessageTemplateTest:::testContactTokens() (for example) tests tokens rendered by hooks.

There have been quite a few token changes in 5.43 (there is a docs MR and I will do another dev-digest write up.

One of the changes is that this class now works 'propa' - ie

the tokens are resolved in the eval which runs before the render. Only the smarty work is now done in the render.
if there are hook tokens to be resolved the full contact is loaded in the eval & hookTokenValues is called. Otherwise only 'enough' of the token is loaded for contact tokens which runs in an eval to run after hooks.

What that means for the extension is

to run on 5.43 it probably ONLY needs to implement the token processor.

to run on older versions it needs the hook too - but to ensure the hook doesn't run on 5.43 (ie prefer the token processor since it's probably more efficient) the weight of the eval registration should probably be set to greater than 500 in order to run first

civicrm-core/Civi/Token/TokenCompatSubscriber.php

Lines 31 to 35 in 1527957

    
           'civi.token.eval' => [ 
        
             ['setupSmartyAliases', 1000], 
        
             ['evaluateLegacyHookTokens', 500], 
        
             ['onEvaluate'], 
        
           ],

I'm going to be trying to encourage people to test tokens heavily including via custom implementations once the rc is cut (or all the PRs are merged - whichever is sooner) (this is the gitlab list of related issues https://lab.civicrm.org/dev/core/-/issues?label_name%5B%5D=sig%3Atoken

It's likely the changes to make it propa remove the reason for this test -

aydun · 2021-09-22T13:30:57Z

If it's all 'propa' then we can probably remove the test and see if anything unexpected breaks.

FYI @jaapjansma - see Eileen's comments re your extension.

jaapjansma · 2021-09-22T15:19:23Z

It is not perse my extension, I have developed it for a specific use case and it was funded for this use case. So the extension is now shared with the rest of the world. :-)

So a PR on 'my' extension is always welcome. I will only fix this hence I have funding for it and that is probably when a client who is using this extension is going to upgrade. Up till then feel free to submit a PR.

aydun · 2021-09-22T17:32:28Z

@jaapjansma I wasn't suggesting that you "ought" to fix it, just that it's something you have an interest in and, since you probably have clients using it, might affect your clients. I don't use that extension. It's a friendly 'heads-up' - nothing more!

jaapjansma · 2021-09-22T19:05:05Z

@aydun I might have been to harsh in the tone of my message. Apologies. Thanks anyway for the heads up

eileenmcnaughton · 2021-09-22T20:58:55Z

tests/phpunit/CRM/Core/BAO/MessageTemplateTest.php

@@ -397,6 +397,17 @@ public function hookTokenValues(array &$details): void {
    }
  }

+  /**
+   * Test that unresolved tokens are removed in the end.


I should make it clear that I'm actually testing for lack-of-smarty fatal error here

This fixes the tokenCompat subscriber to replace unresolved tokens with a blank string in a consistent way. Prior to this it would crash if smarty was enabled but not all tokens were resolved & print unresolved tokens if smarty was not enabled. The inconsistencies appear to be due to 'separate evolution' rather than '*reasons*'

…ments+tests.

…wn tokens Additionally, this uses the regex from `TokenProcessor` instead of the regex from `CRM_Utils_Token`. These differ in whether they capture the '|filter' expressions.

totten · 2021-09-23T00:35:49Z

(@eileenmcnaughton) @totten I copied & pasted the regex it's currently using from CRM_Utils_Token - but hopefully you have a better option for that

Yeah, TokenProcessor::visitTokens() is a better helper for that, though it was marked private. I've pushed up some commits to make it more accessible (@internal public), to improve the docblocks/test-coverage, and to use it here.

Based on the feedback from @aydun @jaapjansma and from my own testing, I'll add "merge ready". (Eileen, if you're fine with my changes, then go ahead and merge.)

eileenmcnaughton · 2021-09-23T00:38:29Z

tests/phpunit/Civi/Token/TokenProcessorTest.php

@@ -45,19 +44,23 @@ public function testVisitTokens() {
      '{foo.bar}' => ['foo', 'bar', NULL],
      '{foo.bar|whiz}' => ['foo', 'bar', ['whiz']],
      '{foo.bar|whiz:"bang"}' => ['foo', 'bar', ['whiz', 'bang']],
-      '{love.shack|place:"bang":"b@ng, on +he/([do0r])?!"}' => ['love', 'shack', ['place', 'bang', 'b@ng, on +he/([do0r])?!']],


you didn't like this one?

Well, I wouldn't want anyone to accuse of desecrating the art of the B-52s...

eileenmcnaughton · 2021-09-23T00:41:33Z

thanks @totten that looks great! I've upped it to MOP.

However, one further thought - I believe this comment is obsolete now?

totten · 2021-09-23T00:46:46Z

(@eileenmcnaughton) However, one further thought - I believe this comment is obsolete now?

I believe you are correct.

eileenmcnaughton · 2021-09-23T00:51:07Z

@totten shall I push up a commit to remove that (I only ask in case you are doing it)

eileenmcnaughton · 2021-09-23T00:55:25Z

I did it

aydun · 2021-09-23T08:18:34Z

tests/phpunit/CRM/Activity/Form/Task/PDFLetterCommonTest.php

   * @throws \CRM_Core_Exception
   * @throws \CiviCRM_API3_Exception
   */
  public function testCreateDocumentUnknownTokens(): void {
    $activity = $this->activityCreate();
-    $html_message = 'Unknown token: {activity.something_unknown}';
+    $html_message = 'Unknown token: ';


What is this testing now without an unknown token in the input message? How about:

$html_message = 'Unknown token: {activity.something_unknown}'; $expected_output = 'Unknown token: '; ... $this->assertEquals($expected_output, $output[0]);

Oh dang - did I remove from the wrong line!

#21585 puts it back

civibot bot added the master label Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

View reviewed changes

eileenmcnaughton force-pushed the labels branch from e2d052a to 6352727 Compare September 22, 2021 22:34

eileenmcnaughton force-pushed the labels branch from 6352727 to 7237967 Compare September 22, 2021 22:35

totten added 2 commits September 22, 2021 17:20

(REF) TokenProcessor::visitTokens() - Increase visibility. Refine com…

8d4c4d6

…ments+tests.

(REF) TokenCompatSubscriber - Simplify expression that replaces unkno…

03946c1

…wn tokens Additionally, this uses the regex from `TokenProcessor` instead of the regex from `CRM_Utils_Token`. These differ in whether they capture the '|filter' expressions.

eileenmcnaughton commented Sep 23, 2021

View reviewed changes

totten added the merge ready PR will be merged after a few days if there are no objections label Sep 23, 2021

eileenmcnaughton added the merge on pass label Sep 23, 2021

Remove old comment

e1281f2

totten merged commit 9a603bd into civicrm:master Sep 23, 2021

eileenmcnaughton deleted the labels branch September 23, 2021 05:21

aydun reviewed Sep 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#2814 fix tokenCompat to be consistent with unresolved tokens #21568

dev/core#2814 fix tokenCompat to be consistent with unresolved tokens #21568

eileenmcnaughton commented Sep 22, 2021 •

edited

Loading

civibot bot commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

aydun commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

aydun commented Sep 22, 2021

jaapjansma commented Sep 22, 2021

aydun commented Sep 22, 2021

jaapjansma commented Sep 22, 2021

eileenmcnaughton Sep 22, 2021

totten commented Sep 23, 2021

eileenmcnaughton Sep 23, 2021

totten Sep 23, 2021

eileenmcnaughton Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

totten commented Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

aydun Sep 23, 2021

eileenmcnaughton Sep 23, 2021

eileenmcnaughton Sep 23, 2021

dev/core#2814 fix tokenCompat to be consistent with unresolved tokens #21568

dev/core#2814 fix tokenCompat to be consistent with unresolved tokens #21568

Conversation

eileenmcnaughton commented Sep 22, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

aydun commented Sep 22, 2021

eileenmcnaughton commented Sep 22, 2021

aydun commented Sep 22, 2021

jaapjansma commented Sep 22, 2021

aydun commented Sep 22, 2021

jaapjansma commented Sep 22, 2021

eileenmcnaughton Sep 22, 2021

Choose a reason for hiding this comment

totten commented Sep 23, 2021

eileenmcnaughton Sep 23, 2021

Choose a reason for hiding this comment

totten Sep 23, 2021

Choose a reason for hiding this comment

eileenmcnaughton Sep 23, 2021

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 23, 2021

totten commented Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

eileenmcnaughton commented Sep 23, 2021

aydun Sep 23, 2021

Choose a reason for hiding this comment

eileenmcnaughton Sep 23, 2021

Choose a reason for hiding this comment

eileenmcnaughton Sep 23, 2021

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 22, 2021 •

edited

Loading