Switch replaceGreetingTokens over

[eileenmcnaughton]

Overview

Another attempt at #21701 - note this replaces replaceGreetingTokens but if it works I would then stop calling & deprecate that function & move the love up to processGreetingTemplate & then up one more level

@colemanw I'm a bit stuck tracking down an apiv4 caching issue

    $group = \Civi\Api4\Group::get()
      ->addSelect('custom.*', 'id')
      ->addWhere('title', '=', 'Test Group With Custom Field')
      ->execute();

is failing because the apiv4 cache is loaded before the custom field is added

so this test fails
not ok 602 - Failure: CRM_Contact_Form_Task_AddToGroupTest::testAddToNewGroupWithCustomField

---

message: new_custom_group.Custom_Field element exists?
severity: fail

Before

legacy call to the hook replace

After

calls the token processor

Technical Details

Comments

[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]

We just forked but discussed putting the large token-patches into the 'token-release' 5.43 so people only have to focus on one release

[eileenmcnaughton]

Well it seems to be failing really quickly now....

[eileenmcnaughton]

Currently failing on testApi4CustomEntityACL - still something around caching

[totten]

There's also a test failure in DynamicFKAuthorizationTest. The backtrace is long - here's a condensed interpretation:

1. To reset the test (DynamicFKAuthorizationTest::tearDown()), it must recreate an Organization
2. To recreate an organization, it must set the greeting - for which the template has token {contact.organization_name}.
3. To resolve the token, it must determine what type of data is inorganization_name (money, date, etc)
4. To determine the type, it pulls a list of all fields on Contact API
5. To get a list of fields on Contact API, it pulls a list of all possible joins
6. To get a list of all possible joins, it loops through AllCoreTables and scans all tables.
7. When looping through AllCoreTables, it tries to access CRM_Fake_DAO_FakeFile
8. But this DAO isn't real - it's just a fake value used to exercise the authz code. Failure due to undefined class.

I guess there are a couple potential weak-links in the chain. It could be the mocked entity ("Use a real entity - or make full class for the mock one!"). Or it could be the join-scan ("Writing to one table shouldn't require scanning all tables!"). Or it could have a guard during the scanning ("Only scan extant classes!").

[eileenmcnaughton]

@totten ok thanks - let's see how it goes - that feels like a different gotcha to the one I was looking at so if we are lucky it's the same one but I suspect there is more than one

[eileenmcnaughton]

@totten still not finishing :-(

[eileenmcnaughton]

the worst of the fails seem to be in this zone

RUN:[[./scripts/phpunit --tap --log-junit '/home/jenkins/workspace/CiviCRM-Core-PR@2/junit/Civi\AllTests.xml' '--exclude-group' 'ornery' 'Civi\AllTests']]
Parsing schema description /home/jenkins/bknix-dfl/build/core-21790-sdjf/web/sites/all/modules/civicrm/xml/schema/Schema.xml
Extracting database information
Extracting table information
TAP version 13
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

ok 1 - Civi\API\Event\PrepareEventTest::testOnPrepare with data set #0 ('onPrepare_null', array('Widget', 'frobnicate', array(98, 'green', 3)), array('frob[green]'))

Installing core21790sdjftest_g5eol database

ok 2 - Civi\API\Event\PrepareEventTest::testOnPrepare with data set #1 ('onPrepare_wrapApi', array('Widget', 'frobnicate', array(98, 'green', 3)), array('frob[go green] and frob[who green]'))
not ok 3 - Error: Civi\API\Subscriber\DynamicFKAuthorizationTest::testOk with data set #0 ('Widget', 'create', array(20))
not ok 4 - Error: Civi\API\Subscriber\DynamicFKAuthorizationTest::testOk with data set #1 ('Widget', 'get', array(20))
not ok 5 - Error: Civi\API\Subscriber\DynamicFKAuthorizationTest::testOk with data set #2 ('FakeFile', 'create', array(10))
not ok 6 - Error: Civi\API\Subscriber\DynamicFKAuthorizationTest::testOk with data set #3 ('FakeFile', 'get', array(10))
not ok 7 - Error: Civi\API\Subscriber\DynamicFKAuthorizationTest::testOk with data set #4 ('FakeFile', 'create', array('fake_widget', 20))

[totten]

@eileenmcnaughton - I went to the hardware store to ask if they had a fix for the Fake DAO failures. They offered a sledgehammer, which I pushed up to the branch.

[eileenmcnaughton]

@totten I think I might cry in the corner for a bit...

[eileenmcnaughton]

It's finishing!

I think the last 3 might be caching within the processor - but it was the first one I got stuck on

Test Result (4 failures / +4)

api_v3_ACLPermissionTest.testApi4CustomEntityACL
Civi.Token.TokenProcessorTest.testRenderLocalizedHookToken
Civi.Token.TokenProcessorTest.testHookTokenDiagonal
Civi.Token.TokenProcessorTest.testHookTokenExtraChar

[totten]

Yeah, I went poking around testApi4CustomEntityACL yesterday, and it was a bit of a mystery to me. Just verbalizing in case it raises ideas... it seems that the test does this:

• Create a contact (C2); flag it as deleted; add some custom data
• Implement hook_civicrm_aclWhereClause to allow access to C2
• Use CustomValue.get to read both the custom-data and also contact.first_name (via join?)
• The result of CustomValue.get does mention C2 - but it fails to return the joined contact.first_name.

So... (1) i'd wouldn't have thought to test joining from CustomValue to a contact, though I guess it is pretty reasonable use-case; (2) given that it is a join, I would've expected contact_id.first_name or entity_id.first_name rather than contact.first_name; (3) assuming all that is as-intended, it's hard to see why a change in contact-greeting-tokens would prevent first_name from loading. (4) I set breakpoints and inspected the DB during execution, and I could see that the first_name in the DB was the expected value.

My only theory is some kind of caching issue? Like the mechanism of token-valuation causes some cache to hydrate (or not hydrate), and the same cache is somehow important to the CustomValue.get join mechanism? Nothing comes to mind, though there are some parts of this code-path where I'm not super-familiar...

---

Happy to see some other failures in TokenProcessorTests. I've worked on those tests for hook_civicrm_token*(), so maybe I'll give that whirl.

[totten]

Civi.Token.TokenProcessorTest.testRenderLocalizedHookToken
Civi.Token.TokenProcessorTest.testHookTokenDiagonal
Civi.Token.TokenProcessorTest.testHookTokenExtraChar

Pushed up a patch which should fix these. Each test registers more hook-listeners, and the hook-listeners aren't getting called at the needful time (due to Civi::$statics[__CLASS__]['hook_tokens']).

As for why the PR caused a change... I guess that the test invokes indivdualCreate() fairly early/often. With the revision, we get a very early call-path like individualCreate() => replaceGreetingTokens() => TokenProcessor => CRM_Contact_Tokens. Thus, fairly early on, it hydrates Civi::$statics[__CLASS__]['hook_tokens']. But now the hook-token-list is frozen, and it doesn't matter what the hook-listeners do.

@eileenmcnaughton The patch removes the static-cache because that seemed simple -and because I'm not sure it's frequent enough to need the static-cache. But I'm not wedded to that change. If you can figure a better way, then that's cool. Hopefully this at least points toward the issue.

[eileenmcnaughton]

@totten so re the static cache - my understanding is that if you had a job or mailing that rendered 5000 messages that function would be called 5000 times.

I guess the \Civi tests don't call the function to set up hooks that clears the cache & hence they fail whereas the ones in CRM are happily clearing the static.

[eileenmcnaughton]

Only real fail is the one we want @colemanw to help with -
Test Result (10 failures / +1)
api_v3_ACLPermissionTest.testApi4CustomEntityACL

• some master fails to re-run - test this please

[colemanw]

Whew @eileenmcnaughton that was a doozy but I think I fixed it.

[eileenmcnaughton]

@colemanw thanks - I totally think we should merge if this fixes it -

[eileenmcnaughton]

Also note #21912 is the next in the line of this piece of work - it simplifies the function to set greetings close to the point where we don't have to call this function 3 times to replace 3 tokens

[colemanw]

Yea if all tests pass then I just want to do a little class cleanup - my commit changes some service classes into just-plain-php classes so I should move them out of the api4\service namespace.

[colemanw]

Nevermind, I tried moving the classes but it wasn't an improvement.

[eileenmcnaughton]

woohoo!

[eileenmcnaughton]

I note the latest test run only took 2 hours - which eases my concern that this was slowing stuff down - however - we can still speed it up I think- #21912 is the key step towards that as it gets the update out of that function