dev/core#2747 - Reconcile fields. Consolidate/simplify loading of APIv4-style tokens. #21079

totten · 2021-08-10T09:07:23Z

Overview

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

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

This adds the last fields that were in legacy tokens but not scheduled reminders and now the same code is deriving the list for each. I wound up UI testing rather than unit testing that custom fields are still advertised without this line as the test uses rollback & adding custom fields would break that.

…cale(pickLocale(...))

…le']` Before: Runs `setLocale()` and then executes the entire pipeline for `TokenProcessor` After: Leaves the global locale alone. Instead, rely on `TokenProcessor` to switch locale as it visits each recipient.

This block purports to catch any exceptions of type `TokenException`. However, if you grep the source tree, you will find that it is never thrown.

Overview: `ActionSchedule::sendMailings()` fetches a batch of pending reminders (per some specific schedule/rule). This improves support for prefetching related data. Before: For each item in the batch, it makes a new `TokenProcessor`. This means that the `TokenProcessor` only sees one pending reminder -- and it cannot meaningfully fetch batched data. After: It creates one `TokenProcessor` and adds rows for each pending reminder. This means that `TokenProcessor` can fetch batched data.

This will help us to consolidate the prefetching logic in `CRM_*_Tokens`. Currently, `CRM_*_Token::alterActionScheduleQuery()` updates the query to select all fields for the entity. This is regardless of how many fields there are, whether they are needed, etc. This is also prone to naming conflicts. With this patch, `CRM_*_Token::alterActionScheduleQuery()` only needs to select one field (`tokenContext_fooId`). This will then propagate to the `TokenProcessor` which can do its own optimized data-loading.

* 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.

civibot · 2021-08-10T09:07:25Z

(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-08-10T10:31:13Z

CRM/Contribute/Tokens.php

   */
-  protected function getExposedFields(): array {
-    return [
+  protected function getApiTokens(): array {


So once we resolve https://lab.civicrm.org/dev/core/-/issues/2745 my expectation is we would entirely remove this function in favour of a metadata-driven function on the parent class - because there is nothing (yet) in this token class that should not be purely metadata driven

At some point in the future we probably will need to add some calculated fields (paid_amount, balance_amount) but that is out of scope

eileenmcnaughton · 2021-08-10T10:33:08Z

CRM/Contribute/Tokens.php

-    $return = [];
-    foreach ($this->getExposedFields() as $fieldName) {
-      $return[$fieldName] = $this->getFieldMetadata()[$fieldName]['title'];
+  public function getAliasTokens(): array {


I think at some point we will need a getDeprecatedTokens function - but we don't want to add already-deprecated tokens - we should agree what the syntax is we are adding and standardise on it

ie I think we have 'advertised tokens' and 'deprecatedTokens' but we shouldn't need 'aliasTokens' - that's apiv3 all over again.

eileenmcnaughton · 2021-08-10T10:33:52Z

CRM/Contribute/Tokens.php

-   * @return string
-   */
-  protected function getEntityAlias(): string {
-    return 'contrib_';


OK - if changing this is definitely ok for scheduled reminders

eileenmcnaughton · 2021-08-10T10:37:34Z

CRM/Core/EntityTokens.php

+ *
+ * - (MUST) Implement getEntityName() and getApiEntityName()
+ * - (MUST) Implement getApiTokens()
+ * - (MAY) Implement getAliasTokens()


I think we should offer 'getDeprecatedTokensnotgetAliasTokens` - we are going to need a way to deprecate tokens but we only want to have one 'right' token for each thing.

eileenmcnaughton · 2021-08-10T10:38:44Z

CRM/Core/EntityTokens.php

    }
-    if ($this->isMoneyField($field)) {


This is generic - lots of classes will need this - so moving it to contribution is the wrong way - we might need a 'getCurrency' function for when we hit a class that doesn't have it's own currency field

eileenmcnaughton · 2021-08-10T10:48:33Z

@totten per chat - I don't think there is anything in the contribution tokens class that isn't generic / shouldn't be metadata driven.

My expectation was that once 2747 was resolved the final class would look like

class CRM_Contribution_Tokens {
  public function getApiEntityName() {
    return 'Contribution';
  }

  /** 
   * Specify currency field if it can be derived from the given entity.
   * 
   * If currency can't be loaded the site default currency will be used.
   */
  public function getCurrencyField() {
    return 'currency';
  }

}

as this token class has nothing in it that wouldn't be wanted by other token classes with the same fields - ie every entity has money fields, every entity has campaigns and fields with psuedoconstants & they should all be able to be driven off the metadata.

In terms of transitioning - I do think that we will need a getDeprecatedTokens function - but we shouldn't need that in the EntityTokens class because we don't have any tokens in that class at the moment (and we don't want to add any) that don't match whatever we agree should be 'expected'

eileenmcnaughton · 2021-08-10T10:53:45Z

tests/phpunit/CRM/Contribute/ActionMapping/ByTypeTest.php

+      fee_amount = {contribution.fee_amount}
+      legacy campaign = {contribution.campaign}
+      campaign_id = {contribution.campaign_id}
+      campaign name = {contribution.campaign_id:name}


OK - so it looks like you added the pseudoconstant form - I think we should either make this work in apiv4 or use the dot notation per https://github.com/civicrm/civicrm-core/pull/21076/files#diff-196c289971f24f7589d89f613b6199310f614fbe5e2aac9368b1563bf64d2f96R291

eileenmcnaughton · 2021-08-10T10:57:37Z

tests/phpunit/Civi/Token/TokenProcessorTest.php

@@ -139,6 +139,33 @@ public function testRowTokens() {
    }
  }

+  public function testRenderLocalizedSmarty() {


eileenmcnaughton · 2021-08-10T11:02:21Z

CRM/Core/EntityTokens.php

-    }
-    if ($pseudoKey === 'label') {
-      $fieldValue = (string) CRM_Core_PseudoConstant::getLabel($this->getBAOName(), $realField, $fieldValue);
+  public function getPseudoValues(int $id): array {


It looks like this is only called from a different class now - so it should be moved to that class.

note there is no reason to deprecate - it's only called from recent tested core code

eileenmcnaughton · 2021-08-10T11:04:02Z

CRM/Core/BAO/ActionSchedule.php

@@ -267,38 +267,44 @@ public static function sendMailings($mappingID, $now) {
      );

      $multilingual = CRM_Core_I18n::isMultilingual();
+      $tokenProcessor = self::createTokenProcessor($actionSchedule, $mapping);


If these changes all work - I'm all for them :-)

eileenmcnaughton · 2021-08-10T11:04:26Z

CRM/Core/BAO/ActionSchedule.php

   */
-  public static function setCommunicationLanguage($communication_language, $preferred_language) {
+  public static function pickLocale($communication_language, $preferred_language) {


eileenmcnaughton · 2021-08-10T11:05:17Z

CRM/Core/EntityTokens.php

+ * To write a subclass:
+ *
+ * - (MUST) Implement getEntityName() and getApiEntityName()
+ * - (MUST) Implement getApiTokens()


I think this should be 'May' implement getApiTokens. - We don't need to implement this if we just use the metadata

eileenmcnaughton · 2021-08-10T11:05:29Z

CRM/Core/EntityTokens.php

+ *
+ * To write a subclass:
+ *
+ * - (MUST) Implement getEntityName() and getApiEntityName()


Only one of these should be must imho

eileenmcnaughton · 2021-08-10T11:06:19Z

CRM/Core/EntityTokens.php

@@ -25,31 +32,29 @@
 * AbstractTokenSubscriber in future. It is being used to clarify
 * functionality but should NOT be used from outside of core tested code.
 */
-class CRM_Core_EntityTokens extends AbstractTokenSubscriber {
+abstract class CRM_Core_EntityTokens extends AbstractTokenSubscriber {


sure- there's still an open question about merging this with AbstractTokenSubscriber or not extending it - but I'm agnostic

eileenmcnaughton · 2021-08-10T11:09:54Z

CRM/Core/EntityTokens.php

  }

  /**
   * Get pseudoTokens - it tokens that reflect the name or label of a pseudoconstant.
   *
-   * @internal - this function will likely be made protected soon.
-   *
+   * @internal - this function is a bridge for legacy CRM_Utils_Token callers. It should be removed.


We don't need to deprecate this - we can move or remove it - none of the functions on this class are set in stone - they are only used in recent tested core places

Having said that - the point of this was to take the metadata for the entity and add the pseudotokens where the metadata required it - once we remove the hard coded list of tokens in favour of metadata we will need 'something' to add in these tokens

eileenmcnaughton · 2021-08-10T11:10:39Z

CRM/Core/EntityTokens.php

-        $return[$fieldName . ':name'] = ts('Machine name') . ': ' . $this->fieldMetadata[$fieldName]['input_attrs']['label'];
+    $labels = $this->getAllTokens();
+    // Simpler, but doesn't currently pass: $labels = $this->tokenNames;
+    $r = [];


is$r what $return looks like in your bikeshed?

eileenmcnaughton · 2021-08-10T11:17:31Z

CRM/Contribute/Tokens.php

+  public function getAliasTokens(): array {
+    $aliases = [];
+    if (CRM_Campaign_BAO_Campaign::isCampaignEnable()) {
+      // Unit-tests are written to use these funny tokens - but they're not valid in APIv4.


the unit tests from one of the 2 PRs used them..... the other one used campaign_id.name - we just need to agree which syntax it is

mattwire · 2021-08-11T20:46:47Z

@totten Needs rebase

eileenmcnaughton · 2021-08-11T20:55:34Z

Yeah - probably not worth rebasing this until we've resolved the campaign handling / https://lab.civicrm.org/dev/core/-/issues/2745

eileenmcnaughton · 2021-08-11T20:57:39Z

Or more accurately - we should go back to this as the reconciliation PR #21046 if we can resolve #21093 & then this PR probably gets closed in favour of one that just addresses the loading

eileenmcnaughton · 2021-08-13T22:11:21Z

I'm closing this out for now as it's less of a 'needs rebase' and more of a 'the couple of bits that are still relevant need to be picked out & put in a PR with this test #21134 '

I'm leaving that step (setting up evaluate token) to you @totten since what I did was just 'something I copied from elsewhere that was just enough to get the tests to pass'

eileenmcnaughton and others added 11 commits August 10, 2021 11:52

(REF) ActionSchedule - Convert setCommunicationLanguage(...) to setLo…

0725c3f

…cale(pickLocale(...))

(REF) TokenProcessor::render() - If there is a locale, then use it

ced4ac6

ActionSchedule - Convert from global setLocale() to `$context['loca…

4344436

…le']` Before: Runs `setLocale()` and then executes the entire pipeline for `TokenProcessor` After: Leaves the global locale alone. Instead, rely on `TokenProcessor` to switch locale as it visits each recipient.

(REF) ActionSchedule - Remove unused try/catch

c359783

This block purports to catch any exceptions of type `TokenException`. However, if you grep the source tree, you will find that it is never thrown.

CRM_Utils_Token - Accept multiple dots in token names

192bb4f

Contribution Tokens - Add legacy token {contribution.campaign} to test

2d94646

(REF) TokenRow - Add copyToken() helper

6b548fa

civibot bot added the master label Aug 10, 2021

eileenmcnaughton reviewed Aug 10, 2021

View reviewed changes

totten mentioned this pull request Aug 11, 2021

ActionSchedule - Pass real batches into TokenProcessor. Simplify CRM_Activity_Tokens. #21088

Merged

eileenmcnaughton mentioned this pull request Aug 13, 2021

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

Merged

eileenmcnaughton closed this Aug 13, 2021

This was referenced Aug 15, 2021

(dev/mail#83) Workflow Messages - Introduce class contracts #21139

Merged

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#2747 - Reconcile fields. Consolidate/simplify loading of APIv4-style tokens. #21079

dev/core#2747 - Reconcile fields. Consolidate/simplify loading of APIv4-style tokens. #21079

totten commented Aug 10, 2021 •

edited

Loading

civibot bot commented Aug 10, 2021

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton commented Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

eileenmcnaughton Aug 10, 2021

mattwire commented Aug 11, 2021

eileenmcnaughton commented Aug 11, 2021

eileenmcnaughton commented Aug 11, 2021

eileenmcnaughton commented Aug 13, 2021

dev/core#2747 - Reconcile fields. Consolidate/simplify loading of APIv4-style tokens. #21079

dev/core#2747 - Reconcile fields. Consolidate/simplify loading of APIv4-style tokens. #21079

Conversation

totten commented Aug 10, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Aug 10, 2021

eileenmcnaughton Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Aug 11, 2021

eileenmcnaughton commented Aug 11, 2021

eileenmcnaughton commented Aug 11, 2021

eileenmcnaughton commented Aug 13, 2021

totten commented Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton Aug 10, 2021 •

edited

Loading

eileenmcnaughton commented Aug 10, 2021 •

edited

Loading