Standardise what we pass to tokenProcessor so we don't have to add specific handling in each class for actionSchedule

[mattwire]

Overview

Currently when implementing an entity for tokenProcessor we have to try two different ways of getting the parameters - depending on if we were invoked via actionschedule (scheduled reminders) or via a "standard" tokenProcessor context (eg. activity search -> print document / emailapi extension with tokenprocessor (https://lab.civicrm.org/mattwire/emailapi/-/tree/newtokenprocessor)).

This PR modifies the parameters passed by actionschedule to be "standard" so that we can simplify the entity classes for tokenProcessor. Here, the work has been done for activities.

Also note the example - switching over the actionschedule token selector to use tokenProcessor to retrieve the list of activity tokens - as that is what is actually available to be processed!

Before

Two sets of parameters to check.

After

Just one set of parameters.

Technical Details

Explained above in overview. getEntityTableToSchemaMapping() function is not necessarily in the right place - we need a way of mapping from entity table to the context version - ideas welcome!

Comments

@eileenmcnaughton @aydun Be great if you have time to look at this as it will help with moving forward with the tokenProcessor more quickly!

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

[aydun]

@mattwire I haven't tested, but I like the intention. In my earlier work around this I mostly left the actionschedule stuff alone but simplifying this should make it easier to use tokenProcessor more widely.

[mattwire]

test this please

[eileenmcnaughton]

I've started looking at this but I'm not too familiar with the code. I clarified the dispatch change & did this #17154

[eileenmcnaughton]

I'm going to close this because it's stale & it's probably stalled until @totten is able to give some attention to this & related PRs. (I tried but eventually failed to get my head around the token processor plan so this is out of my wheel house)

[eileenmcnaughton]

@mattwire looks like you are still actively working on this - obviously the test issues need to be resolved before it's review ready - but you probably also want to connect with @totten on this because I think he is the only one who feels comfortable reviewing token processor related PRs (there are a few in the queue)

[eileenmcnaughton]

Note the existing test fails indicate we have test cover - but I don't know if this code approach fits the vision or not

[eileenmcnaughton]

	No changes.
	GitHub pull request #16983 of commit 8c29c5b, no merge conflicts.
	Checkstyle: 0 warnings from one analysis.No warnings since build 1,916.New zero warnings highscore: no warnings for 2,002 days!
	Test Result (45 failures / +45)CRM_Mailing_TokensTest.testTokensWithMailingId with data set #0CRM_Mailing_TokensTest.testTokensWithMailingId with data set #1CRM_Mailing_TokensTest.testTokensWithMailingId with data set #2CRM_Mailing_TokensTest.testTokensWithMailingId with data set #3CRM_Mailing_TokensTest.testTokensWithMailingId with data set #4CRM_Mailing_TokensTest.testTokensWithMailingId with data set #5CRM_Mailing_TokensTest.testTokensWithoutMailingJob with data set #0CRM_Mailing_TokensTest.testTokensWithoutMailingJob with data set #1CRM_Mailing_TokensTest.testTokensWithMailingObjectCivi.Token.TokenProcessorTest.testAddRowShow all failed tests >>>

[eileenmcnaughton]

I'm going to close this for now as the failing tests are long-standing. Please re-open when it's review ready (although it might be worth discussing with @totten first).

I'm doing another pass on the review queue trying to find the older reviewable PRs

[magnolia61]

Hi @mattwire I cannot review code quality but what could I do to help you test this PR? What testscenario's would I help you with?

[mattwire]

@magnolia61 The most important thing is that there is no change before or after this PR. Both contact and activity tokens should continue to work with schedule reminders with and without this PR applied so validation of that would cover the r-run part of the review.

[seamuslee001]

@mattwire we need some more explanation for why and we need to see this approved in gitlab first.

[mattwire]

This PR modifies the parameters passed by actionschedule to be "standard" so that we can simplify the entity classes for tokenProcessor. Here, the work has been done for activities.

@seamuslee001 The explanation is simple. Currently there are two different ways of passing params to tokenProcessor. This PR reduces that to one.

I appreciate that this does require someone with better knowledge of the tokenProcessor like @totten or @aydun to review but currently any work on improving the tokenProcessor system is basically stalled on this PR. The only thing I would say needs "concept approval" is "do we want one unified way or two different ways of initiating tokenProcessor" - the answer to that would seem obvious?

[MegaphoneJon]

@mattwire This is the same issue as CRM-19758, correct? The issue that's worked around in this PR.

If so, I think you'd get a pretty strong "concept approved" from myself, @twomice, and we can probably even get @xurizaemon to cast a vote :). Also see this commit.

[twomice]

Agreed, I like the concept, and it seems silly to require extensions and other token consumers to check the format of the token params; seems an obviously good idea to pass params in a consistent format. So concept approval from me: yes. Unfortunately I don't know much about the tokenProcessor internals, so must leave that to someone else.

[eileenmcnaughton]

OK - people seem to understand what it's doing so I've switched the labels back - to be honest the 'needs-concept-approval' was more 'is this the right technical approach'

[aydun]

@mattwire This makes sense to me from reading the changes and looks ok to merge.

[totten]

@mattwire - I've split out a few parts from here as separate/smaller PRs (#21091 and #21092). That gives three PRs:

• Scheduled Reminders UI - Show more activity tokens in admin GUI #21091 - Extract the update for admin UI
• (NFC) Expand test coverage for scheduled-reminders with {activity.*} tokens #21092 - Extract the updated unit-tests
• ActionSchedule - Pass real batches into TokenProcessor. Simplify CRM_Activity_Tokens. #21088 - A revised/alternative patch for TokenProcessor. This also ensures that activityId is propagated as part of the $row->context. It goes a bit further and ensures that the batch used by prefetch() corresponds to the full batch of pending messages.