CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support #9563

totten · 2016-12-20T01:19:15Z

This includes support for both {mailing.*} and {action.*} tokens used
in CiviMail -- and includes a performance optimization to ignore unused
tokens in AbstractTokenSubscriber.

Notably, this provides support for CiviMail tokens in TokenProcessor...
but the CiviMail engine will not use TokenProcessor. Rather, this helps
other/future delivery-engines (such as the upcoming FlexMailer) get access to
the CiviMail tokens. The existing CiviMail engine should remain unchanged.

CRM-19690: Allow alternative email templating systems

This includes support for both `{mailing.*}` and `{action.*}` tokens used in CiviMail.

This is more significant with mailing tokens because some of them are fairly sizeable/expensive.

totten · 2016-12-20T18:29:58Z

Aside: The TokenProcessor was introduced recently (4.7.alpha's; as a way to make ScheduledReminders more hookable), so it may be a bit new/unfamiliar for most folks. There's some background at https://wiki.civicrm.org/confluence/display/CRMDOC/Token+Reference

omarabuhussein

just a view notes

omarabuhussein · 2016-12-22T01:03:56Z

CRM/Mailing/Tokens.php

+  }
+
+  /**
+   * Check something about being active.


same here .. you can just use @inheritdoc for this and other methods in this class

omarabuhussein · 2016-12-22T01:17:35Z

CRM/Mailing/ActionTokens.php

+  }
+
+  /**
+   * Evaluate the content of a single token.


you can just use @inheritdoc here

Hmm, well, I have pretty mixed feelings about this, e.g.

The tool I use most (PHPStorm) doesn't do anything special with inheritDoc, so it's easier to read the docs if you copy the docblock. (Would be really nice to cmd-click or hover or something to see the docs in situ.)

Copying sucks. (Duplication/stale info/etal.)

Civi coding standards (de jure) extend Drupal standards as a baseline, which means we ought to follow https://www.drupal.org/docs/develop/coding-standards/api-documentation-and-comment-standards#inheritdoc -- i.e. {@inheritDoc}

Civi code standards (de facto) use @inheritDoc instead of {@inheritDoc}.

The vast majority of cases of @inheritDoc are redundant/useless (you can just omit the docblock and the docs say it will assume inheritance).

Do you see a concrete upside in resolving this? What is it?

actually phpstorm do something about @inheritdoc ,, try to point to the method and press ctrl+Q or view >> quick documentation . So I think adding it is better than omitting it as in the last option you provided above.

Oh, nice. (For OSX, I had to use Ctlr-J or F1.) It seems to inherit docs correctly with each of these variations: {@inheritDoc}, @inheritDoc, and non-existent docblock. (Although with extended docs, {@inheritDoc} doesn't seem to interpolate docs in the expected way.) I'll change it to match the de facto convention from other Civi classes (@inheritDoc)

omarabuhussein · 2016-12-22T01:18:40Z

CRM/Mailing/ActionTokens.php

+class CRM_Mailing_ActionTokens extends \Civi\Token\AbstractTokenSubscriber {
+
+  /**
+   * Class constructor.


I think this (Class constructor ) is uneeded .. same in CRM_Mailing_Tokens class

We need it to declare the list of tokens ($this->tokenNames) which is used in a couple ways:

Optimizing the list of token to process (eg $activeTokens = array_intersect($messageTokens[$this->entity], array_keys($this->tokenNames));)

Providing metadata about the list of available tokens.

sorry ... I meant the docblock .. not the constructor itself ..

omarabuhussein · 2016-12-22T01:21:55Z

CRM/Mailing/ActionTokens.php

+        $field, $verp, $urls, TRUE));
+  }
+
+  protected function getTrackOpenUrl(\Civi\Token\TokenRow $row) {


you are not using this method so I think it is better to remove it for now

Good catch. It was a leftover from an earlier/misconceived approach to open-tracking. I'll remove it.

omarabuhussein · 2016-12-22T01:34:43Z

CRM/Mailing/ActionTokens.php

+      $row->context['mailingActionTarget']['email']
+    );
+
+    $row->format('text/plain')->tokens($entity, $field,


maybe this is a bit cleaner :

$formats = ['text/plain' => FALSE, 'text/html' => true]; foreach($formats as $format => $isHTML) { CRM_Utils_Token::getActionTokenReplacement( $field, $verp, $urls, $isHTML)); }

omarabuhussein · 2016-12-22T02:01:26Z

Civi/Core/Container.php

@@ -203,8 +203,12 @@ public function createContainer() {
      'Civi\Token\TokenCompatSubscriber',
      array()
    ))->addTag('kernel.event_subscriber');
+    $container->setDefinition("crm_mailing_action_tokens", new Definition(


a space before this line would make it easier to read .. by the way I don't like the way how are these events are registered here and this method may go bigger and bigger with time .. but I see there is already a plan to fix that https://github.com/totten/civicrm-core/blob/e0c753854db4ed1bcd3b0659a29b79f535c1aa08/Civi/Core/Container.php#L112

Yup, the intent is to address that readability issue by moving the declarations to smaller/separate files. Outside scope of the current PR, though.

omarabuhussein · 2016-12-22T02:04:01Z

Civi/Core/Container.php


-    foreach (array('Activity', 'Contribute', 'Event', 'Member') as $comp) {
+    foreach (array('Activity', 'Contribute', 'Event', 'Mailing', 'Member') as $comp) {


I was thinking that it may make much sense to fetch the components list dynamically and then checking if CRM_{$comp}_Tokens is their for that component ... but I think this might affect performance unless some sort of caching is performed .

We do cache the container definitions now-a-days (v4.7+; see loadContainer()), but I'd rather not create a sui-generis mechanism just for locating the CRM_*_Token classes. Rather, cleaning this bit should wait until we have a generally decent way to locate/discover service definitions (e.g. via annotation or YAML -- as hinted by https://github.com/totten/civicrm-core/blob/e0c753854db4ed1bcd3b0659a29b79f535c1aa08/Civi/Core/Container.php#L112 ).

omarabuhussein · 2016-12-22T02:07:53Z

tests/phpunit/CRM/Mailing/TokensTest.php

+/**
+ * @group headless
+ */
+class CRM_Mailing_TokensTest extends \CiviUnitTestCase {


why not just implement the headless interface instead of the heavy CiviUnitTestCase class .. the tests should perform much faster ..

Mostly because I reflexively want to reason about tests at the suite level -- e.g. "all the tests in CRM_AllTests extend CiviUnitTestCase" or "all the tests in org.example.foo extend PHPUnit_Framework_TestCase. (This way of thinking probably arose as a defensive reaction all the heaviness in CiviUnitTestCase...) Using something else (not CiviUnitTestsCase) would make the suite more... nuanced/diverse.

I think your suggestion has better-than-even odds of working, but I don't want to put that issue question in this scope-of-work.

yup I think you can just ignore this note ... because away from diversity thing you had talked about ... it would time consuming to use PHPUnit_Framework_TestCase instead of CiviUnitTestCase for these tests .

omarabuhussein · 2016-12-22T02:20:28Z

tests/phpunit/CRM/Mailing/TokensTest.php

+    }
+    $this->assertEquals(1, $count);
+  }
+


