Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#2745 - Contribution Tokens - Load all API fields #21145

Closed
wants to merge 4 commits into from

Conversation

totten
Copy link
Member

@totten totten commented Aug 15, 2021

Overview

A brief description of the pull request. Keep technical jargon to a minimum. Hyperlink relevant discussions.

Similar to #21109 but builds on top of #21144.

Before

What is the old user-interface or technical-contract (as appropriate)?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

After

What changed? What is new old user-interface or technical-contract?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Anything else you would like the reviewer to note

totten and others added 4 commits August 14, 2021 17:01
* All data-loading uses `prefetch()` and an ID field (eg `contributionId`).
* All special APIv4 values (pseudoconstants/joins/etc) are loaded by `civicrm_api4()`. No need to duplicate this stuff.
* When using `TokenProcessor`, columns will only be prefetched if they are actually needed.
* `alterActionScheduleQuery()` no longer prefetches data. Instead, it populates `tokenContext_contributionId` which feeds into `prefetch()`.
* Properly declare `abstract` methods.
* Subclasses (i.e. `CRM_Contribute_Tokens`) should primarily use these methods:
    - `getApiTokens()` - List of visible/exported fields.
    - `getAliasTokens()` - List of funny aliases for API fields. This adds support for the legacy token
       `{contribution.campaign}`, and it also
* The special `currency` rules cannot work in other entities. Move them to `CRM_Contribution_Tokens` specifically.
The default implementation of `getApiFields()` will return the list of all fields. You can
override to hide fields or add join/pseudo-constant fields.
@civibot
Copy link

civibot bot commented Aug 15, 2021

(Standard links)

@civibot civibot bot added the master label Aug 15, 2021
@eileenmcnaughton
Copy link
Contributor

@totten Ok - so I guess I thought we had been working on agreeing the idea of something metadata driven when there was no fields we wanted to skip - but this goes back to hard-defining all the fields & not using the metadata. I guess we didn't reach agreement on what we were trying to achieve.

I was hoping to build on #21109 by adding very basic classes for the missing entities (participant and contribution recur) and standardising the exisitng ones - but I was definitely expecting us to use metadata to define the pseudo-constant discoverable fields (and not add any dot notation fields until we get to an entity where it makes sense - we shouldn't be adding those campaign ones).

'check_number',
];
protected function getApiTokens(): array {
$result = parent::getApiTokens();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like #21184 provides the list of PC types. Then the parent implementation of getApiTokens() can provide the :name (etc). So this override goes away.

@eileenmcnaughton
Copy link
Contributor

Per discussion - closing - although some variant will likely emerge regarding the load consolidation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants