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

REF Simplify tokenProcessor code #18612

Merged
merged 1 commit into from
Nov 29, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Sep 26, 2020

Overview

This does two things:

  1. Don't cast entity to an object in evaluateToken. We can just use the array directly.
  2. Simplify getReturnFields() and improve performance by building arrays outside the foreach loop and clarify what variables mean. Update to reflect that fieldMapping no longer has arrays as values.

Before

More complex to understand.

After

Simpler and easier to understand. Performance of getReturnFields() improved (slightly).

Technical Details

Comments

@civibot
Copy link

civibot bot commented Sep 26, 2020

(Standard links)

@aydun
Copy link
Contributor

aydun commented Nov 19, 2020

@mattwire I've eventually got round to looking at this ...

  1. Array instead of object - this make sense.

  2. fieldMapping maps from the token name to the API fields required to construct the token. Using arrays allows more complex tokens to be handled that require more than one API field. For example, an address token might want ['street_address', 'city', 'postal_code'] Although that is not needed for the current activity tokens the extra flexibility in CRM_Core_TokenTrait may be useful for other entities so I would be in favour of keeping that.

@mattwire
Copy link
Contributor Author

@aydun Thanks for reviewing - I hadn't considered that you might want to map a single token to multiple API fields but that does make sense. I'll back out that part of this PR and add some commenting to make that clearer.

@mattwire mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Nov 19, 2020
@mattwire mattwire force-pushed the tokenprocessorsimplify branch 2 times, most recently from 9d19e93 to 60196d1 Compare November 28, 2020 19:57
@mattwire mattwire force-pushed the tokenprocessorsimplify branch from 60196d1 to f10c962 Compare November 28, 2020 20:00
@mattwire mattwire removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Nov 28, 2020
@mattwire mattwire changed the title Simplify tokenProcessor code REF Simplify tokenProcessor code Nov 28, 2020
@mattwire
Copy link
Contributor Author

Have addressed comments from @aydun and should now be good to merge

@mattwire
Copy link
Contributor Author

Merging based on @aydun review

@mattwire mattwire merged commit 273a7df into civicrm:master Nov 29, 2020
@mattwire mattwire deleted the tokenprocessorsimplify branch November 29, 2020 15:30
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