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#2814 Fix profile edit to use messagetemplate:render #21364

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#2814 Fix profile edit to use messagetemplate:render

Before

Weird & wonderful way of resolving the tokens, no test

After

Test, uses renderTemplate

 $url = CRM_Core_BAO_MessageTemplate::renderTemplate([
        'messageTemplate' => ['msg_text' => $this->_postURL],
        'contactId' => $this->_id,
        'disableSmarty' => TRUE,
      ])['text'];

Technical Details

If the api were merged I would have used that - but this adds the test & when can test converting
to the api when it is merged. It gets the hard lifting of this conversion out of the way

  • note I put the notes below on the gitlab but thought I'd copy them here too

As of a 6 months ago, or so, we had resolved all differences between using the token processor and replaceContactTokens to ....replaceContactTokens.
We don't quite have the api yet but when complete it is pretty much a drop in replacement for renderTemplate

Calling renderTemplate directly is supported provided

  1. it is only done from core and
  2. there is test cover for each instance

We don't have that many places left so I'm going to push through & replace them.
The last place in core is the token processor itself. I intend to cut that dependency last & duplicate the code in some way onto the processor.
While not supported from outside of core replaceContactTokens IS called from a number of places. We are also firming up the new token api so I don't want to 'hustle' people to change their code until it's bedded in so I propose to deprecate quietly at this stage (@deprecated on the function but don't make it 'noisy' for a few more months)

Comments

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

If the api were merged I would have used that - but this adds the test & when can test converting
to the api when it is merged. It gets the hard lifting of this conversion out of the way
@civibot
Copy link

civibot bot commented Sep 3, 2021

(Standard links)

@colemanw
Copy link
Member

colemanw commented Sep 6, 2021

Refactor makes sense. This simplifies code & adds test coverage.

@colemanw colemanw merged commit f7fb561 into civicrm:master Sep 6, 2021
@eileenmcnaughton eileenmcnaughton deleted the prof_edit branch September 6, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants