dev/core#2745 - Contribution Tokens - Support 'contributionId'

[eileenmcnaughton]

Overview

This salvages the useful part (the test) from #21058 - it should fail for now but once #21109 is merged I'll rebase out that & we'll be left with the (still failing) test that will cover contribution tokens once they are listening properly - @totten had ideas on how to make it listen in #21079 - I'm also going to close that one as it needs to be re-worked to pass this test once #21109 is merged - but at this stage it's mostly full of stuff that is not relevant now

Before

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

This 'schema' is required if I copy the mechanism from #21144 - but that feels like it should be unnecessary

[totten]

Off the cuff, I'm OK with that because (one way or another) the signal has to be provided in the listTokens use-case, and it's good discipline to have an upfront idea of what data is going in.

OTOH, yes, it's sort of redundant for this use-case. There's enough information in the row-data to infer that contributionId is present. I suppose an opportune spot to leverage that might be at the start of TokenProcessor->evaluate() -- recompute schema to include array_unique(array_merge(array_keys(...each row...)). And the recently added bit in ActionSchedule::sendMailings() (vis-a-vis schema) would be -3 SLOC if the autodetection were better.

[totten]

This asserts a strong opinion that currency can be fetched as a single DB field. While the civicrm_contribution record meets that expectation, I don't see why it's a good thing to bake that opinion into the base-class of token source.

[totten]

This asserts a strong opinion that currency is one-per-row. While the civicrm_contribution record meets that expectation, I don't see why it's a good thing to bake that opinion into the base-class of token source.

[eileenmcnaughton]

Yeah - it's also overrideable by class. I can't offhand think of an exception to there being zero or one per row as this expects - but if there were more than one it could be added

[totten]

My main problem with this formulation is that the class is getting so heavy. It adds new mass+surface-area without removing equivalent stuff, and (for me) the purpose of loading data via APIv4+prefetch is to be more useful and also simpler.

To try to show where this sense comes from, I did a bit of rundown comparing two previous branches for this (#21109 aka cont_toke and #21145 aka master-contrib-allkeys). It's not quite apropos for this branch (b/c it remixes some of the elements between them), but I think this may still be a useful read.

Same signatures/semantics

public function checkActive(TokenProcessor $processor) {
public function __construct() {
protected $fieldMetadata = [];
protected function getEntityName(): string {
public function evaluateToken(TokenRow $row, $entity, $field, $prefetch = NULL) {	## similar except for currency
abstract protected function getApiEntityName(): string;     				## this is formally missing in cont_toke, but it's strongly implied
public function getAllTokens(): array {							## same, altho tbh it's confusing
public function isCustomField(string $fieldName) : bool {				## same. both have this awkward need to support `custom_123` tokens

SignatureWeight: Same

Compare: Primary data loading

public function alterActionScheduleQuery(MailingQueryEvent $e): void {		## same sig, diff content
public function prefetch(\Civi\Token\Event\TokenValueEvent $e): ?array {	## same sig, diff content
protected function getEntityAlias(): string {					## cont_toke
public function getExtendableTableName(): string {				## cont_toke
public function getBAOName(): string {						## cont_toke
protected function getFieldValue(TokenRow $row, string $field) {		## cont_toke
public function getEntityIDField() {						## master-contrib-allkeys

Opinion: This is the fundamental difference in how they load data. IMHO loading via prefetch+apiv4 is better/more maintainable than loading via alterActionScheduleQuery().

SignatureWeight: cont_toke=6; master-contrib-allkeys=3

Compare: Token/field lists

public function getReturnFields(): array {						## cont_toke
public function getPrefetchFields(\Civi\Token\Event\TokenValueEvent $e): array {	## master-contrib-allkeys

Opinion: These are basically the same idea. Either name is fine. Passing in$e allows you to fetch only needed columns, which matters somewhat now - and will matter more when folks can choose more PCs/FKs.

SignatureWeight:cont_toke=1; master-contrib-allkeys=1

public function getBasicTokens(): array {						## cont_toke
public function getExposedFields(): array {						## cont_toke
public function getSkippedFields(): array {						## cont_toke
protected function getApiTokens(): array {						## master-contrib-allkeys
public function getAliasTokens(): array {						## master-contrib-allkeys

Opinion: Both getApiTokens() and getBasicTokens+getExposedFields+getSkippedFields allow you to add/remove stuff. ButgetApiTokens() is simpler, less public, and more powerful than getBasicTokens+getExposedFields+getSkippedFields combined -- subclass writers only need to understand one function, and external callers can't access that function.

Opinion: getAliasTokens() is a trade. It adds functionality at the cost of adding complexity. One's opinion on that turns to one's opinion on the trade-offs/feasibility of compatibility-vs-migration. (Aliasing is a useful technique in a compatibility-based strategy and but not useful in a migration-based strategy.)

(It seems to me that you want both to push for migration and push against compatibility. I agree migration is more ideal, so I support work on that, but I have a dose of skepticism about its efficacy, and I prefer to keep clear space for compatibility in the interim.)

SignatureWeight: cont_toke=3; master-contrib-allkeys=2

Compare: Field reflection

public function isMoneyField(string $fieldName): bool {					## cont_toke
public function isDateField(string $fieldName): bool {					## cont_toke
protected function isApiFieldType(string $fieldName, string $expectType): bool {	## master-contrib-allkeys

Opinion: isApiFieldType() gives more functionality/expressiveness with less surface-area/code and similar readability/writeability.

Opinion: I'm not really sure that any of these functions belong in CRM_*_Tokens. Maybe there's some theoretical rationalization for that. I try to not be religious, though. But if it's there, seems better to keep it light.

SignatureWeight: cont_toke=2; master-contrib-allkeys=1

Compare: Pseudo fields

public function isPseudoField(string $fieldName): bool {					## cont_toke
public function getPseudoTokens(): array {							## cont_toke
public function isAddPseudoTokens($fieldName): bool {						## cont_toke
public function getPseudoValue(string $realField, string $pseudoKey, $fieldValue): string {	## cont_toke
public function getPseudoTokens(): array {							## master-contrib-allkeys
public function getPseudoValues(int $id): array {						## master-contrib-allkeys

Opinion: All of these seem broadly redundant to me, so personally I'd love to remove them. However, ByTypeTest locks in a strong equivalence between CRM_Contribution_Token/CRM_Utils_Token, and these functions allows one to hotwire CRM_Contribution_Token with CRM_Utils_Token as way to ensure that equivalence.

SignatureWeight: cont_toke=4; master-contrib-allkeys=2

(Note: Tallying up the signatures that differ: 16 in cont_toke vs 9 in master-contrib-allkeys. As far as I can see, it provides more functionality with less surface-area.)

[eileenmcnaughton]

@totten yeah - we are definitely on the bikes here - this class is solely used by contribution tokens at the moment and is clearly marked as internal so it's not an external interface - although we might agree one later (preferably in the Civi\ namespace . Some of the functions I added will of course be able to go - the switch to the new way of interacting with scheduled reminders is only partially done in this PR post your other work - which would mean the end of getEntityAlias() and getExtendableTableName(). Potentially also it would see the end of getBAOName - I haven't done any testing around a batch of records with multiple languages - ie when we do the v4 api get call we retrieve multiple rows with labels - presumably all having the same language whereas when we call the getLabel function we are presumably changing language each row - note I haven't tested this - I was trying to leave that consideration out of scope for now (I would have tested it before merging your other PR but in the end it was otherwise merged)

My plan, once we have a contribution tokens working is, as you note, to add the missing processors - contribution_recur, participant & case and of course I would extend this class - my expectation was that I would implement getSkippedFields for them & specify any fields we didn't want and if I really want to intefere I can override one of the others.

The reason I pushed quite hard on agreemenr around dev/core#2745 is that once we accept that it's OK to expose all tokens except ones that DON'T make sense then declaring which ones DON'T make sense becomes more helpful that an array of the tokens that seemed helpful when we wrote the array

[totten]

Potentially also it would see the end of getBAOName - I haven't done any testing around a batch of records with multiple languages - ie when we do the v4 api get call we retrieve multiple rows with labels - presumably all having the same language whereas when we call the getLabel function we are presumably changing language each row - note I haven't tested this - I was trying to leave that consideration out of scope for now (I would have tested it before merging your other PR but in the end it was otherwise merged)

Yeah, I think that's wise leaving that scope for another day. FWIW, I don't think there's much of a difference between BAO vs CRM_Core_PseudoConstant vs APIv4. In all cases, one wraps the read operation with some kind of setLocale()/setLanguage(). The nub is that wrapping setLocale() is easy for individual transactions but awkward for multilingual batches. I dunno where we'll land on that, but I think it'll be easier to get our heads around if all flows use the same dataload (ie prefetch()+api4).

My plan, once we have a contribution tokens working is, as you note, to add the missing processors - contribution_recur, participant & case and of course I would extend this class -

Right, which I think creates the pressure for us to like the base class. When we add more top, it'll become harder to move the bits underneath.

my expectation was that I would implement getSkippedFields for them & specify any fields we didn't want ... The reason I pushed quite hard on agreemenr around dev/core#2745 is that once we accept that it's OK to expose all tokens except ones that DON'T make sense then declaring which ones DON'T make sense becomes more helpful that an array of the tokens that seemed helpful when we wrote the array

TBH, on a policy level, it doesn't really matter to me if the fields are default-off(opt-in) or default-on(opt-out) for token support. Think of the risks as "probability of mistake * cost of mistake". For simple numbers, guesstimate that 90% of fields should be enabled and 10% should be disabled. With default-on, the default is mistaken only 10% of the time. (Default-off is much worse - 90% mistaken.) But. The cost of removing a bad token is higher than the cost of adding an omitted token. To add an omitted one, you just update the list. To remove a bad one, you need to migrate/break/replace/communicate (and you may have some additional impact - like revealing sensitive data).

I really can't say one is better than the other. I'm ambivalent. I'm OK with default-on, though.

The reason I submitted #21145 was because default-on seemed important to you, and I didn't want that presenting as some kind of reason to block the changes in the data-loading / simplification (which is the part that seems important to me).

[eileenmcnaughton]

@totten but #21145 isn't default on - it's default list all the fields out one by one? Where I was trying to get to was

• only having to override the skipped fields for the other entities I add - in particular contribution recur since I'll be doing testing with contribution & contribution recur tokens
• not listing every aliase in the class
• leaving the aliases question for later since we don't need it for this entity (there is no aliasing that needs to be handled at this stage and no special handling for campaign fields)

I DO see the base class as rather less locked in place than you do - since we'll be only using it from tested core code at the moment the inner workings should be more flexible for now & for the next few months

[mattwire]

What was the reason for creating the new CRM_Core_EntityTokens class instead of extending the CRM_Core_TokenTrait - seems like there's a bit of duplication now?

[eileenmcnaughton]

@mattwire - yeah it was always considered that it might be temporary & the 2 might be reconciled in some way in the end - the contribution class was starting from the point of not being exposed at all & so the stuff that was generic was moved onto the EntityTokens class. It should be a good basis for new classes (contribution_recur, participant) but the classes that already exist need a bit of effort to find the inconsistencies and standardise on a consistent token set. I wasn't going to try looking at reconciling with activity & it's trait down the track

[eileenmcnaughton]

Also - it's important to note this is an internal core class with 100% test cover so we can move stuff around as long as tests pass. I'm less sure with the trait - ie. hopefully no extensions are using it because it's not a supported interface but I don't know about the test cover

[eileenmcnaughton]

@totten so I'm trying to come back to this & find where the resolvable part is.

I guess to outline my priorites

1. the most important part of this PR is obviously the test & that all the code in the 2 classes we are dealing with has 100% test cover & the tests would fail without so we can move them around just relying on test cover until they 'settle'

2. that we work towards a situation* where the same tokens are advertised in the widget in scheduled reminders as in 'send a letter' and that those tokens work in both places (+ any that used to work in only 1 place are handled in some way)

3. that we are NOT finalising an interface at the moment - I'm conscious of this because you want to take out things that I found really helpful in getting to the point of having reconcilied multiple ways to load and render the tokens. I an agreed preferred interface (in the Civi namespace) is a good final result but I had intended to work through a process to add testing and standardisation to other entities which I don't feel good about continuing with if the class is locked down (that was also a key reason for not using the existing trait)

4. that having worked to agree how we want psuedoconstants they are added automagically based on the metadata - ie our token consistency doesn't rely on people defining 'contribution_status_id:name' but that we get that by virtue of 'contribution_status_id' being included. I want that consistency to not rely on how people enter the array.

5. either we are agreed on it being ok to get define an opt out list https://lab.civicrm.org/dev/core/-/issues/2745 - in which case I'll put in test cover so we see if the existing list changes & we should have a function to allow declaration of both opted in and both opted out fields (so that either or can be overridden) or we don't and we only allow opted in fields - but these fields would be the db field names and the pseudoconstant names would be intuited from them. If prefer the opt out approach because I find the decisions behind an opt out list clearer & cleaner - but if you really don't want that then now is your time to veto

6. I tried to leave out of scope anything that changed how the code interacts with the action schedule class and just add support for the loading of individual rows - I had seen that side of things as 'unchanged by the PR and suitable for a follow up

7. I also really want to leave out of scope any handling of aliases - mostly because it is unnecessary for this entity & hence doesn't meet my criteria of being in the test cover - but also because I think there are multiple ways of dealing with them & assuming I do continue on to try to ensure the other entities render consistent tokens regardless of whether they are called from scheduled reminders, work flow templates or the 'send an email' I will keep thinking about how to handle the tokens we are trying to phase out

• I should note the alternative to 2 in my mind is that we just make contribution & contribution_recur tokens work such that I can test your PR & I take all the other potential token classes off my todo list

[totten]

I've been through a few drafts trying to find a way to reply, but they kept getting too long. Trying again as a speed-version (now with lots of typos)...

• (1) Hooray test
• (2,7) I'm OK with tabling alias for the moment. I don't want it to stop you from working on migration stuff, and I don't want aliases to be a blocker/hangup generally. I also don't agree with your straight rejection of the concept, but I'm happy if you prove me wrong with a better alternative.
• (3a) I don't know where the idea of finality came from. I'm just pushing to do these increments better.
• (3b) I want to highlight that you achieved something that I would've said crazy/impossible - namely, wiring up CRM_Utils_Token to use CRM_Contribution_Tokens (ie getPsuedoValue/getPseudoValues) and get the PCs working in CRM_Utils_Token. My changes to the PV functions are not trying to reject that. To the contrary, I was trying to use/respect/improve on the path you made there.
• (4) Hooray metadata. Miscommunication: master-contrib-allkeys does have an override CRM_Contribution_Tokens::getApiTokens(), and it does not infer PCs automatically. That was not about rejecting metadata or PCs. The field-list comes from metadata already (EntityTokesn::getApiTokens()). The override was just to bridge the gap until we understood where to get metadata for PCs. Our conflict was about how to cope in the interim without that metadata. (It looks like that will be resolved a lot sooner than I was expecting, TBH - APIv4 pseudoconstant improvements #21184... somebody's been pushing Coleman... :) )
• (5a) I don't understand the categorical removal of contact_id across all entities.
• (5b) You could do a skiplist with function getApiTokens(){ ... array_filter(parent::getApiTokens()...)...}. But, I see that a dedicated function for a skiplist is appealing to you. So OK.
• (6) Right, for me, this simplification/consolidation is a big deal. I feel like the CRM_*_Tokens have been difficult to read because of the interdependence on actionSearchResult, and cleaning that out (and go straight-api4) goes a long toward that IMHO. It really is trying to fully realize your idea of APIv4 token-notation with Matt's idea of preferring fooId (instead of actionSearchResult).

[eileenmcnaughton]

@totten I've been thinking about this & I think the fact that we are having this level of architectural discussion over an incremental refactoring PR on a class I created a couple of weeks ago in order to work through an incremental refactoring process is actually a pretty clear sign that the process I was attempting to do isn't gonna work. We can negotiate our way through this one - but if this class has become a class with an agreed architecture rather than part of a refactoring and test-writing process then it no longer has the flexibility for me to continue with that process. And that is also OK - in that it was always gonna be unpaid work & choosing not to do it is also a positive thing.

So - assuming I'm no longer committed to looking at any other entities - there are 2 ways forward I see

1. we merge this & you do the next step of 6 as a follow up refactor (I always understood that to be outside the scope of this PR as relates to the interaction with the scheduled reminders which was not the focus of this pr).
2. you clean up your pr to rip out all references to the stuff that only related to a much earlier version of the discussion (the references to campaign_id.name syntax & api aliases) and I merge that.

Either way - with either/or merged I can write what I need for recurring tokens into our wmf extension & the combination of that + this merged will unblock me to review your PR and we can call it a day & just be happy we cleaned up one entity to the point where it is broadly available, tested & works consistently

[totten]

We discussed out-of-band and agreed (a) this does work for adding contributionId support and (b) we'll continue iterating and remain open to significant changes in the class.

Topics that may lead to major changes in this class will be (a) deduplicating the loading code, (b) allowing multilingual pseudoconstant evaluation, (c) sharing a lazy-load mechanism between TokenProcessor+Smarty.