maybe a negative test could be added where neither mailingId or mailingObject is provided .

During development of FlexMailer, I flirted with using a special token (e.g. `{actions.trackOpen}`) but ultimately found that was positioned well in the order of operations.

totten · 2016-12-22T19:32:46Z

@omarabuhussein Thanks for the review. I've pushed updates for the unused function and the negative test scenario. Unless the style issues seem critical, I'm inclined to merge on pass.

omarabuhussein · 2016-12-22T19:46:28Z

Thanks @totten ... the style issues is not critical but I replied to your point though .. not sure what are u going to decide but the generally it is approved .

This is an offshoot from the conversation on civicrm#9563.

totten added 2 commits December 19, 2016 17:11

CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support

56df2d0

This includes support for both `{mailing.*}` and `{action.*}` tokens used in CiviMail.

CRM-19690 - AbstractTokenSubscriber - Only evaluate tokens that are used

e0c7538

This is more significant with mailing tokens because some of them are fairly sizeable/expensive.

civicrm-builder added the master label Dec 20, 2016

totten mentioned this pull request Dec 20, 2016

CRM-19690 - CiviMail/FlexMailer - Allow alternative email templating systems #9497

Closed

omarabuhussein suggested changes Dec 22, 2016

View reviewed changes

totten added 2 commits December 22, 2016 11:25

CRM-19690 - CRM_Mailing_Tokens - Remove unused getTrackOpenUrl

80f8e48

During development of FlexMailer, I flirted with using a special token (e.g. `{actions.trackOpen}`) but ultimately found that was positioned well in the order of operations.

CRM-19690 - CRM_Mailing_TokensTest - Define negative test scenario

b4a0d8e

omarabuhussein approved these changes Dec 22, 2016

View reviewed changes

CRM-19690 - CRM_Mailing_{Action,}Tokens - Docblock tweak

0ba33ff

totten added a commit to totten/civicrm-core that referenced this pull request Dec 22, 2016

(NFC) CRM_*_Tokens - Reduce redundant docs

298795c

This is an offshoot from the conversation on civicrm#9563.

totten mentioned this pull request Dec 22, 2016

(NFC) CRM_*_Tokens - Reduce redundant docs #9573

Merged

totten merged commit 68b4ca5 into civicrm:master Dec 23, 2016

totten deleted the master-19690-tokens branch December 23, 2016 01:28

totten mentioned this pull request Jan 6, 2017

Accumulated backports for Mosaico support #9555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support #9563

CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support #9563

totten commented Dec 20, 2016 •

edited by civicrm-builder

Loading

totten commented Dec 20, 2016

omarabuhussein left a comment

omarabuhussein Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

omarabuhussein Dec 22, 2016

omarabuhussein Dec 22, 2016

totten Dec 22, 2016

totten commented Dec 22, 2016

omarabuhussein commented Dec 22, 2016


		foreach (array('Activity', 'Contribute', 'Event', 'Member') as $comp) {
		foreach (array('Activity', 'Contribute', 'Event', 'Mailing', 'Member') as $comp) {

CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support #9563

CRM-19690 - CRM_Mailing_Tokens - Add TokenProcessor support #9563

Conversation

totten commented Dec 20, 2016 • edited by civicrm-builder Loading

totten commented Dec 20, 2016

omarabuhussein left a 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

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

totten commented Dec 22, 2016

omarabuhussein commented Dec 22, 2016

totten commented Dec 20, 2016 •

edited by civicrm-builder

Loading