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#2832 Switch membership pdf to use token processor #21521

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 17, 2021

Overview

dev/core#2832 Switch membership pdf to use token processor

Replaces final instances of replaceEntityTokens

https://lab.civicrm.org/dev/core/-/issues/2832

Before

legacy function used for membership tokens

After

token processor used

Technical Details

Test cover in tests/phpunit/CRM/Member/Form/Task/PDFLetterTest.php

Note that the alterations to that test are for token changes previously documented here and for which we added a rule to prevent them being submitted

Also note that I updated the deprecated class with the same change in order to make it easier to see the function is no longer used

https://lab.civicrm.org/documentation/docs/sysadmin/-/merge_requests/318

Comments

@civibot
Copy link

civibot bot commented Sep 17, 2021

(Standard links)

@civibot civibot bot added the master label Sep 17, 2021
@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Sep 20, 2021

@demeritcowboy so my thinking is that the path on pdfs looks like

  1. get this & dev/core#2851 Remove calls to replaceContributionTokens in contribution pdf letter task #21524 merged

  2. I figure out how to replace replaceMultipleContributionTokens from the contribution one (I think I'm close)

  3. update so all classes that use the pdfTrait use the same render code

  4. add support to that for participant tokens (at that point it should be a one line change to do that - I'm hoping the participant tokens will be otherwise merged by then)

  5. breathe a sigh of relief

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 I'm a bit blocked on the tokens PR you tried testing until #21584 is resolved -but if you had time to test this membership tokens one it would help unblock getting them working with pdf letters once that path is sorted

'messageTemplate' => ['msg_html' => $html_message],
'contactId' => $membership['contact_id'],
'schema' => ['contactId', 'membershipId'],
'tokenContext' => ['membershipId' => $membership['id']],
Copy link
Member

Choose a reason for hiding this comment

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

looks good

$html[] = CRM_Core_BAO_MessageTemplate::renderTemplate([
'messageTemplate' => ['msg_html' => $html_message],
'contactId' => $membership['contact_id'],
'schema' => ['contactId', 'membershipId'],
Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton for my own knowledge what is the purpose of schema parameter in general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monishdeb yeah - I'm hoping we can flush that out in the docs - it became required (in master) to tell the token processors which entities to resolve - I'm not sure if that is good or bad. I was going to turn my attention to in once the anomalies with the actual tokens were in hand

@monishdeb
Copy link
Member

Tested the patch on local and didn't encounter any regression/breakage. Reviewed the patch and looks good.

@monishdeb monishdeb merged commit 89d280e into civicrm:master Sep 29, 2021
@eileenmcnaughton eileenmcnaughton deleted the member_tokens branch September 29, 2021 04:06
@eileenmcnaughton
Copy link
Contributor Author

woo hoo - that's the last call to replaceEntityTokens

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